Conversation
kkysen
left a comment
There was a problem hiding this comment.
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.
@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 <kkysen@gmail.com>
Co-authored-by: Khyber Sen <kkysen@gmail.com>
Co-authored-by: Khyber Sen <kkysen@gmail.com>
Co-authored-by: Khyber Sen <kkysen@gmail.com>
Co-authored-by: Khyber Sen <kkysen@gmail.com>
Co-authored-by: Khyber Sen <kkysen@gmail.com>
Co-authored-by: Khyber Sen <kkysen@gmail.com>
Co-authored-by: Khyber Sen <kkysen@gmail.com>
Co-authored-by: Khyber Sen <kkysen@gmail.com>
I'm not sure what this means. Why does that make it not useful?
That seems to be passing a |
|
Yes, due to the limitations of the dav1d API. The I'm not terribly concerned about avoiding this copy in the initial version of the API. AV1/AVIF has a very high compression ratio so if we're copying the data in memory the amount is still very small and will only add a couple percent of overhead to the entire decoding, if that. I'm perfectly happy to get rid of the C dependency first and optimize later. |
When used as a superficial conversion, this is very true. But
We can fix the
Okay, I think we'll go with option 1 here then. No |
kkysen
left a comment
There was a problem hiding this comment.
@Shnatsel, if you change the .to_vec() (https://github.com/image-rs/image/blob/5d0418d0eff747c829d52738c092581312f775c9/src/codecs/avif/decoder.rs#L86) to Box::<[u8]>::from(...) and change the dav1d-rs API to just accept Box<[u8]> instead of the impl AsRef<[u8]>, we should be able to avoid an extra copy or two here. We'll still need the one, the Box::<[u8]>::from(...), but we won't be able to fix that one until we fix the 'static lifetime, which is much more tricky. #1470 sets things up to work well with Box<[u8]>.
…rap_c` (#1470) This can be used by #1439 in `fn send_data` to safely create a `Rav1dData` without a second extra allocation. We'll still need one copy with the way the APIs are right now, but this should keep things obviously safe and without a second extra allocation. Next, we can work on redesigning the API to allow zero-copy usage.
…API (#1471) This does some API simplification for #1439 while also supporting other `impl AsRef` types. It separates `CRef` from `CBox`. `CBox` is the pure C abstraction over an owned C reference/"Box". `CRef` is an `enum` over `CBox` and other Rust-native `impl AsRef`s. I tried using `dyn AsRef` first, but we make some safety guarantees about reference stability that we can't make about any `impl AsRef`, and that would force an extra `Box`. Moving the `dyn AsRef` one level higher to `Arc<dyn AsRef<T>>` would be super nice, but in order to send the `Arc`s through FFI boundaries, they can't wrap an unsized type, as we can't send unsized pointers over FFI boundaries (the ptr metadata API is still very unstable). Adding support for non `'static` `&T`s would be extremely useful and would allow us to eliminate a data copy, but that is much more tricky, as we'd have to thread the lifetime through a lot of places and probably rearrange some of the `Rav1dContext`/`Rav1dState` API.
Co-authored-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
Co-authored-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
Co-authored-by: Khyber Sen <kkysen@gmail.com>
|
This all looks good so far to me, only note is I had to remove static_assertions::assert_impl_all!(Decoder: Send, Sync);Since CRef isn't Send or Sync, if its trivial to add that back in then we should but I'm not sure what safety assumptions you made on the C backend so I didn't want to go messing with that, otherwise I don't think remove the assertion is a very big deal since it just restricts the API a bit |
|
Think that's most of the outstanding issues wrapped up! |
|
I can't wait for this to be finished! |
| .as_byte_mut_ptr() | ||
| .cast_const(); | ||
|
|
||
| if stride == 0 || raw_plane_data_pointer.is_null() { |
There was a problem hiding this comment.
This should never be null. Let me see if I can fix the API for that.
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.