struct Rav1dPic{tureData,Allocator}: Refactor in preparation for `A…

…rc`ification (#685)

I've been trying to `Arc`ify this `Rav1dPictureData`, but keep running
into a bug with `Rav1dFrameHeader`'s `Arc` getting corrupted and having
a ref count `> isize::MAX`. To narrow down a smaller change, this is a
set of changes in preparation for the actual `Arc`ification that don't
have any issues.

include/dav1d/picture.rs

@@ -17,7 +17,6 @@ use crate::src::c_arc::RawArc;
 use crate::src::error::Dav1dResult;
 use crate::src::error::Rav1dError;
 use crate::src::error::Rav1dError::EINVAL;
-use crate::src::error::Rav1dResult;
 use crate::src::r#ref::Rav1dRef;
 use libc::ptrdiff_t;
 use libc::uintptr_t;
@@ -30,6 +29,7 @@ use std::sync::Arc;
 pub(crate) const RAV1D_PICTURE_ALIGNMENT: usize = 64;
 pub const DAV1D_PICTURE_ALIGNMENT: usize = RAV1D_PICTURE_ALIGNMENT;
 
+#[derive(Default)]
 #[repr(C)]
 pub struct Dav1dPictureParameters {
     pub w: c_int,
@@ -72,11 +72,12 @@ impl From<Rav1dPictureParameters> for Dav1dPictureParameters {
     }
 }
 
+#[derive(Default)]
 #[repr(C)]
 pub struct Dav1dPicture {
     pub seq_hdr: Option<NonNull<Dav1dSequenceHeader>>,
     pub frame_hdr: Option<NonNull<Dav1dFrameHeader>>,
-    pub data: [*mut c_void; 3],
+    pub data: [Option<NonNull<c_void>>; 3],
     pub stride: [ptrdiff_t; 2],
     pub p: Dav1dPictureParameters,
     pub m: Dav1dDataProps,
@@ -90,24 +91,43 @@ pub struct Dav1dPicture {
     pub mastering_display_ref: Option<RawArc<Rav1dMasteringDisplay>>, // opaque, so we can change this
     pub itut_t35_ref: Option<RawArc<DRav1d<Rav1dITUTT35, Dav1dITUTT35>>>, // opaque, so we can change this
     pub reserved_ref: [uintptr_t; 4],
-    pub r#ref: *mut Dav1dRef,
-    pub allocator_data: *mut c_void,
+    pub r#ref: Option<NonNull<Dav1dRef>>,
+    pub allocator_data: Option<NonNull<c_void>>,
 }
 
 #[derive(Clone)]
+pub(crate) struct Rav1dPictureData {
+    pub data: [*mut c_void; 3],
+    pub allocator_data: Option<NonNull<c_void>>,
+}
+
+impl Default for Rav1dPictureData {
+    fn default() -> Self {
+        Self {
+            data: [ptr::null_mut(); 3],
+            allocator_data: Default::default(),
+        }
+    }
+}
+
+// TODO(kkysen) Eventually the [`impl Default`] might not be needed.
+// It's needed currently for a [`mem::take`] that simulates a move,
+// but once everything is Rusty, we may not need to clear the `dst` anymore.
+// This also applies to the `#[derive(Default)]`
+// on [`Rav1dPictureParameters`] and [`Rav1dPixelLayout`].
+#[derive(Clone, Default)]
 #[repr(C)]
 pub(crate) struct Rav1dPicture {
     pub seq_hdr: Option<Arc<DRav1d<Rav1dSequenceHeader, Dav1dSequenceHeader>>>,
     pub frame_hdr: Option<Arc<DRav1d<Rav1dFrameHeader, Dav1dFrameHeader>>>,
-    pub data: [*mut c_void; 3],
+    pub data: Rav1dPictureData,
     pub stride: [ptrdiff_t; 2],
     pub p: Rav1dPictureParameters,
     pub m: Rav1dDataProps,
     pub content_light: Option<Arc<Rav1dContentLightLevel>>,
     pub mastering_display: Option<Arc<Rav1dMasteringDisplay>>,
     pub itut_t35: Option<Arc<DRav1d<Rav1dITUTT35, Dav1dITUTT35>>>,
-    pub r#ref: *mut Rav1dRef,
-    pub allocator_data: *mut c_void,
+    pub r#ref: Option<NonNull<Rav1dRef>>,
 }
 
 impl From<Dav1dPicture> for Rav1dPicture {
@@ -139,7 +159,10 @@ impl From<Dav1dPicture> for Rav1dPicture {
             // We don't `.update_rav1d()` [`Rav1dFrameHeader`] because it's meant to be read-only.
             // Safety: `raw` came from [`RawArc::from_arc`].
             frame_hdr: frame_hdr_ref.map(|raw| unsafe { raw.into_arc() }),
-            data,
+            data: Rav1dPictureData {
+                data: data.map(|data| data.map_or_else(ptr::null_mut, NonNull::as_ptr)),
+                allocator_data,
+            },
             stride,
             p: p.into(),
             m: m.into(),
@@ -151,7 +174,6 @@ impl From<Dav1dPicture> for Rav1dPicture {
             // Safety: `raw` came from [`RawArc::from_arc`].
             itut_t35: itut_t35_ref.map(|raw| unsafe { raw.into_arc() }),
             r#ref,
-            allocator_data,
         }
     }
 }
@@ -161,22 +183,25 @@ impl From<Rav1dPicture> for Dav1dPicture {
         let Rav1dPicture {
             seq_hdr,
             frame_hdr,
-            data,
+            data:
+                Rav1dPictureData {
+                    data,
+                    allocator_data,
+                },
             stride,
             p,
             m,
             content_light,
             mastering_display,
             itut_t35,
             r#ref,
-            allocator_data,
         } = value;
         Self {
             // [`DRav1d::from_rav1d`] is called right after [`parse_seq_hdr`].
             seq_hdr: seq_hdr.as_ref().map(|arc| (&arc.as_ref().dav1d).into()),
             // [`DRav1d::from_rav1d`] is called in [`parse_frame_hdr`].
             frame_hdr: frame_hdr.as_ref().map(|arc| (&arc.as_ref().dav1d).into()),
-            data,
+            data: data.map(NonNull::new),
             stride,
             p: p.into(),
             m: m.into(),
@@ -197,29 +222,6 @@ impl From<Rav1dPicture> for Dav1dPicture {
     }
 }
 
-// TODO(kkysen) Eventually the [`impl Default`] might not be needed.
-// It's needed currently for a [`mem::take`] that simulates a move,
-// but once everything is Rusty, we may not need to clear the `dst` anymore.
-// This also applies to the `#[derive(Default)]`
-// on [`Rav1dPictureParameters`] and [`Rav1dPixelLayout`].
-impl Default for Rav1dPicture {
-    fn default() -> Self {
-        Self {
-            seq_hdr: None,
-            frame_hdr: None,
-            data: [ptr::null_mut(); 3],
-            stride: Default::default(),
-            p: Default::default(),
-            m: Default::default(),
-            content_light: None,
-            mastering_display: None,
-            itut_t35: None,
-            r#ref: ptr::null_mut(),
-            allocator_data: ptr::null_mut(),
-        }
-    }
-}
-
 #[derive(Clone)]
 #[repr(C)]
 pub struct Dav1dPicAllocator {
@@ -248,6 +250,14 @@ pub struct Dav1dPicAllocator {
     ///     with a custom pointer that will be passed to
     ///     [`release_picture_callback`].
     ///
+    ///     The only fields of `pic` that will be already set are:
+    ///     * [`Dav1dPicture::p`]
+    ///     * [`Dav1dPicture::seq_hdr`]
+    ///     * [`Dav1dPicture::frame_hdr`]
+    ///     
+    ///     This is not a change from the original `DAV1D_API`,
+    ///     just a clarification of it.
+    ///
     /// * `cookie`: Custom pointer passed to all calls.
     ///
     /// *Note*: No fields other than [`data`], [`stride`] and [`allocator_data`]
@@ -279,6 +289,23 @@ pub struct Dav1dPicAllocator {
     /// # Args
     ///
     /// * `pic`: The picture that was filled by [`alloc_picture_callback`].
+    ///     
+    ///     The only fields of `pic` that will be set are
+    ///     the ones allocated by [`Self::alloc_picture_callback`]:
+    ///     * [`Dav1dPicture::data`]
+    ///     * [`Dav1dPicture::allocator_data`]
+    ///     
+    ///     NOTE: This is a slight change from the original `DAV1D_API`, which was underspecified.
+    ///     However, all known uses of this API follow this already:
+    ///     * `libdav1d`: [`dav1d_default_picture_release`](https://code.videolan.org/videolan/dav1d/-/blob/16ed8e8b99f2fcfffe016e929d3626e15267ad3e/src/picture.c#L85-87)
+    ///     * `dav1d`: [`picture_release`](https://code.videolan.org/videolan/dav1d/-/blob/16ed8e8b99f2fcfffe016e929d3626e15267ad3e/tools/dav1d.c#L180-182)
+    ///     * `dav1dplay`: [`placebo_release_pic`](https://code.videolan.org/videolan/dav1d/-/blob/16ed8e8b99f2fcfffe016e929d3626e15267ad3e/examples/dp_renderer_placebo.c#L375-383)
+    ///     * `libplacebo`: [`pl_release_dav1dpicture`](https://github.com/haasn/libplacebo/blob/34e019bfedaa5a64f268d8f9263db352c0a8f67f/src/include/libplacebo/utils/dav1d_internal.h#L594-L607)
+    ///     * `ffmpeg`: [`libdav1d_picture_release`](https://github.com/FFmpeg/FFmpeg/blob/00b288da73f45acb78b74bcc40f73c7ba1fff7cb/libavcodec/libdav1d.c#L124-L129)
+    ///
+    ///     Making this API safe without this slight tightening of the API
+    ///     [is very difficult](https://github.com/memorysafety/rav1d/pull/685#discussion_r1458171639).
+    ///
     /// * `cookie`: Custom pointer passed to all calls.
     ///
     /// [`dav1d_get_picture`]: crate::src::lib::dav1d_get_picture
@@ -343,18 +370,3 @@ impl From<Rav1dPicAllocator> for Dav1dPicAllocator {
         }
     }
 }
-
-impl Rav1dPicAllocator {
-    pub unsafe fn alloc_picture(&self, p: *mut Rav1dPicture) -> Rav1dResult {
-        let mut p_c = p.read().into();
-        let result = (self.alloc_picture_callback)(&mut p_c, self.cookie);
-        p.write(p_c.into());
-        result.try_into().unwrap()
-    }
-
-    pub unsafe fn release_picture(&self, p: *mut Rav1dPicture) {
-        let mut p_c = p.read().into();
-        (self.release_picture_callback)(&mut p_c, self.cookie);
-        p.write(p_c.into());
-    }
-}

src/decode.rs

@@ -4674,8 +4674,8 @@ pub(crate) unsafe fn rav1d_decode_frame_init(f: &mut Rav1dFrameContext) -> Rav1d
     // what they point at, as long as the pointers are valid.
     let has_chroma = (f.cur.p.layout != Rav1dPixelLayout::I400) as usize;
     f.lf.mask_ptr = f.lf.mask;
-    f.lf.p = array::from_fn(|i| f.cur.data[has_chroma * i].cast());
-    f.lf.sr_p = array::from_fn(|i| f.sr_cur.p.data[has_chroma * i].cast());
+    f.lf.p = array::from_fn(|i| f.cur.data.data[has_chroma * i].cast());
+    f.lf.sr_p = array::from_fn(|i| f.sr_cur.p.data.data[has_chroma * i].cast());
 
     Ok(())
 }
@@ -4849,7 +4849,7 @@ unsafe fn rav1d_decode_frame_main(f: &mut Rav1dFrameContext) -> Rav1dResult {
 
 pub(crate) unsafe fn rav1d_decode_frame_exit(f: &mut Rav1dFrameContext, retval: Rav1dResult) {
     let c = &*f.c;
-    if !f.sr_cur.p.data[0].is_null() {
+    if !f.sr_cur.p.data.data[0].is_null() {
         f.task_thread.error = AtomicI32::new(0);
     }
     if c.n_fc > 1 && retval.is_err() && !f.frame_thread.cf.is_null() {
@@ -4953,7 +4953,8 @@ pub unsafe fn rav1d_submit_frame(c: &mut Rav1dContext) -> Rav1dResult {
             pthread_cond_wait(&mut f.task_thread.cond, &mut c.task_thread.lock);
         }
         let out_delayed = &mut *c.frame_thread.out_delayed.offset(next as isize);
-        if !out_delayed.p.data[0].is_null() || f.task_thread.error.load(Ordering::SeqCst) != 0 {
+        if !out_delayed.p.data.data[0].is_null() || f.task_thread.error.load(Ordering::SeqCst) != 0
+        {
             let first = c.task_thread.first.load(Ordering::SeqCst);
             if first + 1 < c.n_fc {
                 c.task_thread.first.fetch_add(1, Ordering::SeqCst);
@@ -4976,7 +4977,7 @@ pub unsafe fn rav1d_submit_frame(c: &mut Rav1dContext) -> Rav1dResult {
             c.cached_error = error;
             c.cached_error_props = out_delayed.p.m.clone();
             rav1d_thread_picture_unref(out_delayed);
-        } else if !out_delayed.p.data[0].is_null() {
+        } else if !out_delayed.p.data.data[0].is_null() {
             let progress = out_delayed.progress.as_ref().unwrap()[1].load(Ordering::Relaxed);
             if (out_delayed.visible || c.output_invisible_frames) && progress != FRAME_ERROR {
                 rav1d_thread_picture_ref(&mut c.out, out_delayed);
@@ -5091,14 +5092,14 @@ pub unsafe fn rav1d_submit_frame(c: &mut Rav1dContext) -> Rav1dResult {
     if frame_hdr.frame_type.is_inter_or_switch() {
         if frame_hdr.primary_ref_frame != RAV1D_PRIMARY_REF_NONE {
             let pri_ref = frame_hdr.refidx[frame_hdr.primary_ref_frame as usize] as usize;
-            if c.refs[pri_ref].p.p.data[0].is_null() {
+            if c.refs[pri_ref].p.p.data.data[0].is_null() {
                 on_error(f, c, out_delayed);
                 return Err(EINVAL);
             }
         }
         for i in 0..7 {
             let refidx = frame_hdr.refidx[i] as usize;
-            if c.refs[refidx].p.p.data[0].is_null()
+            if c.refs[refidx].p.p.data.data[0].is_null()
                 || (frame_hdr.size.width[0] * 2) < c.refs[refidx].p.p.p.w
                 || (frame_hdr.size.height * 2) < c.refs[refidx].p.p.p.h
                 || frame_hdr.size.width[0] > c.refs[refidx].p.p.p.w * 16

src/fg_apply.rs

@@ -119,16 +119,16 @@ pub(crate) unsafe fn rav1d_prep_grain<BD: BitDepth>(
         let sz = out.p.h as isize * stride;
         if sz < 0 {
             memcpy(
-                (out.data[0] as *mut u8)
+                (out.data.data[0] as *mut u8)
                     .offset(sz as isize)
                     .offset(-(stride as isize)) as *mut c_void,
-                (r#in.data[0] as *mut u8)
+                (r#in.data.data[0] as *mut u8)
                     .offset(sz as isize)
                     .offset(-(stride as isize)) as *const c_void,
                 -sz as usize,
             );
         } else {
-            memcpy(out.data[0], r#in.data[0], sz as usize);
+            memcpy(out.data.data[0], r#in.data.data[0], sz as usize);
         }
     }
 
@@ -140,32 +140,32 @@ pub(crate) unsafe fn rav1d_prep_grain<BD: BitDepth>(
         if sz < 0 {
             if data.num_uv_points[0] == 0 {
                 memcpy(
-                    (out.data[1] as *mut u8)
+                    (out.data.data[1] as *mut u8)
                         .offset(sz as isize)
                         .offset(-(stride as isize)) as *mut c_void,
-                    (r#in.data[1] as *mut u8)
+                    (r#in.data.data[1] as *mut u8)
                         .offset(sz as isize)
                         .offset(-(stride as isize)) as *const c_void,
                     -sz as usize,
                 );
             }
             if data.num_uv_points[1] == 0 {
                 memcpy(
-                    (out.data[2] as *mut u8)
+                    (out.data.data[2] as *mut u8)
                         .offset(sz as isize)
                         .offset(-(stride as isize)) as *mut c_void,
-                    (r#in.data[2] as *mut u8)
+                    (r#in.data.data[2] as *mut u8)
                         .offset(sz as isize)
                         .offset(-(stride as isize)) as *const c_void,
                     -sz as usize,
                 );
             }
         } else {
             if data.num_uv_points[0] == 0 {
-                memcpy(out.data[1], r#in.data[1], sz as usize);
+                memcpy(out.data.data[1], r#in.data.data[1], sz as usize);
             }
             if data.num_uv_points[1] == 0 {
-                memcpy(out.data[2], r#in.data[2], sz as usize);
+                memcpy(out.data.data[2], r#in.data.data[2], sz as usize);
             }
         }
     }
@@ -188,15 +188,15 @@ pub(crate) unsafe fn rav1d_apply_grain_row<BD: BitDepth>(
     let ss_x = (r#in.p.layout != Rav1dPixelLayout::I444) as usize;
     let cpw = out.p.w as usize + ss_x >> ss_x;
     let is_id = seq_hdr.mtrx == RAV1D_MC_IDENTITY;
-    let luma_src = (r#in.data[0] as *mut BD::Pixel)
+    let luma_src = (r#in.data.data[0] as *mut BD::Pixel)
         .offset(((row * 32) as isize * BD::pxstride(r#in.stride[0] as usize) as isize) as isize);
     let bitdepth_max = (1 << out.p.bpc) - 1;
     let bd = BD::from_c(bitdepth_max);
 
     if data.num_y_points != 0 {
         let bh = cmp::min(out.p.h as usize - row * 32, 32);
         dsp.fgy_32x32xn.call(
-            (out.data[0] as *mut BD::Pixel).offset(
+            (out.data.data[0] as *mut BD::Pixel).offset(
                 ((row * 32) as isize * BD::pxstride(out.stride[0] as usize) as isize) as isize,
             ),
             luma_src.cast(),
@@ -230,8 +230,8 @@ pub(crate) unsafe fn rav1d_apply_grain_row<BD: BitDepth>(
     if data.chroma_scaling_from_luma {
         for pl in 0..2 {
             dsp.fguv_32x32xn[r#in.p.layout.try_into().unwrap()].call(
-                (out.data[1 + pl] as *mut BD::Pixel).offset(uv_off as isize),
-                (r#in.data[1 + pl] as *const BD::Pixel).offset(uv_off as isize),
+                (out.data.data[1 + pl] as *mut BD::Pixel).offset(uv_off as isize),
+                (r#in.data.data[1 + pl] as *const BD::Pixel).offset(uv_off as isize),
                 r#in.stride[1],
                 data,
                 cpw,
@@ -250,8 +250,8 @@ pub(crate) unsafe fn rav1d_apply_grain_row<BD: BitDepth>(
         for pl in 0..2 {
             if data.num_uv_points[pl] != 0 {
                 dsp.fguv_32x32xn[r#in.p.layout.try_into().unwrap()].call(
-                    (out.data[1 + pl] as *mut BD::Pixel).offset(uv_off as isize),
-                    (r#in.data[1 + pl] as *const BD::Pixel).offset(uv_off as isize),
+                    (out.data.data[1 + pl] as *mut BD::Pixel).offset(uv_off as isize),
+                    (r#in.data.data[1 + pl] as *const BD::Pixel).offset(uv_off as isize),
                     r#in.stride[1],
                     data_c,
                     cpw,