`struct Rav1dFrameContext_lf::lr_lpf_line`: Convert to offsets by randomPoison · Pull Request #793 · memorysafety/rav1d

randomPoison · 2024-03-11T17:41:51Z

No description provided.

kkysen · 2024-03-11T19:40:33Z

src/lf_apply.rs

-    let dst: [&mut [BD::Pixel]; 3] = [
-        slice::from_raw_parts_mut(
-            (f.lf.lr_lpf_line[0] as *mut BD::Pixel).offset(cmp::min(y_span, 0)),
-            lr_plane_sz[0] as usize,
-        ),
-        slice::from_raw_parts_mut(
-            (f.lf.lr_lpf_line[1] as *mut BD::Pixel).offset(cmp::min(uv_span, 0)),
-            lr_plane_sz[1] as usize / 2,
-        ),
-        slice::from_raw_parts_mut(
-            (f.lf.lr_lpf_line[2] as *mut BD::Pixel).offset(cmp::min(uv_span, 0)),
-            lr_plane_sz[1] as usize / 2,
-        ),
-    ];
-    let dst_offset: [usize; 2] = [
+    let yuv_offset: [usize; 2] = [
        (tt_off as isize * y_stride - cmp::min(y_span, 0)) as usize,
        (tt_off as isize * uv_stride - cmp::min(uv_span, 0)) as usize,
    ];
+    let dst_offset = [
+        f.lf.lr_lpf_line[0].wrapping_add_signed(cmp::min(y_span, 0)) + yuv_offset[0],
+        f.lf.lr_lpf_line[1].wrapping_add_signed(cmp::min(uv_span, 0)) + yuv_offset[1],
+        f.lf.lr_lpf_line[2].wrapping_add_signed(cmp::min(uv_span, 0)) + yuv_offset[1],
+    ];


Why can't we keep the original dst as an array of &mut [_]s? They don't overlap, so we should be able to split up lr_line_buf, right?

Splitting up lr_line_buf into separate slices isn't necessary here. We were passing dst[i] plus an offset into backup_lpf, so the most straightforward thing to do is combine the offsets and pass all of lr_line_buf in along with the calculated offset. I don't see any reason to go through the extra steps of split_at_muting the slice only to still pass in a buffer+offset pair.

randomPoison · 2024-03-12T21:08:34Z

This is blocked on #801. The test failures we're seeing originate from the out-of-bounds pointer access that is already present on main, and moving to slice operations triggered an array OOB panic. As far as I can tell this change didn't introduce the issue, so we'll need to fix #801 first and rebase this on top.

randomPoison · 2024-03-12T23:53:51Z

src/lr_apply.rs

-    let lpf_plane_sz = BD::pxstride(f.lf.lr_buf_plane_sz[(plane != 0) as usize] as isize);
-    let mut lpf_offset = cmp::max(lpf_stride - lpf_plane_sz, 0);


@fbossen I ended up removing lpf_plane_sz and lpf_offset in order to make the calculation here the same as the original C. You had added those in #746 to handle negative strides I think. Do we still need those with the fixed pxstride? Was there a reason why we needed those values in Rust but not in the original C?

The reason I added lpf_plane_sz and lpf_offset is that I defined lpf as a slice that begins at the memory section being effectively used and needed the compute the memory location of the beginning of the slice (and cover the case of a negative stride). If, instead of defining a new slice lpf, you use lr_line_buf, then removing lpf_plane_sz and lpf_offset looks appropriate.

fbossen

Upon further testing, it looks like I am hitting an error on one of the test bitstreams. Let me look into it and report more details later.

fbossen · 2024-03-13T04:05:25Z

src/lf_apply.rs

@@ -161,29 +161,21 @@ pub(crate) unsafe fn rav1d_copy_lpf<BD: BitDepth>(
    let y_span = lr_plane_sz[0] as isize - y_stride;


I think you should be able to remove y_span and uv_span here as well. They were used to compute the base address of the slice. It seems superfluous to subtract y_span in yuv_offset[0] and add it back in dst_offset[0]

fbossen · 2024-03-13T12:11:51Z

Upon further testing, it looks like I am hitting an error on one of the test bitstreams. Let me look into it and report more details later.

The issues boils down to the following: in fn rav1d_decode_frame_init f.lf.lr_line_buf is reallocated if needed. The offsets lr_lpf_line are updated upon reallocation. However, there are cases where f.lf.lr_line_buf doesn't need to be reallocated but the offsets need updating: for example, if the picture width doubles and the pixel size is halved. (e.g., y_stride doesn't change but y_stride_px does).

src/lr_apply.rs

randomPoison · 2024-03-15T16:43:33Z

The issues boils down to the following: in fn rav1d_decode_frame_init f.lf.lr_line_buf is reallocated if needed. The offsets lr_lpf_line are updated upon reallocation. However, there are cases where f.lf.lr_line_buf doesn't need to be reallocated but the offsets need updating

Huh, is that being caused by the changes in this PR? I haven't changed anything related to when we do the re-allocation and update lr_lpf_line, so I would think the behavior there is the same as before. I can remove the if condition around the initialization logic so that we always resize and recompute lr_lpf_line, that would fix the issue I think.

fbossen · 2024-03-15T16:50:33Z

Huh, is that being caused by the changes in this PR? I haven't changed anything related to when we do the re-allocation and update lr_lpf_line, so I would think the behavior there is the same as before.

Yes. Changes in this PR cause the issue. If lr_lpf_line was always in units of u8, then there would be no issue. But lr_lpf_line is now in units of u8 or u16 depending on the bit depth. So if the bit depth changes, but the amount (in bytes) of allocated memory doesn't, the values of lr_lpf_line require updating.

randomPoison · 2024-03-15T16:59:00Z

Okay, thanks for confirming! I've removed the conditional logic for initializing lr_lpf_line so it should be updated properly now if the bitdepth changes.

fbossen

LGTM

randomPoison requested review from kkysen and rinon March 11, 2024 17:41

randomPoison force-pushed the legare/Rav1dFrameContext_lf/lr_lpf_line branch from 879f132 to 3849584 Compare March 11, 2024 17:42

Base automatically changed from legare/Rav1dFrameContext_lf/lr_line_buf to main March 11, 2024 17:43

kkysen reviewed Mar 11, 2024

View reviewed changes

kkysen changed the title ~~Rav1dFrameContext_lf::lr_lpf_line: Convert to offsets~~ struct Rav1dFrameContext_lf::lr_lpf_line: Convert to offsets Mar 11, 2024

randomPoison mentioned this pull request Mar 12, 2024

Out of bounds array access in lr_stripe #801

Closed

randomPoison force-pushed the legare/Rav1dFrameContext_lf/lr_lpf_line branch from 3849584 to 21f7300 Compare March 12, 2024 23:50

randomPoison commented Mar 12, 2024

View reviewed changes

randomPoison requested a review from fbossen March 12, 2024 23:54

fbossen approved these changes Mar 13, 2024

View reviewed changes

fbossen reviewed Mar 13, 2024

View reviewed changes

fbossen self-requested a review March 13, 2024 03:46

fbossen reviewed Mar 13, 2024

View reviewed changes

kkysen mentioned this pull request Mar 13, 2024

struct Rav1dFrameContext_lf::lr_line_buf: Make into AlignedVec64 #792

Merged

kkysen reviewed Mar 13, 2024

View reviewed changes

src/lr_apply.rs Outdated Show resolved Hide resolved

randomPoison force-pushed the legare/Rav1dFrameContext_lf/lr_lpf_line branch from 21f7300 to 68fda0e Compare March 15, 2024 16:48

Rav1dFrameContext_lf::lr_lpf_line: Convert to offsets

b637376

randomPoison force-pushed the legare/Rav1dFrameContext_lf/lr_lpf_line branch from 68fda0e to b637376 Compare March 15, 2024 16:58

randomPoison requested review from fbossen and kkysen March 15, 2024 23:06

fbossen approved these changes Mar 18, 2024

View reviewed changes

randomPoison merged commit 1e394d7 into main Mar 18, 2024

randomPoison deleted the legare/Rav1dFrameContext_lf/lr_lpf_line branch March 18, 2024 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`struct Rav1dFrameContext_lf::lr_lpf_line`: Convert to offsets#793

`struct Rav1dFrameContext_lf::lr_lpf_line`: Convert to offsets#793
randomPoison merged 1 commit intomainfrom
legare/Rav1dFrameContext_lf/lr_lpf_line

randomPoison commented Mar 11, 2024

Uh oh!

kkysen Mar 11, 2024

Uh oh!

randomPoison Mar 11, 2024

Uh oh!

randomPoison commented Mar 12, 2024

Uh oh!

randomPoison Mar 12, 2024

Uh oh!

fbossen Mar 13, 2024

Uh oh!

fbossen left a comment

Uh oh!

fbossen Mar 13, 2024

Uh oh!

fbossen commented Mar 13, 2024

Uh oh!

Uh oh!

randomPoison commented Mar 15, 2024

Uh oh!

fbossen commented Mar 15, 2024

Uh oh!

randomPoison commented Mar 15, 2024

Uh oh!

fbossen left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		let lpf_plane_sz = BD::pxstride(f.lf.lr_buf_plane_sz[(plane != 0) as usize] as isize);
		let mut lpf_offset = cmp::max(lpf_stride - lpf_plane_sz, 0);

		@@ -161,29 +161,21 @@ pub(crate) unsafe fn rav1d_copy_lpf<BD: BitDepth>(
		let y_span = lr_plane_sz[0] as isize - y_stride;

Conversation

randomPoison commented Mar 11, 2024

Uh oh!

kkysen Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

randomPoison Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

randomPoison commented Mar 12, 2024

Uh oh!

randomPoison Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

fbossen Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

fbossen left a comment

Choose a reason for hiding this comment

Uh oh!

fbossen Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

fbossen commented Mar 13, 2024

Uh oh!

Uh oh!

randomPoison commented Mar 15, 2024

Uh oh!

fbossen commented Mar 15, 2024

Uh oh!

randomPoison commented Mar 15, 2024

Uh oh!

fbossen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants