-
Notifications
You must be signed in to change notification settings - Fork 58
Implement dav1d-rs's Rust API
#1439
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
base: main
Are you sure you want to change the base?
Conversation
kkysen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the much safer implementation! I'm still taking a closer look at the unsafes, as the few remaining are still quite critical, but I wanted to give some initial feedback in general first.
Pulled out the docs additions from #1439 into its own PR.
kkysen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leo030303, could you rebase this on main? There are a lot of other commits in here now, so it's harder to review.
Co-authored-by: Khyber Sen <[email protected]>
Co-authored-by: Khyber Sen <[email protected]>
Co-authored-by: Khyber Sen <[email protected]>
Co-authored-by: Khyber Sen <[email protected]>
Co-authored-by: Khyber Sen <[email protected]>
Co-authored-by: Khyber Sen <[email protected]>
Co-authored-by: Khyber Sen <[email protected]>
Co-authored-by: Khyber Sen <[email protected]>
Co-authored-by: Khyber Sen <[email protected]>
|
I think the core of this PR, the rust API, seems to be in a good place, for the work aligning the implementation closer to the spec I think it makes sense to do those under separate PR's, but overall I think the rust API should be good to merge if it all looks fine from your end |
|
I think this was closed accidentally? |
Oh, whoops, I didn't realize "fix" expressions for a PR comment close them. I thought it only worked on issues. |
Signed-off-by: Leo Ring <[email protected]>
|
Is there anything blocking this? I'd love to see this merged and published so we could drop the last remaining C library, |
|
Is there anything stopping this from being merged? Happy to make any requested changes @kkysen |
| unsafe { | ||
| slice::from_raw_parts( | ||
| self.0.plane_data_ptr(self.1) as *const u8, | ||
| (stride * height) as usize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Casting after the multiplication is a bit suspicious. These are u32 where stride in particular came from a ptrdiff_t downcast to a u32 with try_into().unwrap() in Picture::stride. This will overflow for larger images. Of course that's never a memory safety problem, it will just make the returned buffer smaller.
| let height = match component { | ||
| PlanarImageComponent::Y => self.height(), | ||
| _ => match self.pixel_layout() { | ||
| PixelLayout::I420 => (self.height() + 1) / 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| PixelLayout::I420 => (self.height() + 1) / 2, | |
| PixelLayout::I420 => self.height().div_ceil(2), |
Handles the case where height = u32::MAX correctly, too.
| impl AsRef<[u8]> for Plane { | ||
| fn as_ref(&self) -> &[u8] { | ||
| let (stride, height) = self.0.plane_data_geometry(self.1); | ||
| // SAFETY: both stride and height can't be negative, the `stride` and `height` methods panic if they are so there's no undefined behaviour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording here misses the mark and ends in a non-sequitur. It should justify why the extent is correct for the pointer that's used but only justifies how the inputs are somehow bounded in [0; u32::MAX) (repeating the type explicitly would also help readability).
Does Rav1dPictureDataComponent uphold that invariant properly? For instance there's a Rav1dPictureDataComponent::wrap_buf constructor which takes an arbitrary, unverified stride and Rav1dPictureData::data and Rav1dPicture::data are indeed a public attributes so that is non-obvious by itself. The invariant has to be protected by Picture and its only constructor path Decoder::get_picture but I don't seem exactly how, and that comment is certainly way to little to prove it.
I've copy pasted the PR from #1362 and updated it with some of the suggestions made on that pull request. The main changes are:
enums fromrav1dinstead of redefining new ones, and added in doc comments from the originaldav1d-rslibrary to a few items.unsafecode as I could and replaced it with the Rust methods fromrav1das much as possible.It currently works as a drop-in replacement for
dav1d-rs; adding inuse rav1d as dav1d;to my fork of image makes everything work fine.The only functional changes I made are I removed the
unsafe impls ofSendandSyncforInnerPicturesoPictureis no longerSyncorSend. I looked through the code and I don't believeDisjointMut<Rav1dPictureDataComponentInner>, which is a field of one of its children, is thread safe, though I'm open to correction there; I'm pretty unfamiliar withunsafeRust.I also don't have safety comments on the two
unsafeblocks inrust_api.rs; I'm unsure what these would look like, so open to suggestions there. These are mostly taken verbatim from the old pull request.