Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

struct Rav1dFrameContext_lf::cdef_line: Convert pointers to offsets #777

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

randomPoison
Copy link
Collaborator

The pointers in cdef_line were pointing into the buffer owned by cdef_line_buf. In converting them to offsets, I had to make sure that that offsets were consistently in terms of pixels (rather than bytes). The original code was inconsistent here, with the logic working with u8 pointers (and so was using byte offsets), but the code that uses cdef_line treats the pointers as BD::Pixel pointers, so the offset logic is calculated in terms of pixels. In converting to offsets I normalized on pixel offsets. To do so I had to add a couple of helpers:

  • BPC::pxstride - Does the same thing as BitDepth::pxstride, but works in non generic code. This was needed to calculate pixel offsets during the initialization logic.
  • BitDepth::cast_pixel_slice{_mut} - Converts a &[u8] to a &[Pixel]. This was needed so that we could index into the buffer using pixel indices within our bidepth-dependent code.

In practice most of the usages of cdef_line is to calculate pointers to be passed into asm functions, so the cleaned up code is still doing some pointer arithmetic, though we never dereference those pointers from Rust code.

@randomPoison randomPoison force-pushed the legare/rav1dframecontext_lf/cdef_line branch from 1c915d7 to b152d3c Compare March 1, 2024 00:00
Copy link
Collaborator

@kkysen kkysen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had some comments about the pixel casting checks, but the rest of the offset stuff all looks good.

Comment on lines 210 to 217
assert!(bytes.len() % len == 0);
assert!(bytes.as_ptr() as usize % mem::align_of::<Self::Pixel>() == 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to have to check this everytime we access the pixels, so it'd be better to check it once during creation. I'd been planning to create a type for this. This is probably fine in the meantime, though, just worst perf.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What were you thinking wrt making a custom type to handle it? Something like a PixelBuf type that wraps a Vec<u8> and has helpers for accessing the buffer as either a &[u8] or a &[Pixel]?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What were you thinking wrt making a custom type to handle it? Something like a PixelBuf type that wraps a Vec<u8> and has helpers for accessing the buffer as either a &[u8] or a &[Pixel]?

Yeah, basically. It's a bit trickier for Rav1dPicture::data specifically since it supports custom allocators, but that'd be the gist. The alignment/length stuff would all be checked during construction, and so these safe transmute casts would be no-ops other than dividing the length.

include/common/bitdepth.rs Outdated Show resolved Hide resolved
include/common/bitdepth.rs Outdated Show resolved Hide resolved
src/decode.rs Outdated Show resolved Hide resolved
@kkysen kkysen changed the title Rav1dFrameContext_lf::cdef_line: Convert pointers to offsets struct Rav1dFrameContext_lf::cdef_line: Convert pointers to offsets Mar 1, 2024
@randomPoison randomPoison force-pushed the legare/rav1dframecontext_lf/cdef_line_buf branch from dae2cc1 to 3fba038 Compare March 1, 2024 18:27
@randomPoison randomPoison force-pushed the legare/rav1dframecontext_lf/cdef_line branch 2 times, most recently from 8a433b4 to 73689be Compare March 1, 2024 18:58
@randomPoison randomPoison requested a review from kkysen March 1, 2024 22:56
include/common/bitdepth.rs Outdated Show resolved Hide resolved
@randomPoison randomPoison force-pushed the legare/rav1dframecontext_lf/cdef_line_buf branch 2 times, most recently from ca91098 to b344fd2 Compare March 4, 2024 17:43
@randomPoison randomPoison force-pushed the legare/rav1dframecontext_lf/cdef_line branch from 73689be to 1d83d68 Compare March 4, 2024 18:03
@randomPoison randomPoison requested a review from kkysen March 4, 2024 18:03
Base automatically changed from legare/rav1dframecontext_lf/cdef_line_buf to main March 4, 2024 18:10
kkysen added a commit that referenced this pull request Mar 4, 2024
…be used non-generically (#780)

@randomPoison, follow-up to
#777 (comment). I
wanted to share the implementation between `BPC::pxstride` and
`BitDepth::pxstride`.
@randomPoison randomPoison force-pushed the legare/rav1dframecontext_lf/cdef_line branch from 1d83d68 to 89acd73 Compare March 4, 2024 23:08
Copy link
Collaborator

@kkysen kkysen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you merged the changes from #780 wrong; they're all overwritten here with the old versions.

@randomPoison randomPoison force-pushed the legare/rav1dframecontext_lf/cdef_line branch from 89acd73 to 285a342 Compare March 5, 2024 19:04
@randomPoison
Copy link
Collaborator Author

Oh weird, idk how I did that. Should be fixed now.

@randomPoison randomPoison requested a review from kkysen March 5, 2024 19:05
@randomPoison randomPoison merged commit 2f42efb into main Mar 5, 2024
34 checks passed
@randomPoison randomPoison deleted the legare/rav1dframecontext_lf/cdef_line branch March 5, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants