Skip to content
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

Allow Frame struct initialization with u16 data #13

Closed
rzumer opened this issue Aug 13, 2018 · 6 comments
Closed

Allow Frame struct initialization with u16 data #13

rzumer opened this issue Aug 13, 2018 · 6 comments

Comments

@rzumer
Copy link
Member

rzumer commented Aug 13, 2018

When encoding at high bit depths, unless passing decoded frames right back to the encoder, we currently have to perform u16 to u8 conversions in order to initialize a Frame. The two options to do that that I am aware of are an unsafe from_raw_parts operation, or iterating over each input pixel to generate a new frame. It would be convenient to have a constructor for high bit depth data that could perform one of these options when fed u16 input. Alternatively, a frame could always maintain a u16 buffer and the encoder could write as u8 when outputting in 8-bit.

@Kagami
Copy link
Member

Kagami commented Aug 13, 2018

The simplest way is to add from_u16 method which would create Frame from u16 planes.

A bit better method is to implement #11 and accept either u8 or u16 depending on the colorspace. But it would be impossible to create high bitdepth frame from u8 planes then (unless introducing something like from_u8).

Also I'm not entirely sure y4m should deal with concept of pixels. Because u8 vs u16 is about the pixel values, not the data stream. Any frame can be represented as a stream of u8.

What do you think?

@rzumer
Copy link
Member Author

rzumer commented Aug 13, 2018

Handling pixel values instead of raw data does introduce some additional concerns. We can leave it as is and leave the user with the responsibility of providing the raw data in u8 format. On the other hand, converting from high bit depth u16 data to u8 in safe Rust seems to require a significant amount of boilerplate for a use case that should be fairly common, so there is a clear benefit in handling this conversion as part of y4m.

@Kagami
Copy link
Member

Kagami commented Aug 13, 2018

for a use case that should be fairly common

Seems like it, accepting u16 planes might give better user experience in real world use-cases.

Would you like to implement from_u16 for Frame?

@rzumer
Copy link
Member Author

rzumer commented Aug 13, 2018

Sure, I will give it a try.

The conversion could look like this:

planes.iter().map(|&plane| plane.iter()
        .flat_map(|&plane_value| vec![(plane_value >> 8) as u8, plane_value as u8])
        .collect()).collect();

Which results in a Vec<Vec<u8>>. Afterwards slicing the inner vectors for 3 planes should be enough, but due to the Frame structure containing a reference to frame data rather than owning that data, I have not been able to figure out a clean way to persist this processed data, as the borrow dies as the constructor returns. Changing plane data to a vector type would allow it to own the data, but I'm not sure that this is desirable.

@Kagami
Copy link
Member

Kagami commented Aug 15, 2018

(Haven't seen github notifications for last post editings.)

I think we should unsafely represent u16 as u8 with from_raw_parts.

@rzumer
Copy link
Member Author

rzumer commented Aug 15, 2018

OK, I was trying to avoid unsafe code but that should work, I will try it soon.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants