fn generate_grain_{y,uv}_rust: Make safe (#1128)

This makes the `fn generate_grain_{y,uv}_rust` fallbacks safe, and then marks all callers safe where possible. * Fixes #856. * Fixes #858. Note that for `mod fg_apply` (#856), it is made `#![deny(unsafe_code)]`. However, for `mod filmgrain` (#858), I didn't add a module-wide `#![deny(unsafe_op_in_unsafe_fn)]`, as I left the `unsafe fn *_neon` asm wrappers, as it's mostly just delegating to the asm `fn`s. The other few `ptr::add`s are done safely in the fallbacks, so adding `// SAFETY` comments here seems redundant. The other `unsafe fn *_c_erased` ones are individually marked `#[deny(unsafe_op_in_unsafe_fn)]`, as it makes sense for them.
memorysafety · May 28, 2024 · fd02926 · fd02926
2 parents decb534 + 7418502
commit fd02926
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 56 deletions.
diff --git a/src/fg_apply.rs b/src/fg_apply.rs
@@ -1,3 +1,5 @@
+#![deny(unsafe_code)]
+
 use crate::include::common::bitdepth::BitDepth;
 use crate::include::common::bitdepth::BPC;
 use crate::include::dav1d::headers::Rav1dMatrixCoefficients;
@@ -69,7 +71,7 @@ fn generate_scaling<BD: BitDepth>(bd: BD, points: &[[u8; 2]]) -> BD::Scaling {
     scaling_array
 }
 
-pub(crate) unsafe fn rav1d_prep_grain<BD: BitDepth>(
+pub(crate) fn rav1d_prep_grain<BD: BitDepth>(
     dsp: &Rav1dFilmGrainDSPContext,
     out: &mut Rav1dPicture,
     r#in: &Rav1dPicture,
@@ -80,27 +82,16 @@ pub(crate) unsafe fn rav1d_prep_grain<BD: BitDepth>(
     let data = &frame_hdr.film_grain.data;
     let bitdepth_max = (1 << out.p.bpc) - 1;
     let bd = BD::from_c(bitdepth_max);
+    let layout = || r#in.p.layout.try_into().unwrap();
 
     // Generate grain LUTs as needed
     let [grain_lut_0, grain_lut_1, grain_lut_2] = &mut grain_lut.0;
     dsp.generate_grain_y.call(grain_lut_0, data, bd);
     if data.num_uv_points[0] != 0 || data.chroma_scaling_from_luma {
-        dsp.generate_grain_uv[r#in.p.layout.try_into().unwrap()].call(
-            grain_lut_1,
-            grain_lut_0,
-            data,
-            false,
-            bd,
-        );
+        dsp.generate_grain_uv[layout()].call(grain_lut_1, grain_lut_0, data, false, bd);
     }
     if data.num_uv_points[1] != 0 || data.chroma_scaling_from_luma {
-        dsp.generate_grain_uv[r#in.p.layout.try_into().unwrap()].call(
-            grain_lut_2,
-            grain_lut_0,
-            data,
-            true,
-            bd,
-        );
+        dsp.generate_grain_uv[layout()].call(grain_lut_2, grain_lut_0, data, true, bd);
     }
 
     // Generate scaling LUTs as needed
@@ -230,7 +221,7 @@ pub(crate) fn rav1d_apply_grain_row<BD: BitDepth>(
     }
 }
 
-pub(crate) unsafe fn rav1d_apply_grain<BD: BitDepth>(
+pub(crate) fn rav1d_apply_grain<BD: BitDepth>(
     dsp: &Rav1dFilmGrainDSPContext,
     out: &mut Rav1dPicture,
     r#in: &Rav1dPicture,

diff --git a/src/filmgrain.rs b/src/filmgrain.rs
@@ -53,16 +53,17 @@ wrap_fn_ptr!(pub unsafe extern "C" fn generate_grain_y(
 ) -> ());
 
 impl generate_grain_y::Fn {
-    pub unsafe fn call<BD: BitDepth>(
+    pub fn call<BD: BitDepth>(
         &self,
         buf: &mut GrainLut<BD::Entry>,
         data: &Rav1dFilmGrainData,
         bd: BD,
     ) {
-        let buf = (buf as *mut GrainLut<BD::Entry>).cast();
+        let buf = ptr::from_mut(buf).cast();
         let data = &data.clone().into();
         let bd = bd.into_c();
-        self.get()(buf, data, bd)
+        // SAFETY: Fallback `fn generate_grain_y_rust` is safe; asm is supposed to do the same.
+        unsafe { self.get()(buf, data, bd) }
     }
 }
 
@@ -75,20 +76,21 @@ wrap_fn_ptr!(pub unsafe extern "C" fn generate_grain_uv(
 ) -> ());
 
 impl generate_grain_uv::Fn {
-    pub unsafe fn call<BD: BitDepth>(
+    pub fn call<BD: BitDepth>(
         &self,
         buf: &mut GrainLut<BD::Entry>,
         buf_y: &GrainLut<BD::Entry>,
         data: &Rav1dFilmGrainData,
         is_uv: bool,
         bd: BD,
     ) {
-        let buf = (buf as *mut GrainLut<BD::Entry>).cast();
-        let buf_y = (buf_y as *const GrainLut<BD::Entry>).cast();
+        let buf = ptr::from_mut(buf).cast();
+        let buf_y = ptr::from_ref(buf_y).cast();
         let data = &data.clone().into();
         let uv = is_uv.into();
         let bd = bd.into_c();
-        self.get()(buf, buf_y, data, uv, bd)
+        // SAFETY: Fallback `fn generate_grain_uv_rust` is safe; asm is supposed to do the same.
+        unsafe { self.get()(buf, buf_y, data, uv, bd) }
     }
 }
 
@@ -326,12 +328,16 @@ fn row_seed(rows: usize, row_num: usize, data: &Rav1dFilmGrainData) -> [c_uint;
     seed
 }
 
+/// # Safety
+///
+/// Must be called by [`generate_grain_y::Fn::call`].
+#[deny(unsafe_op_in_unsafe_fn)]
 unsafe extern "C" fn generate_grain_y_c_erased<BD: BitDepth>(
     buf: *mut GrainLut<DynEntry>,
     data: &Dav1dFilmGrainData,
     bitdepth_max: c_int,
 ) {
-    // Safety: Casting back to the original type from the `fn` ptr call.
+    // Safety: Casting back to the original type from the `generate_grain_y::Fn::call`.
     let buf = unsafe { &mut *buf.cast() };
     let data = &data.clone().into();
     let bd = BD::from_c(bitdepth_max);
@@ -340,7 +346,7 @@ unsafe extern "C" fn generate_grain_y_c_erased<BD: BitDepth>(
 
 const AR_PAD: usize = 3;
 
-unsafe fn generate_grain_y_rust<BD: BitDepth>(
+fn generate_grain_y_rust<BD: BitDepth>(
     buf: &mut GrainLut<BD::Entry>,
     data: &Rav1dFilmGrainData,
     bd: BD,
@@ -365,7 +371,7 @@ unsafe fn generate_grain_y_rust<BD: BitDepth>(
 
     for y in 0..GRAIN_HEIGHT - AR_PAD {
         for x in 0..GRAIN_WIDTH - 2 * AR_PAD {
-            let mut coeff = (data.ar_coeffs_y).as_ptr();
+            let mut coeff = &data.ar_coeffs_y[..];
             let mut sum = 0;
             for (dy, buf_row) in buf[y..][AR_PAD - ar_lag..=AR_PAD].iter().enumerate() {
                 for (dx, &buf_val) in buf_row[x..][AR_PAD - ar_lag..=AR_PAD + ar_lag]
@@ -375,19 +381,19 @@ unsafe fn generate_grain_y_rust<BD: BitDepth>(
                     if dx == ar_lag && dy == ar_lag {
                         break;
                     }
-                    sum += *coeff as c_int * buf_val.as_::<c_int>();
-                    coeff = coeff.offset(1);
+                    sum += coeff[0] as c_int * buf_val.as_::<c_int>();
+                    coeff = &coeff[1..];
                 }
             }
 
             let buf_yx = &mut buf[y + AR_PAD][x + AR_PAD];
             let grain = (*buf_yx).as_::<c_int>() + round2(sum, data.ar_coeff_shift);
-            (*buf_yx) = iclip(grain, grain_min, grain_max).as_::<BD::Entry>();
+            *buf_yx = iclip(grain, grain_min, grain_max).as_::<BD::Entry>();
         }
     }
 }
 
-unsafe fn generate_grain_uv_rust<BD: BitDepth>(
+fn generate_grain_uv_rust<BD: BitDepth>(
     buf: &mut GrainLut<BD::Entry>,
     buf_y: &GrainLut<BD::Entry>,
     data: &Rav1dFilmGrainData,
@@ -479,7 +485,7 @@ unsafe fn generate_grain_uv_rust<BD: BitDepth>(
 
     for y in 0..is_sub.len().0 {
         for x in 0..is_sub.len().1 {
-            let mut coeff = (data.ar_coeffs_uv[uv]).as_ptr();
+            let mut coeff = &data.ar_coeffs_uv[uv][..];
             let mut sum = 0;
             for (dy, buf_row) in buf[y..][AR_PAD - ar_lag..=AR_PAD].iter().enumerate() {
                 for (dx, &buf_val) in buf_row[x..][AR_PAD - ar_lag..=AR_PAD + ar_lag]
@@ -503,21 +509,25 @@ unsafe fn generate_grain_uv_rust<BD: BitDepth>(
                         }
                         luma = round2(luma, is_sub.y as u8 + is_sub.x as u8);
 
-                        sum += luma * *coeff as c_int;
+                        sum += luma * coeff[0] as c_int;
                         break;
                     }
-                    sum += *coeff as c_int * buf_val.as_::<c_int>();
-                    coeff = coeff.offset(1);
+                    sum += coeff[0] as c_int * buf_val.as_::<c_int>();
+                    coeff = &coeff[1..];
                 }
             }
 
             let buf_yx = &mut buf[y + AR_PAD][x + AR_PAD];
             let grain = (*buf_yx).as_::<c_int>() + round2(sum, data.ar_coeff_shift);
-            (*buf_yx) = iclip(grain, grain_min, grain_max).as_::<BD::Entry>();
+            *buf_yx = iclip(grain, grain_min, grain_max).as_::<BD::Entry>();
         }
     }
 }
 
+/// # Safety
+///
+/// Must be called by [`generate_grain_uv::Fn::call`].
+#[deny(unsafe_op_in_unsafe_fn)]
 #[inline(never)]
 unsafe extern "C" fn generate_grain_uv_c_erased<
     BD: BitDepth,
@@ -531,9 +541,9 @@ unsafe extern "C" fn generate_grain_uv_c_erased<
     uv: intptr_t,
     bitdepth_max: c_int,
 ) {
-    // Safety: Casting back to the original type from the `fn` ptr call.
+    // Safety: Casting back to the original type from the `generate_grain_uv::Fn::call`.
     let buf = unsafe { &mut *buf.cast() };
-    // Safety: Casting back to the original type from the `fn` ptr call.
+    // Safety: Casting back to the original type from the `generate_grain_uv::Fn::call`.
     let buf_y = unsafe { &*buf_y.cast() };
     let data = &data.clone().into();
     let is_uv = uv != 0;

diff --git a/src/lib.rs b/src/lib.rs
@@ -536,7 +536,7 @@ pub unsafe extern "C" fn dav1d_get_picture(
     .into()
 }
 
-pub(crate) unsafe fn rav1d_apply_grain(
+pub(crate) fn rav1d_apply_grain(
     c: &mut Rav1dContext,
     out: &mut Rav1dPicture,
     in_0: &Rav1dPicture,

diff --git a/src/thread_task.rs b/src/thread_task.rs
@@ -697,27 +697,21 @@ fn delayed_fg_task<'l, 'ttd: 'l>(
             match &mut delayed_fg.grain {
                 #[cfg(feature = "bitdepth_8")]
                 Grain::Bpc8(grain) => {
-                    // SAFETY: TODO make safe
-                    unsafe {
-                        rav1d_prep_grain::<BitDepth8>(
-                            dsp,
-                            &mut delayed_fg.out,
-                            &delayed_fg.in_0,
-                            grain,
-                        );
-                    }
+                    rav1d_prep_grain::<BitDepth8>(
+                        dsp,
+                        &mut delayed_fg.out,
+                        &delayed_fg.in_0,
+                        grain,
+                    );
                 }
                 #[cfg(feature = "bitdepth_16")]
                 Grain::Bpc16(grain) => {
-                    // SAFETY: TODO make safe
-                    unsafe {
-                        rav1d_prep_grain::<BitDepth16>(
-                            dsp,
-                            &mut delayed_fg.out,
-                            &delayed_fg.in_0,
-                            grain,
-                        );
-                    }
+                    rav1d_prep_grain::<BitDepth16>(
+                        dsp,
+                        &mut delayed_fg.out,
+                        &delayed_fg.in_0,
+                        grain,
+                    );
                 }
             }
             delayed_fg.type_0 = TaskType::FgApply;