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>
src/rust_api.rs
Outdated
| if let Some(mut pending_data) = self.pending_data.take() { | ||
| let _ = mem::take(&mut pending_data); | ||
| } |
There was a problem hiding this comment.
So the self.pending_data.take() here replaces self.pending_data = None and then moves the Rav1dData to the local variable pending_data. That local variable is dropped no matter what, so the mem::take(&mut pending_data) should be redundant.
src/rust_api.rs
Outdated
| if stride == 0 || data.is_null() { | ||
| return &[]; | ||
| } | ||
| // SAFETY: Copied checks from david-rs added in this pull request https://github.com/rust-av/dav1d-rs/pull/121 |
There was a problem hiding this comment.
I think this needs its own SAFETY comment. There's none in rust-av/dav1d-rs#121.
There was a problem hiding this comment.
To be honest I don't know what a valid SAFETY comment looks like, I've never really used languages like C or unsafe rust so I'm unsure how you'd guarantee there's no undefined behaviour when you're using raw pointers, if yourself or anyone else has any suggestions here I'd be happy to help review
kkysen
left a comment
There was a problem hiding this comment.
Sorry again for such a long wait. I left some comments, and could you also rebase to remove the merge commits? It's also important that we keep each commit buildable/testable so that we can do benchmarking on each commit, so if any commits aren't, could you rebase them such that they are? Thanks!!
|
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. |
|
@Shnatsel How about "Squash and merge" instead of "Create a merge commit" or "Rebase and merge"? It will automatically do it. |
Pulled from #1439. Co-authored-by: Leo Ring <leoring03@gmail.com>
Pulled from #1439. Co-authored-by: Leo Ring <leoring03@gmail.com>
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. |
Fixes `clippy` warning found in CI in #1439 (comment).
Co-authored-by: Khyber Sen <kkysen@gmail.com>
|
@kkysen Thanks for the review, resolved most of the comments, and happy to see all the outside interest in the PR! |
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.