From 40a9639295935c3fda3b945f1acba7c028d27e50 Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Thu, 21 Dec 2023 17:11:58 -0800 Subject: [PATCH 1/2] `struct {D,R}av1dITUTT35`: Skip `.update_rav1d()`ing it as we never read it. This also lets us remove the `{D => R}av1dITUTT35` conversion, which contains `unsafe`. --- include/dav1d/headers.rs | 19 ------------------- include/dav1d/picture.rs | 2 +- src/lib.rs | 9 +-------- 3 files changed, 2 insertions(+), 28 deletions(-) diff --git a/include/dav1d/headers.rs b/include/dav1d/headers.rs index 9e18ddc74..6bfe17d4d 100644 --- a/include/dav1d/headers.rs +++ b/include/dav1d/headers.rs @@ -2,7 +2,6 @@ use crate::src::enum_map::EnumKey; use std::ffi::c_int; use std::ffi::c_uint; use std::ops::BitAnd; -use std::slice; use strum::EnumCount; use strum::FromRepr; @@ -509,24 +508,6 @@ pub(crate) struct Rav1dITUTT35 { pub payload: Box<[u8]>, } -impl From for Rav1dITUTT35 { - fn from(value: Dav1dITUTT35) -> Self { - let Dav1dITUTT35 { - country_code, - country_code_extension_byte, - payload_size, - payload, - } = value; - let payload = unsafe { slice::from_raw_parts_mut(payload, payload_size) }; - let payload = unsafe { Box::from_raw(payload) }; - Self { - country_code, - country_code_extension_byte, - payload, - } - } -} - impl From for Dav1dITUTT35 { fn from(value: Rav1dITUTT35) -> Self { let Rav1dITUTT35 { diff --git a/include/dav1d/picture.rs b/include/dav1d/picture.rs index e75f1f29f..5d29d60cf 100644 --- a/include/dav1d/picture.rs +++ b/include/dav1d/picture.rs @@ -167,7 +167,7 @@ impl From for Rav1dPicture { content_light: content_light_ref.map(|raw| unsafe { raw.into_arc() }), // Safety: `raw` came from [`RawArc::from_arc`]. mastering_display: mastering_display_ref.map(|raw| unsafe { raw.into_arc() }), - // `.update_rav1d()` happens in `#[no_mangle] extern "C"`/`DAV1D_API` calls + // We don't `.update_rav1d` [`Rav1dITUTT35`] because never read it. itut_t35: if itut_t35.is_null() { ptr::null_mut() } else { diff --git a/src/lib.rs b/src/lib.rs index 20223af72..d5210c2ea 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -16,7 +16,6 @@ use crate::include::dav1d::dav1d::RAV1D_DECODEFRAMETYPE_KEY; use crate::include::dav1d::dav1d::RAV1D_INLOOPFILTER_ALL; use crate::include::dav1d::headers::DRav1d; use crate::include::dav1d::headers::Dav1dFrameHeader; -use crate::include::dav1d::headers::Dav1dITUTT35; use crate::include::dav1d::headers::Dav1dSequenceHeader; use crate::include::dav1d::headers::Rav1dFilmGrainData; use crate::include::dav1d::headers::Rav1dFrameHeader; @@ -845,13 +844,7 @@ pub unsafe extern "C" fn dav1d_apply_grain( .cast::>()) .update_rav1d(); } - if let Some(mut itut_t35_ref) = NonNull::new(in_0.itut_t35_ref) { - (*itut_t35_ref - .as_mut() - .data - .cast::>()) - .update_rav1d(); - } + // Don't `.update_rav1d` [`Rav1dITUTT35`] because we never read it. let mut out_rust = MaybeUninit::zeroed().assume_init(); // TODO(kkysen) Temporary until we return it directly. let in_rust = in_0.into(); let result = rav1d_apply_grain(c, &mut out_rust, &in_rust); From 6ed6e8bef05885875d8d0d5a888a1cf784d7ebc9 Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Thu, 21 Dec 2023 17:48:09 -0800 Subject: [PATCH 2/2] `struct {D,R}av1dITUTT35`: Replace `Rav1dRef`s with `Option>>`s. --- include/dav1d/headers.rs | 4 ++-- include/dav1d/picture.rs | 43 +++++++++------------------------------- src/internal.rs | 5 +++-- src/lib.rs | 8 +++----- src/obu.rs | 21 ++++++-------------- src/picture.rs | 40 +++++++++---------------------------- tests/seek_stress.rs | 9 ++++----- tools/dav1d.rs | 5 ++--- 8 files changed, 38 insertions(+), 97 deletions(-) diff --git a/include/dav1d/headers.rs b/include/dav1d/headers.rs index 6bfe17d4d..e5ca51674 100644 --- a/include/dav1d/headers.rs +++ b/include/dav1d/headers.rs @@ -8,7 +8,7 @@ use strum::FromRepr; /// This is so we can store both `*mut D` and `*mut R` /// for maintaining `dav1d` ABI compatibility, /// where `D` is the `Dav1d*` type and `R` is the `Rav1d` type. -pub(crate) struct DRav1d { +pub struct DRav1d { pub rav1d: R, pub dav1d: D, } @@ -502,7 +502,7 @@ pub struct Dav1dITUTT35 { #[derive(Clone)] #[repr(C)] -pub(crate) struct Rav1dITUTT35 { +pub struct Rav1dITUTT35 { pub country_code: u8, pub country_code_extension_byte: u8, pub payload: Box<[u8]>, diff --git a/include/dav1d/picture.rs b/include/dav1d/picture.rs index 5d29d60cf..4cfe014ac 100644 --- a/include/dav1d/picture.rs +++ b/include/dav1d/picture.rs @@ -77,13 +77,13 @@ pub struct Dav1dPicture { pub m: Dav1dDataProps, pub content_light: Option>, pub mastering_display: Option>, - pub itut_t35: *mut Dav1dITUTT35, + pub itut_t35: Option>, pub reserved: [uintptr_t; 4], pub frame_hdr_ref: *mut Dav1dRef, pub seq_hdr_ref: *mut Dav1dRef, pub content_light_ref: Option>, // opaque, so we can change this pub mastering_display_ref: Option>, // opaque, so we can change this - pub itut_t35_ref: *mut Dav1dRef, + pub itut_t35_ref: Option>>, // opaque, so we can change this pub reserved_ref: [uintptr_t; 4], pub r#ref: *mut Dav1dRef, pub allocator_data: *mut c_void, @@ -100,10 +100,9 @@ pub(crate) struct Rav1dPicture { pub m: Rav1dDataProps, pub content_light: Option>, pub mastering_display: Option>, - pub itut_t35: *mut Rav1dITUTT35, + pub itut_t35: Option>>, pub frame_hdr_ref: *mut Rav1dRef, pub seq_hdr_ref: *mut Rav1dRef, - pub itut_t35_ref: *mut Rav1dRef, pub r#ref: *mut Rav1dRef, pub allocator_data: *mut c_void, } @@ -119,7 +118,7 @@ impl From for Rav1dPicture { m, content_light: _, mastering_display: _, - itut_t35, + itut_t35: _, reserved: _, frame_hdr_ref, seq_hdr_ref, @@ -168,21 +167,10 @@ impl From for Rav1dPicture { // Safety: `raw` came from [`RawArc::from_arc`]. mastering_display: mastering_display_ref.map(|raw| unsafe { raw.into_arc() }), // We don't `.update_rav1d` [`Rav1dITUTT35`] because never read it. - itut_t35: if itut_t35.is_null() { - ptr::null_mut() - } else { - unsafe { - addr_of_mut!( - (*(itut_t35_ref.read()) - .data - .cast::>()) - .rav1d - ) - } - }, + // Safety: `raw` came from [`RawArc::from_arc`]. + itut_t35: itut_t35_ref.map(|raw| unsafe { raw.into_arc() }), frame_hdr_ref, seq_hdr_ref, - itut_t35_ref, r#ref, allocator_data, } @@ -203,7 +191,6 @@ impl From for Dav1dPicture { itut_t35, frame_hdr_ref, seq_hdr_ref, - itut_t35_ref, r#ref, allocator_data, } = value; @@ -243,24 +230,13 @@ impl From for Dav1dPicture { content_light: content_light.as_ref().map(|arc| arc.as_ref().into()), mastering_display: mastering_display.as_ref().map(|arc| arc.as_ref().into()), // `DRav1d::from_rav1d` is called in [`rav1d_parse_obus`]. - itut_t35: if itut_t35.is_null() { - ptr::null_mut() - } else { - unsafe { - addr_of_mut!( - (*(itut_t35_ref.read()) - .data - .cast::>()) - .dav1d - ) - } - }, + itut_t35: itut_t35.as_ref().map(|arc| (&arc.as_ref().dav1d).into()), reserved: Default::default(), frame_hdr_ref, seq_hdr_ref, content_light_ref: content_light.map(RawArc::from_arc), mastering_display_ref: mastering_display.map(RawArc::from_arc), - itut_t35_ref, + itut_t35_ref: itut_t35.map(RawArc::from_arc), reserved_ref: Default::default(), r#ref, allocator_data, @@ -284,10 +260,9 @@ impl Default for Rav1dPicture { m: Default::default(), content_light: None, mastering_display: None, - itut_t35: ptr::null_mut(), + itut_t35: None, frame_hdr_ref: ptr::null_mut(), seq_hdr_ref: ptr::null_mut(), - itut_t35_ref: ptr::null_mut(), r#ref: ptr::null_mut(), allocator_data: ptr::null_mut(), } diff --git a/src/internal.rs b/src/internal.rs index fce2f104e..0a43d5095 100644 --- a/src/internal.rs +++ b/src/internal.rs @@ -8,6 +8,8 @@ use crate::include::dav1d::data::Rav1dData; use crate::include::dav1d::dav1d::Rav1dDecodeFrameType; use crate::include::dav1d::dav1d::Rav1dEventFlags; use crate::include::dav1d::dav1d::Rav1dInloopFilterType; +use crate::include::dav1d::headers::DRav1d; +use crate::include::dav1d::headers::Dav1dITUTT35; use crate::include::dav1d::headers::Rav1dContentLightLevel; use crate::include::dav1d::headers::Rav1dFrameHeader; use crate::include::dav1d::headers::Rav1dITUTT35; @@ -211,8 +213,7 @@ pub struct Rav1dContext { pub(crate) frame_hdr: *mut Rav1dFrameHeader, pub(crate) content_light: Option>, pub(crate) mastering_display: Option>, - pub(crate) itut_t35_ref: *mut Rav1dRef, - pub(crate) itut_t35: *mut Rav1dITUTT35, + pub(crate) itut_t35: Option>>, // decoded output picture queue pub(crate) in_0: Rav1dData, diff --git a/src/lib.rs b/src/lib.rs index d5210c2ea..cb578936a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -19,7 +19,6 @@ use crate::include::dav1d::headers::Dav1dFrameHeader; use crate::include::dav1d::headers::Dav1dSequenceHeader; use crate::include::dav1d::headers::Rav1dFilmGrainData; use crate::include::dav1d::headers::Rav1dFrameHeader; -use crate::include::dav1d::headers::Rav1dITUTT35; use crate::include::dav1d::headers::Rav1dSequenceHeader; use crate::include::dav1d::picture::Dav1dPicture; use crate::include::dav1d::picture::Rav1dPicture; @@ -844,7 +843,7 @@ pub unsafe extern "C" fn dav1d_apply_grain( .cast::>()) .update_rav1d(); } - // Don't `.update_rav1d` [`Rav1dITUTT35`] because we never read it. + // Don't `.update_rav1d()` [`Rav1dITUTT35`] because we never read it. let mut out_rust = MaybeUninit::zeroed().assume_init(); // TODO(kkysen) Temporary until we return it directly. let in_rust = in_0.into(); let result = rav1d_apply_grain(c, &mut out_rust, &in_rust); @@ -879,8 +878,7 @@ pub(crate) unsafe fn rav1d_flush(c: *mut Rav1dContext) { rav1d_ref_dec(&mut (*c).seq_hdr_ref); let _ = mem::take(&mut (*c).content_light); let _ = mem::take(&mut (*c).mastering_display); - (*c).itut_t35 = 0 as *mut Rav1dITUTT35; - rav1d_ref_dec(&mut (*c).itut_t35_ref); + let _ = mem::take(&mut (*c).itut_t35); let _ = mem::take(&mut (*c).cached_error_props); if (*c).n_fc == 1 as c_uint && (*c).n_tc == 1 as c_uint { return; @@ -1086,7 +1084,7 @@ unsafe fn close_internal(c_out: &mut *mut Rav1dContext, flush: c_int) { rav1d_ref_dec(&mut (*c).frame_hdr_ref); let _ = mem::take(&mut (*c).mastering_display); let _ = mem::take(&mut (*c).content_light); - rav1d_ref_dec(&mut (*c).itut_t35_ref); + let _ = mem::take(&mut (*c).itut_t35); rav1d_mem_pool_end((*c).seq_hdr_pool); rav1d_mem_pool_end((*c).frame_hdr_pool); rav1d_mem_pool_end((*c).segmap_pool); diff --git a/src/obu.rs b/src/obu.rs index f95051a62..d34814324 100644 --- a/src/obu.rs +++ b/src/obu.rs @@ -2464,16 +2464,11 @@ unsafe fn parse_obus(c: &mut Rav1dContext, r#in: &Rav1dData, global: bool) -> Ra let country_code_extension_byte = country_code_extension_byte as u8; let payload = (0..payload_size).map(|_| gb.get_bits(8) as u8).collect(); // TODO(kkysen) fallible allocation - let itut_t35_metadatas = - (*r#ref).data.cast::>(); - itut_t35_metadatas.write(DRav1d::from_rav1d(Rav1dITUTT35 { + c.itut_t35 = Some(Arc::new(DRav1d::from_rav1d(Rav1dITUTT35 { country_code, country_code_extension_byte, payload, - })); - rav1d_ref_dec(&mut c.itut_t35_ref); - c.itut_t35 = &mut (*itut_t35_metadatas).rav1d; - c.itut_t35_ref = r#ref; + }))); // TODO(kkysen) fallible allocation } } OBU_META_SCALABILITY | OBU_META_TIMECODE => {} // Ignore metadata OBUs we don't care about. @@ -2542,13 +2537,11 @@ unsafe fn parse_obus(c: &mut Rav1dContext, r#in: &Rav1dData, global: bool) -> Ra &mut (*c).out.p, &c.content_light, &c.mastering_display, - c.itut_t35, - c.itut_t35_ref, + &c.itut_t35, &r#in.m, ); // Must be removed from the context after being attached to the frame - rav1d_ref_dec(&mut c.itut_t35_ref); - c.itut_t35 = 0 as *mut Rav1dITUTT35; + let _ = mem::take(&mut c.itut_t35); c.event_flags |= c.refs[(*c.frame_hdr).existing_frame_idx as usize] .p .flags @@ -2617,13 +2610,11 @@ unsafe fn parse_obus(c: &mut Rav1dContext, r#in: &Rav1dData, global: bool) -> Ra &mut (*out_delayed).p, &c.content_light, &c.mastering_display, - c.itut_t35, - c.itut_t35_ref, + &c.itut_t35, &r#in.m, ); // Must be removed from the context after being attached to the frame - rav1d_ref_dec(&mut c.itut_t35_ref); - c.itut_t35 = 0 as *mut Rav1dITUTT35; + let _ = mem::take(&mut c.itut_t35); pthread_mutex_unlock(&mut c.task_thread.lock); } if (*c.refs[(*c.frame_hdr).existing_frame_idx as usize] diff --git a/src/picture.rs b/src/picture.rs index e612040f2..256981f8e 100644 --- a/src/picture.rs +++ b/src/picture.rs @@ -1,6 +1,8 @@ use crate::include::common::validate::validate_input; use crate::include::dav1d::common::Rav1dDataProps; use crate::include::dav1d::dav1d::Rav1dEventFlags; +use crate::include::dav1d::headers::DRav1d; +use crate::include::dav1d::headers::Dav1dITUTT35; use crate::include::dav1d::headers::Rav1dContentLightLevel; use crate::include::dav1d::headers::Rav1dFrameHeader; use crate::include::dav1d::headers::Rav1dITUTT35; @@ -178,8 +180,7 @@ unsafe fn picture_alloc_with_edges( frame_hdr_ref: *mut Rav1dRef, content_light: &Option>, mastering_display: &Option>, - itut_t35: *mut Rav1dITUTT35, - itut_t35_ref: *mut Rav1dRef, + itut_t35: &Option>>, bpc: c_int, props: *const Rav1dDataProps, p_allocator: *mut Rav1dPicAllocator, @@ -239,14 +240,7 @@ unsafe fn picture_alloc_with_edges( if !frame_hdr_ref.is_null() { rav1d_ref_inc(frame_hdr_ref); } - rav1d_picture_copy_props( - p, - content_light, - mastering_display, - itut_t35, - itut_t35_ref, - props, - ); + rav1d_picture_copy_props(p, content_light, mastering_display, itut_t35, props); if extra != 0 && !extra_ptr.is_null() { *extra_ptr = &mut (*pic_ctx).extra_ptr as *mut *mut c_void as *mut c_void; @@ -259,21 +253,13 @@ pub unsafe fn rav1d_picture_copy_props( p: *mut Rav1dPicture, content_light: &Option>, mastering_display: &Option>, - itut_t35: *mut Rav1dITUTT35, - itut_t35_ref: *mut Rav1dRef, + itut_t35: &Option>>, props: *const Rav1dDataProps, ) { (*p).m = (*props).clone(); - (*p).content_light = content_light.clone(); (*p).mastering_display = mastering_display.clone(); - - rav1d_ref_dec(&mut (*p).itut_t35_ref); - (*p).itut_t35_ref = itut_t35_ref; - (*p).itut_t35 = itut_t35; - if !itut_t35_ref.is_null() { - rav1d_ref_inc(itut_t35_ref); - } + (*p).itut_t35 = itut_t35.clone(); } pub(crate) unsafe fn rav1d_thread_picture_alloc( @@ -294,8 +280,7 @@ pub(crate) unsafe fn rav1d_thread_picture_alloc( (*f).frame_hdr_ref, &(*c).content_light, &(*c).mastering_display, - (*c).itut_t35, - (*c).itut_t35_ref, + &(*c).itut_t35, bpc, &mut (*f).tiles[0].data.m, &mut (*c).allocator, @@ -309,8 +294,7 @@ pub(crate) unsafe fn rav1d_thread_picture_alloc( if res.is_err() { return res; } - rav1d_ref_dec(&mut (*c).itut_t35_ref); - (*c).itut_t35 = 0 as *mut Rav1dITUTT35; + let _ = mem::take(&mut (*c).itut_t35); let flags_mask = if (*(*f).frame_hdr).show_frame != 0 || (*c).output_invisible_frames { PictureFlags::empty() } else { @@ -345,8 +329,7 @@ pub(crate) unsafe fn rav1d_picture_alloc_copy( (*src).frame_hdr_ref, &(*src).content_light, &(*src).mastering_display, - (*src).itut_t35, - (*src).itut_t35_ref, + &(*src).itut_t35, (*src).p.bpc, &(*src).m, &mut (*pic_ctx).allocator, @@ -372,9 +355,6 @@ pub(crate) unsafe fn rav1d_picture_ref(dst: &mut Rav1dPicture, src: &Rav1dPictur if !src.seq_hdr_ref.is_null() { rav1d_ref_inc(src.seq_hdr_ref); } - if !src.itut_t35_ref.is_null() { - rav1d_ref_inc(src.itut_t35_ref); - } *dst = src.clone(); } @@ -415,7 +395,6 @@ pub(crate) unsafe fn rav1d_picture_unref_internal(p: &mut Rav1dPicture) { mut r#ref, mut frame_hdr_ref, mut seq_hdr_ref, - mut itut_t35_ref, .. } = mem::take(p); if !r#ref.is_null() { @@ -426,7 +405,6 @@ pub(crate) unsafe fn rav1d_picture_unref_internal(p: &mut Rav1dPicture) { } rav1d_ref_dec(&mut seq_hdr_ref); rav1d_ref_dec(&mut frame_hdr_ref); - rav1d_ref_dec(&mut itut_t35_ref); } pub(crate) unsafe fn rav1d_thread_picture_unref(p: *mut Rav1dThreadPicture) { diff --git a/tests/seek_stress.rs b/tests/seek_stress.rs index bd3ee36d1..fd1b63426 100644 --- a/tests/seek_stress.rs +++ b/tests/seek_stress.rs @@ -48,7 +48,6 @@ use rav1d::include::dav1d::dav1d::DAV1D_DECODEFRAMETYPE_ALL; use rav1d::include::dav1d::dav1d::DAV1D_INLOOPFILTER_NONE; use rav1d::include::dav1d::headers::Dav1dColorPrimaries; use rav1d::include::dav1d::headers::Dav1dFrameHeader; -use rav1d::include::dav1d::headers::Dav1dITUTT35; use rav1d::include::dav1d::headers::Dav1dSequenceHeader; use rav1d::include::dav1d::headers::Dav1dSequenceHeaderOperatingParameterInfo; use rav1d::include::dav1d::headers::Dav1dSequenceHeaderOperatingPoint; @@ -169,13 +168,13 @@ unsafe fn decode_rand( }, content_light: None, mastering_display: None, - itut_t35: 0 as *mut Dav1dITUTT35, + itut_t35: None, reserved: [0; 4], frame_hdr_ref: 0 as *mut Dav1dRef, seq_hdr_ref: 0 as *mut Dav1dRef, content_light_ref: None, mastering_display_ref: None, - itut_t35_ref: 0 as *mut Dav1dRef, + itut_t35_ref: None, reserved_ref: [0; 4], r#ref: 0 as *mut Dav1dRef, allocator_data: 0 as *mut c_void, @@ -224,13 +223,13 @@ unsafe fn decode_all( }, content_light: None, mastering_display: None, - itut_t35: 0 as *mut Dav1dITUTT35, + itut_t35: None, reserved: [0; 4], frame_hdr_ref: 0 as *mut Dav1dRef, seq_hdr_ref: 0 as *mut Dav1dRef, content_light_ref: None, mastering_display_ref: None, - itut_t35_ref: 0 as *mut Dav1dRef, + itut_t35_ref: None, reserved_ref: [0; 4], r#ref: 0 as *mut Dav1dRef, allocator_data: 0 as *mut c_void, diff --git a/tools/dav1d.rs b/tools/dav1d.rs index 2df26eb20..b03d2df85 100644 --- a/tools/dav1d.rs +++ b/tools/dav1d.rs @@ -65,7 +65,6 @@ use rav1d::include::dav1d::dav1d::DAV1D_DECODEFRAMETYPE_ALL; use rav1d::include::dav1d::dav1d::DAV1D_INLOOPFILTER_NONE; use rav1d::include::dav1d::headers::Dav1dColorPrimaries; use rav1d::include::dav1d::headers::Dav1dFrameHeader; -use rav1d::include::dav1d::headers::Dav1dITUTT35; use rav1d::include::dav1d::headers::Dav1dSequenceHeader; use rav1d::include::dav1d::headers::Dav1dSequenceHeaderOperatingParameterInfo; use rav1d::include::dav1d::headers::Dav1dSequenceHeaderOperatingPoint; @@ -326,13 +325,13 @@ unsafe fn main_0(argc: c_int, argv: *const *mut c_char) -> c_int { }, content_light: None, mastering_display: None, - itut_t35: 0 as *mut Dav1dITUTT35, + itut_t35: None, reserved: [0; 4], frame_hdr_ref: 0 as *mut Dav1dRef, seq_hdr_ref: 0 as *mut Dav1dRef, content_light_ref: None, mastering_display_ref: None, - itut_t35_ref: 0 as *mut Dav1dRef, + itut_t35_ref: None, reserved_ref: [0; 4], r#ref: 0 as *mut Dav1dRef, allocator_data: 0 as *mut c_void,