Implement dav1d-rs's Rust API

[leo030303]

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:

• Have all the public-facing Rust API code in its own file.
• Used some enums from rav1d instead of redefining new ones, and added in doc comments from the original dav1d-rs library to a few items.
• Removed as much unsafe code as I could and replaced it with the Rust methods from rav1d as much as possible.

It currently works as a drop-in replacement for dav1d-rs; adding in use rav1d as dav1d; to my fork of image makes everything work fine.

The only functional changes I made are I removed the unsafe impls of Send and Sync for InnerPicture so Picture is no longer Sync or Send. I looked through the code and I don't believe DisjointMut<Rav1dPictureDataComponentInner>, which is a field of one of its children, is thread safe, though I'm open to correction there; I'm pretty unfamiliar with unsafe Rust.

I also don't have safety comments on the two unsafe blocks in rust_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.

• Closes Provide Rust API #1252.

[kkysen]

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.

[kkysen]

@leo030303, could you rebase this on main? There are a lot of other commits in here now, so it's harder to review.

[kkysen]

This PR has a rather long commit history. Rewriting the history with such granularity at this point is completely impractical. It's probably best to squash it down to one commit before merging if every commit on main being testable is a requirement. Github has this as an option in the merge button dropdown.

Yup, this is fine, too. I pulled out the two small changes that seem more separate (#1468, #1469). The rest seems fine to be squashed into one commit.

[leo030303]

@kkysen Thanks for the review, resolved most of the comments, and happy to see all the outside interest in the PR!

[Shnatsel]

Here's a safety comment and a potential soundness fix.

Are there any other outstanding work items or is this good to go?

[Shnatsel]

Suggested change

	PixelLayout::I400 | PixelLayout::I422 | PixelLayout::I444 => self.height(),
	PixelLayout::I422 | PixelLayout::I444 => self.height(),
	PixelLayout::I400 => return &[]; // grayscale images don't have color components

There are checks further on that might catch this, but better safe than sorry.

I've also opened a corresponding dav1d-rs PR: rust-av/dav1d-rs#123

[Shnatsel]

Suggested change

	// SAFETY: Copied checks from dav1d-rs added in this pull request https://github.com/rust-av/dav1d-rs/pull/121
	// SAFETY: The following invariants are upheld:
	// 1. Pointer validity: Checked above - if null or stride is 0, we return &[].
	// 2. Pointer alignment: The allocator guarantees RAV1D_PICTURE_ALIGNMENT (64-byte)
	// alignment (see Rav1dPictureDataComponentInner::new), which exceeds any
	// primitive type's alignment requirements.
	// 3. Allocated size: The allocator guarantees the buffer is at least stride * height
	// bytes (the allocator callback contract in Dav1dPicAllocator). The checked_mul
	// ensures this calculation doesn't overflow.
	// 4. Initialization: The allocator is required to initialize the data per the
	// alloc_picture_callback safety requirements.
	// 5. Lifetime: The returned slice borrows &self, keeping the Arc<Rav1dPictureData>
	// alive for the duration of the borrow.
	// 6. No mutable aliases: The Picture is only returned after decoding is complete,
	// so the decoder no longer writes to this buffer. The public API only exposes
	// shared (&[u8]) access, and no &mut references to this data can exist once
	// the Picture is handed to the user.
	//
	// Past dav1d-rs PRs relevant to this line:
	// https://github.com/rust-av/dav1d-rs/pull/121
	// https://github.com/rust-av/dav1d-rs/pull/123

[leo030303]

This looks good to me, very thorough, thanks for the help! @kkysen thoughts?

[kkysen]

I think this is good, but I'd like to finish figuring out the send_data side of things so that I can go through this in more detail, considering the full lifetime of the data. It seems good from a less thorough check, though.

[kkysen]

So this is a bit more complex than I thought. There are a few options here:

1. Keep doing what you're doing now, with an extra boxing allocation. There are actually two here, but we can easily reduce that to just one.
2. Copy dav1d-rs's approach and unsafely call Rav1dData::wrap. We'd have to manage the lifetime through the cookie and this is definitely unsafe, but it should be sound. This does have a small overhead still, and dav1d-rs has this overhead, too.
3. Fix the rav1d API to make everything generic over a R: AsRef<T>/R: AsRef<[u8]>, allowing API parity with dav1d-rs with no overhead and a fully safe code path. This generic parameter is quite infectious, which is annoying, but it appears to work quite well without significant changes besides fixing the generic parameters.

So what we want to do here also depends on what we'd like the API to be. If we restrict the API to just Box<[u8]>, it's simple, but allowing AsRef<[u8]>, like Vec<u8> and &[u8], is more complex.

[Shnatsel]

If we restrict the API to just Box<[u8]>, it's simple, but allowing AsRef<[u8]>, like Vec and &[u8], is more complex.

Box<[u8]> doesn't really gel with the APIs e.g. image exposes. It'll work but require an extra copy of each batch of the input plus an allocation for each batch; when using this with an impl Read this can introduce noticeable overhead.

I like approach 2 in the short term and approach 3 in the long term.

---

Speaking of API improvements, I don't think T: AsRef<[u8]> is even a good idea. It just causes more monomophisation and doesn't even help ergonomics that much compared to a &[u8].

But the real trick is getting rid of the 'static requirement without copying or infecting much of the surrounding code with lifetimes.

[leo030303]

The 'static requirement on send_data is removed, the function looks like this now

    pub fn send_data<T: AsRef<[u8]> + Send + Sync>(
        &mut self,
        buf: T,
        offset: Option<i64>,
        timestamp: Option<i64>,
        duration: Option<i64>,
    ) -> Result<(), Rav1dError> {

[kkysen]

Box<[u8]> doesn't really gel with the APIs e.g. image exposes. It'll work but require an extra copy of each batch of the input plus an allocation for each batch; when using this with an impl Read this can introduce noticeable overhead.

What API is image, etc. exposing? Being able to take that into account would help designing the right API here.

I like approach 2 in the short term and approach 3 in the long term.

👍

Speaking of API improvements, I don't think T: AsRef<[u8]> is even a good idea. It just causes more monomophisation

Hmm, you're right about the monomorphization. That would be quite annoying.

doesn't even help ergonomics that much compared to a &[u8].

It does let use other types like [u8; N], Box<[u8]>, Vec<u8>, Arc<[u8]>, etc. I'm not sure how useful any of those are from the caller's perspective, as I don't have much familiarity with that.

But the real trick is getting rid of the 'static requirement without copying or infecting much of the surrounding code with lifetimes.

The tricky part here is the pending_data. If that's moved to another intermediary type wrapping Rav1dContext, we could limit the lifetime to just CBox, CArc, Rav1dData, the new wrapper type, and maybe a few others. I also experimented with adding a R: AsRef<T>/R: AsRef<[u8]> bound on these types (without the pending_data separation), but without a + 'static bound, and it seems to compile fine.

[kkysen]

The 'static requirement on send_data is removed, the function looks like this now

    pub fn send_data<T: AsRef<[u8]> + Send + Sync>(
        &mut self,
        buf: T,
        offset: Option<i64>,
        timestamp: Option<i64>,
        duration: Option<i64>,
    ) -> Result<(), Rav1dError> {

dav1d-rs had the 'static to ensure that the T, which creates the &[u8], outlives the {D,R}av1dContext. You were able to remove it for now because your current implementation copies the data. But if we want a similar zero-copy API, we may have to do the same, although we do have the advantage of being able to try threading the lifetime through safe code and letting borrowck tell us if it works fine, which based on some experiments, I think it does, and IMO isn't overly infectious (it's still somewhat infectious, but not terribly so).

[Shnatsel]

It does let use other types like [u8; N], Box<[u8]>, Vec, Arc<[u8]>, etc. I'm not sure how useful any of those are from the caller's perspective, as I don't have much familiarity with that.

They aren't useful because all of them can be turned into a &[u8] with a single .as_slice() call or sometimes even automatically by the compiler with only a & operator.

What API is image, etc. exposing? Being able to take that into account would help designing the right API here.

This is the trait image decoders are required to implement: https://docs.rs/image/latest/image/trait.ImageDecoder.html

This is how it's currently wired up to dav1d: https://github.com/image-rs/image/blob/5d0418d0eff747c829d52738c092581312f775c9/src/codecs/avif/decoder.rs

[kkysen]

It does let use other types like [u8; N], Box<[u8]>, Vec, Arc<[u8]>, etc. I'm not sure how useful any of those are from the caller's perspective, as I don't have much familiarity with that.

They aren't useful because all of them can be turned into a &[u8] with a single .as_slice() call or sometimes even automatically by the compiler with only a & operator.

I'm not sure what this means. Why does that make it not useful?

This is how it's currently wired up to dav1d: https://github.com/image-rs/image/blob/5d0418d0eff747c829d52738c092581312f775c9/src/codecs/avif/decoder.rs

That seems to be passing a Vec (at least .to_vec(). So wouldn't supporting Vec<u8> help?

[Shnatsel]

Vec<u8>.as_slice() is a cheap conversion from Vec<u8> to &[u8], so generics like AsRef<&[u8]> are only a slight ergonomic improvement and generally don't pull their weight in terms of additional complexity.

That seems to be passing a Vec (at least .to_vec()).

Yes, due to the limitations of the dav1d API. The .to_vec() call makes a copy of the data. image would pass the input directly if it could, but I believe the 'static bound makes it impossible.

---

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.