initial implementation #1
Conversation
* use "Infinite Athlete, Inc." everywhere * get rid of unneeded `Cargo.toml` release profile tweaking
src/uyvy_to_i420.rs
Outdated
| /// `uyvy_in` is of the shape `(height, width / 2, 4)` and channels are (U, Y, V, Y). | ||
| /// Guaranteed to initialize and fill the supplied output planes on success. | ||
| /// `y_out` is of the shape `(height, width)`; `u_out` and `v_out` are each of | ||
| /// the shape `(height / 2, width / 2)`. |
There was a problem hiding this comment.
I would suggest adding a # Panics section here to make it clear what happens when the args aren't the correct shape. Or make the function return an error in those cases instead.
There was a problem hiding this comment.
With the new frame abstraction, the only panics are here:
let [uyvy_in] = &uyvy_planes[..] else {
panic!("uyvy_in must have one plane");
};
let mut yuv_planes = yuv_out.planes();
let [y_out, u_out, v_out] = &mut yuv_planes[..] else {
panic!("yuv_out must have three planes");
};
// ...
assert_eq!(uyvy_in.data.len(), pixels * 2);
// ...
assert_eq!(y_out.data.len(), pixels);
assert_eq!(u_out.data.len(), pixels / 4);
assert_eq!(v_out.data.len(), pixels / 4);...which can only happen if you supply a buggy implementation of Frame/FrameBuf that returns a number/size of planes inconsistent with its pixel_format(). To me that's kind of a duh, of course it will panic situation, not worth calling out. Would you agree?
* add `Frame`/`FrameBuf` abstraction to allow caller to avoid preinitialization without any use of `unsafe`. This also offers an interface for padded lines, although the conversion currently returns unsupported if it's used. * return a `ConversionError` rather than the more specific `Unsupported`. The error is currently opaque; this could be changed later without breaking compatibility.
|
For the sake of making my stance clear I'm really close to giving an approval. My outstanding concerns are #1 (comment) and #1 (comment) #1 (comment) is a very minor opinion based thing but I recognize there are drawbacks to my proposal so I'm not firm on it. |
src/frame.rs
Outdated
| } | ||
|
|
||
| pub struct FrameBufPlane<'a> { | ||
| pub data: &'a mut [MaybeUninit<u8>], |
There was a problem hiding this comment.
Since FrameBuf is unsafe anyway, is there any reason not to use pointer / length here? As-is, I still don't think it'd be possible to fill an arbitrary &mut [u8] not backed by a Vec (or something convertible to &mut [MaybeUninit<u8>]), right?
There was a problem hiding this comment.
I like &'a mut [MaybeUninit<u8>] as a nice readable way of bundling the lifetime, pointer, and length. It also offers safe writing and thus can limit the size of the unsafe blocks required / ways a writer is likely to screw up even if they ultimately still need an unsafe { buf.finish() } at the end to assert they're initialized everything.
On the other hand, If filling uninitialized data will be mostly done by the conversion stuff here—that uses raw pointers anyway for vendor SIMD intrinsics—yeah, there's not a huge advantage. We could expose raw pointers instead of MaybeUninit. Yes, that'd let you use a &mut [u8] as a target of conversion which yes I think my current FrameBuf would not allow. There also wouldn't have to be a separate FrameBuf trait; it could just be a boolean tracking if someone has pretty-promised it's initialized yet. You could always get a raw pointer; you could only get a &[u8] or &mut [u8] post-initialization.
I'm on the fence. What do you think?
As-is, I still don't think it'd be possible to fill an arbitrary
&mut [u8]not backed by aVec(or something convertible to&mut [MaybeUninit<u8>]), right?
I said no before...and as I think about it some more, I think probably no? The thread I linked was talking about how a MaybeUninit<T> could fill in an invalid bit pattern for T. There's no invalid bit pattern for u8. It's still forbidden to to make an uninitialized u8 though. I think it comes down to this: MaybeUninit<T> implements Copy, so if you do *this = that; where that is uninitialized, do you "taint" a previously-initialized this with uninitialized status? I guess so?
There was a problem hiding this comment.
There also wouldn't have to be a separate
FrameBuftrait; it could just be a boolean tracking if someone has pretty-promised it's initialized yet.
That said I'd probably still provide two separate structs implementing the one trait. One specifically for a Vec so it can do the set_len call on finish. One for borrowed buffers; you could get an initialized one from a &mut [u8] or an uninitialized one from a &mut [MaybeUninit<u8>].
There was a problem hiding this comment.
I think the risk of the user doing something wrong as a result of needing to do something the library doesn't support might be the bigger risk here. And I would expect the goal to be for us to provide implementations of the trait to cover nearly all use-cases, right? I think that could be done with potentially just two implementations: one that takes a &mut [MaybeUninit<u8>] and one that can take in a DerefMut<Target=[u8]>.
There was a problem hiding this comment.
And I would expect the goal to be for us to provide implementations of the trait to cover nearly all use-cases, right?
I think that there will be Frame impls in other libraries that implement capture/decoding/encoding/sending, to avoid unnecessary copies. But hopefully they're not too crazy to write in either case.
I redid this abstraction with a Frame and additive FrameMut trait. And then the concrete implementation...there's one, but also uses Storage/StorageMut traits with a few implementations. I didn't do it based on Deref/DerefMut because Vec is special, and there's no stable/sound specialization.
| data: *const u8, | ||
| stride: usize, | ||
| len: usize, | ||
| initialized: bool, |
There was a problem hiding this comment.
Should there be a pub way to construct a FramePlaneRef and FramePlaneMut?
There was a problem hiding this comment.
Yes, we'll need those for the outside-the-crate frame impls, and I added them now. I'm not totally sure I have the right signature for them; I could add in width also, as in how many bytes each row uses without padding; width <= stride. This would allow the PartialEq/Eq impls to skip the padding bytes as well as just exposing the width to callers.
src/lib.rs
Outdated
|
|
||
| pub use uyvy_to_i420::convert as convert_uyvy_to_i420; | ||
|
|
||
| pub use arrayvec::ArrayVec; |
There was a problem hiding this comment.
Would it be better to re-export arrayvec? With only arrayvec::ArrayVec, there are certain things users won't be able to do, like reference arrayvec::CapacityError by name.
No description provided.