Support DisplayP3 and BT.2020 + PQ
#96
Draft
+289
−222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
MarijnS95
reviewed
Dec 23, 2024
| .value_name("sRGB/displayP3/rec2020Pq") | ||
| .value_parser(parse_color_space) | ||
| .required(true) | ||
| .help("The color space passed values are in [possible values: sRGB, displayP3, rec2020Pq] ")) |
There was a problem hiding this comment.
Suggested change
| .help("The color space passed values are in [possible values: sRGB, displayP3, rec2020Pq] ")) | |
| .help("The color space passed values are in [possible values: sRGB, displayP3, rec2020Pq]")) |
There was a problem hiding this comment.
Note that your parse_color_space() ignores casing, perhaps this should be written without any, too.
(Though personally I favour sticking to case-sensitive cmdline arguments...)
| match s.to_lowercase().as_str() { | ||
| "srgb" => Ok(ColorSpace::Srgb), | ||
| "displayp3" => Ok(ColorSpace::DisplayP3), | ||
| "rec2020Pq" => Ok(ColorSpace::Rec2020Pq), |
There was a problem hiding this comment.
Otherwise to_lowercase() can't ever match:
Suggested change
| "rec2020Pq" => Ok(ColorSpace::Rec2020Pq), | |
| "rec2020pq" => Ok(ColorSpace::Rec2020Pq), |
MarijnS95
reviewed
Dec 23, 2024
Comment on lines
+83
to
+85
| // Since there's no matrix for Display P3 implemented, the one of BT709 is used. | ||
| // Although this isn't perfectly accurate, it should be acceptable as long as the decoder uses the same approach. | ||
| ColorSpace::DisplayP3 => MatrixCoefficients::BT709, |
There was a problem hiding this comment.
Perhaps we should use the BT709 matrix coefficients for DisplayP3 inside rgb_to_ycbcr() in this case?
(at which point you can maybe create a fn color_matrix(self) -> [3; f32] on MatrixCoefficients?)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds support for storing
Display P3andRec.2020 + PQaside from sRGB. Functionality to specify which color space to use has also been added tocavif-rs. Be mindful that exported HDR images are not well supported on the web.Isolated changes: c4e20dc