`mod lf_apply`: clean up code and use slices instead of raw pointers #728

fbossen · 2024-02-05T22:39:49Z

Fixes Cleanup c2rust output in lf_apply.rs #702

kkysen

By the way, feel free to split this PR up more if it's taking a while, and it's easier to review it in smaller chunks as well.

kkysen

This mostly looks good. I had some small comments, as well as a bigger one concerning the "unaligned" reads of lvl, which is UB the way we're currently doing it.

src/lf_apply.rs

src/recon.rs

…vl: &[[u8; 4]]`, as previous accesses were UB (#731) See: * #728 (comment) * #728 (comment) The current "unaligned" access of `lvl`, added in #726, and a proposed change in #728, [are UB according to `miri`](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=80849f822a890cabd324b3b898b291b8), as they cast `&[u8; 3]` and `&u8` to `&[u8; 4]`, which is an out of bounds read of a borrow. This `fn unaligned_lvl_slice` helper `fn` fixes it and passes `miri` by transmuting (through `.align_to`) to a `&[u8]`, doing the slice, and then doing a checked cast back to `[u8; 4]`. It also does correct bounds checking, as the previous ways were off by one in their bounds checking, as they didn't consider the read of the unaligned part into the next `[u8; 4]`. Doing it this way also optimizes just as well as the previous methods. We could also use a dependency like `zerocopy` to encapsulate the `unsafe` `.align_to`, but as long as these kinds of "unaligned" array reads are a one-off, doing it inline here seems better. Update: Switched to using unstable `fn`s from `std` that make things fully safe and correct, copied into our codebase for stability. I already figured out how to fix the UB in reviewing #728, so I just went ahead and made the fix independently.

fbossen · 2024-02-11T21:54:48Z

By the way, feel free to split this PR up more if it's taking a while, and it's easier to review it in smaller chunks as well.

The first few commits are now in #739

rinon · 2024-02-22T01:39:15Z

Is this good to go in now? Seems it needs a rebase but anything else beyond that?

kkysen · 2024-02-22T01:53:47Z

Is this good to go in now? Seems it needs a rebase but anything else beyond that?

I need to re-review it.

kkysen · 2024-02-26T11:40:10Z

Addresses #702

Does this fix and close #702, or just address part of it? If it does, can you use one of the special linking keywords like "fix", "close", or "resolve"?

kkysen

Just a few little things left. Also, for the previous unresolved comments, if those were .add()s where the resulting pointer is negatively offset later, you can dismiss my comments. I wasn't sure if that was the case for all of them, though it may.

src/lf_apply.rs

fbossen · 2024-03-01T01:46:51Z

I ran into some issues with borrowing f.lf.mask when rebasing on top of #768 which borrows f.lf. I removed the call to tx_lpf_right_edge() in 1727e4e @randomPoison can you have a look?

kkysen · 2024-03-01T02:07:42Z

I ran into some issues with borrowing f.lf.mask when rebasing on top of #768 which borrows f.lf. I removed the call to tx_lpf_right_edge() in 1727e4e @randomPoison can you have a look?

That looks right. That said, we could put that Vec in its own new type struct and put the method on that. Then the borrowing should work. We should probably do the same for other similar Vecs and their offsets if they have splitting methods.

src/lf_apply.rs

src/internal.rs

src/lf_apply.rs

…pointer

Use `for` instead of `while`

Define masks as `u16` and simplify code to look more like original C code.

Parameter `p` is changed to be an array of slices. A parameter `p_offset` is added to indicate the location of the "origin" sample within the slice. Note that the offset is the same for both chroma components. Hence `p_offset` has one fewer entries than `p`.

Offsets `dst_offset` and `uv_offset` are added to point to "origin" position within slice

Use slices for parameter of `rav1d_loopfilter_sbrow_rows` and `rav1d_copy_lpf`

Dimensions of slice initially created had to be adjusted to account for memory allocation constraints (e.g., sizes of multiples of 128). Not doing so leeds to invalid access when picture dimensiosn are odd numbers.

Also rename variables, dropping `data` prefix.

`dst_offset` is also added to enable accessing elements in the slice that precede the reference position that was pointed to by `dst`

Use slice for pixel data. Add an offset parameter to indicate reference index within slice.

fbossen · 2024-03-06T04:37:22Z

LGTM! Unfortunately there are now merge conflicts from #779. That change was converting pointers to offsets, so hopefully merging them is smooth.

Had to resolve some conflicts but nothing tedious

fbossen mentioned this pull request Feb 5, 2024

src/lf_apply.rs: clean up usage of masks #727

Closed

kkysen reviewed Feb 6, 2024

View reviewed changes

kkysen requested changes Feb 6, 2024

View reviewed changes

kkysen mentioned this pull request Feb 6, 2024

fn unaligned_lvl_slice: Safely encapsulate "unaligned" slices of lvl: &[[u8; 4]], as previous accesses were UB #731

Merged

folkertdev mentioned this pull request Feb 7, 2024

src/cdef_apply.rs: cleanup #733

Merged

fbossen force-pushed the cleanup-lf-apply branch from 10d7d46 to 2f784f5 Compare February 13, 2024 15:23

fbossen changed the title ~~WIP: lf_apply.rs: clean up code and use slices instead of raw pointers~~ lf_apply.rs: clean up code and use slices instead of raw pointers Feb 13, 2024

kkysen self-requested a review February 22, 2024 01:54

fbossen force-pushed the cleanup-lf-apply branch from 2f784f5 to ba4f768 Compare February 23, 2024 12:42

kkysen requested changes Feb 26, 2024

View reviewed changes

src/lf_apply.rs Show resolved Hide resolved

src/lf_apply.rs Outdated Show resolved Hide resolved

src/lf_apply.rs Outdated Show resolved Hide resolved

src/lf_apply.rs Outdated Show resolved Hide resolved

kkysen changed the title ~~lf_apply.rs: clean up code and use slices instead of raw pointers~~ mod lf_apply: clean up code and use slices instead of raw pointers Feb 26, 2024

kkysen mentioned this pull request Feb 26, 2024

mod lr_apply: clean up code #746

Merged

kkysen self-requested a review February 26, 2024 21:49

fbossen force-pushed the cleanup-lf-apply branch 3 times, most recently from f5c44bb to 864fb5a Compare March 1, 2024 01:37

kkysen requested changes Mar 4, 2024

View reviewed changes

src/lf_apply.rs Outdated Show resolved Hide resolved

src/internal.rs Outdated Show resolved Hide resolved

kkysen mentioned this pull request Mar 4, 2024

struct TxLpfRightEdge: New-type wrap the Vec in Rav1dFrameContext_lf::tx_lpf_right_edge #782

Merged

kkysen reviewed Mar 4, 2024

View reviewed changes

src/lf_apply.rs Outdated Show resolved Hide resolved

kkysen reviewed Mar 4, 2024

View reviewed changes

src/lf_apply.rs Outdated Show resolved Hide resolved

kkysen reviewed Mar 4, 2024

View reviewed changes

src/lf_apply.rs Outdated Show resolved Hide resolved

kkysen reviewed Mar 4, 2024

View reviewed changes

src/lf_apply.rs Outdated Show resolved Hide resolved

fbossen force-pushed the cleanup-lf-apply branch from 864fb5a to 5eccc3a Compare March 4, 2024 14:22

Frank Bossen added 26 commits March 5, 2024 23:28

fn rav1d_copy_lpf: use slice instead of pointer for src_stride

64c507e

fn rav1d_copy_lpf: use slice instead of pointer for lr_stride

5c37e6c

fn rav1d_copy_lpf: make src parameter a slice reference

d769725

fn rav1d_loopfilter_sbrow_cols: use slice reference instead of raw …

602b043

…pointer

fn rav1d_loopfilter_sbrow_cols: clean up loops

87342bd

Use `for` instead of `while`

fn rav1d_loopfilter_sbrow_cols: avoid repeated casts to usize

7935e6e

fn rav1d_loopfilter_sbrow_cols: clean up masks

6e15020

Define masks as `u16` and simplify code to look more like original C code.

Use slice instead of raw pointer

3063ae1

fn rav1d_loopfilter_sbrow_cols: make lpf_y and lpf_uv slices

61f83f1

Clean up types and casts

f8daee4

Use references to Rav1dDSPContext object

689fa49

fn filter_plane_cols_y/uv: make dst/u/v arguments a slice

7666d9e

Offsets `dst_offset` and `uv_offset` are added to point to "origin" position within slice

lf_apply.rs: use slices instead of raw pointers

78ee731

Use slices for parameter of `rav1d_loopfilter_sbrow_rows` and `rav1d_copy_lpf`

fn backup_lpf; use slice as parameter

f0fd40b

Dimensions of slice initially created had to be adjusted to account for memory allocation constraints (e.g., sizes of multiples of 128). Not doing so leeds to invalid access when picture dimensiosn are odd numbers.

Define inner blocks for computation of p and p_offset

68f0817

Also rename variables, dropping `data` prefix.

lflvl: use slice instead of raw pointer

789c78f

fn backup_lpf: use slice instead of raw pointer for dst

ad49d8d

`dst_offset` is also added to enable accessing elements in the slice that precede the reference position that was pointed to by `dst`

fn filter_plane_rows_y/uv: use slice instead of raw pointer

e4d2966

Use slice for pixel data. Add an offset parameter to indicate reference index within slice.

Address comments from review

f54cb43

Copy comments from C code

060574d

Avoid unnecessary type casting when invoking BD::pxstride

793a00f

Reduce number of casts for variable top

47fb8fa

fn backup_lpf: clean up pixel copy

d639377

Remove explicit dereference of dsp

c509c30

Clean up use of split_at_mut and pixel copy

5a24dbd

fbossen force-pushed the cleanup-lf-apply branch from 68b9f35 to 5a24dbd Compare March 6, 2024 04:35

fbossen merged commit f6d4770 into memorysafety:main Mar 7, 2024

rinon mentioned this pull request Apr 18, 2024

Create safe slices from cdef_line_buf #981

Closed

mod lf_apply: clean up code and use slices instead of raw pointers #728

mod lf_apply: clean up code and use slices instead of raw pointers #728

Uh oh!

Conversation

fbossen commented Feb 5, 2024 • edited by kkysen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kkysen left a comment

Choose a reason for hiding this comment

Uh oh!

kkysen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fbossen commented Feb 11, 2024

Uh oh!

rinon commented Feb 22, 2024

Uh oh!

kkysen commented Feb 22, 2024

Uh oh!

kkysen commented Feb 26, 2024

Uh oh!

kkysen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fbossen commented Mar 1, 2024

Uh oh!

kkysen commented Mar 1, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fbossen commented Mar 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

`mod lf_apply`: clean up code and use slices instead of raw pointers #728

`mod lf_apply`: clean up code and use slices instead of raw pointers #728

fbossen commented Feb 5, 2024 •

edited by kkysen

Loading