-
Notifications
You must be signed in to change notification settings - Fork 31
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
Support encoding 10-bit input data #94
Support encoding 10-bit input data #94
Conversation
fe0fa6e
to
493bdfb
Compare
cc6ad03
to
4e0430f
Compare
98ed59c
to
8d28a1d
Compare
b57720b
to
95385e8
Compare
In the current implementation, 10-bit signals can be stored, but they are derived from 8-bit signals, offering no visual improvement. All incoming data has been assumed to be 8-bit sRGB. This PR takes the first step in removing these assumptions by enabling the storage of signals with higher bit depth. It lays the groundwork for full support of writing HDR images.
e400ab9
to
d01cf9d
Compare
let buffer = new_alpha.as_ref().map(|b| b.as_ref()).unwrap_or(in_buffer); | ||
let use_alpha = buffer.pixels().any(|px| px.a != 255); | ||
let buffer = new_alpha.as_ref().map(|b: &Img<Vec<Rgba<P>>>| b.as_ref()).unwrap_or(in_buffer); | ||
let use_alpha = buffer.pixels().any(|px| px.a != P::cast_from(255)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cast_from
doesn't seem to be applying any scaling. For 10-bit depth, the opaque alpha is 1023, so I don't see how this would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change only changes the type to rav1e::Pixel
since that is what is eventually expected anyways. I wanted to keep the alpha changes for a separate PR to keep this reviewable, this doesn't change the functionality. Hardcoding this based on the format didn't seem like a great solution to me since 12 bit values are also valid.
fn weighed_pixel(px: RGBA8) -> (u16, RGB<u32>) { | ||
if px.a == 0 { | ||
return (0, RGB::new(0, 0, 0)); | ||
fn weighed_pixel<P: Pixel + Default>(px: Rgba<P>) -> (P, Rgb<P>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weight can't be the same type as the pixel, since it's the pixel value multiplied by weight, so it needs to be wider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, this was from the issue I opened. I didn't know this was intended since this was not documented. I will add some comments and modify the types here.
There are no tests for this change, and I'm not convinced that it works. Such code is prone to integer overflows.
|
d01cf9d
to
56ad947
Compare
I'm not happy with the expanded use of I'm only merging this to avoid throwing away your work on 10-bit inputs, but please stop using |
I had to revert it. Unit tests were broken. I've added unit tests to GitHub actions to flag that sooner. |
@kornelski thanks for your feedback! While working on this we also found that the Note that we would have been able to address your concerns and preempt a revert if this PR wasn't merged instantly after receiving some solid review. I don't think it would have been "thrown away" if it was left open a bit longer so that all this feedback could have been processed 😅. If anything a new PR has to be opened with reworked changes, while "loosing" oversight on all the context written down here. |
The other PR has these commits, I've left comments there: |
Yes I've seen that, but it unfortunately intermixes (review for) two individual features. It should be easier to process them piecemeal instead of at once. |
In the current implementation, 8 or 10-bit signals can be stored, but they are always derived from 8-bit signals, offering no visual improvement. All incoming data has been assumed to be 8-bit sRGB. This PR takes the first step in removing these assumptions by enabling the input of signals with a depth of 10-bits. It lays the groundwork for full support of writing HDR images.