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

Support encoding 10-bit input data #94

Merged
merged 3 commits into from
Dec 26, 2024

Conversation

hannes-vernooij
Copy link
Contributor

@hannes-vernooij hannes-vernooij commented Dec 12, 2024

⚠️ Depends on #89

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.

@hannes-vernooij hannes-vernooij force-pushed the reduce-code-duplication branch 2 times, most recently from fe0fa6e to 493bdfb Compare December 12, 2024 15:56
@hannes-vernooij hannes-vernooij changed the title Reduce code duplication Support encoding 10-bit input data Dec 12, 2024
@hannes-vernooij hannes-vernooij force-pushed the reduce-code-duplication branch 3 times, most recently from cc6ad03 to 4e0430f Compare December 12, 2024 22:08
@hannes-vernooij hannes-vernooij marked this pull request as draft December 13, 2024 13:51
@hannes-vernooij hannes-vernooij force-pushed the reduce-code-duplication branch 7 times, most recently from 98ed59c to 8d28a1d Compare December 13, 2024 17:17
@hannes-vernooij hannes-vernooij marked this pull request as ready for review December 13, 2024 17:20
@hannes-vernooij hannes-vernooij force-pushed the reduce-code-duplication branch 2 times, most recently from b57720b to 95385e8 Compare December 15, 2024 15:22
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.
@hannes-vernooij hannes-vernooij force-pushed the reduce-code-duplication branch 2 times, most recently from e400ab9 to d01cf9d Compare December 17, 2024 15:29
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));
Copy link
Owner

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.

Copy link
Contributor Author

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>) {
Copy link
Owner

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.

Copy link
Contributor Author

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.

@kornelski
Copy link
Owner

kornelski commented Dec 18, 2024

There are no tests for this change, and I'm not convinced that it works. Such code is prone to integer overflows. cast_from(255) looks suspicious.

rav1e::Pixel is only implemented for u8 and u16, and it can't distinguish between 10, 12 or 14 bits. Despite using a trait that suggests it could support for any "Pixel" type, in reality is can only support two types. This seems like a lot of abstract, complex, error-prone code to only support exactly two depths. I don't think it's important to support more than 10 bits for SDR images, but the fact that it can't be done, suggests that rav1e::Pixel is a wrong abstraction.

@kornelski
Copy link
Owner

I'm not happy with the expanded use of rav1e::Pixel. It's a bad trait. This abstraction only obfuscates things. The code is uglier, more complex, and can't even take advantage of the generics to ensure scaling of values is correct.

I'm only merging this to avoid throwing away your work on 10-bit inputs, but please stop using rav1e::Pixel for anything except bare minimum to feed data to rav1e.

@kornelski kornelski merged commit 50fcf28 into kornelski:main Dec 26, 2024
@kornelski
Copy link
Owner

I had to revert it. Unit tests were broken.

I've added unit tests to GitHub actions to flag that sooner.

@MarijnS95 MarijnS95 mentioned this pull request Dec 27, 2024
@MarijnS95
Copy link
Contributor

@kornelski thanks for your feedback! While working on this we also found that the v_frame::Pixel trait, together with the imgref::Img(Vec) and rgb::RGB(A)8 abstractions are increasingly painful to deal with. We're looking ahead to removing the rav1e::Pixel usage aligned with your request.

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.

@kornelski
Copy link
Owner

The other PR has these commits, I've left comments there:

#96

@MarijnS95
Copy link
Contributor

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.

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

Successfully merging this pull request may close these issues.

3 participants