Support `DisplayP3` and `BT.2020 + PQ` #96

hannes-vernooij · 2024-12-23T12:09:22Z

⚠️ Depends on #94
This PR adds support for storing Display P3 and Rec.2020 + PQ aside from sRGB. Functionality to specify which color space to use has also been added to cavif-rs. Be mindful that exported HDR images are not well supported on the web.

Isolated changes: a8e10d4...e01354c

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.

src/main.rs

ravif/src/av1encoder.rs

kornelski · 2024-12-26T18:33:37Z

ravif/src/av1encoder.rs

    Ten,
-    /// Pick 8 or 10 depending on image format and decoder compatibility
-    Auto,


Please don't remove the Auto option. I plan to keep it around for at least a year longer.

kornelski · 2024-12-26T18:35:04Z

ravif/src/av1encoder.rs

@@ -84,7 +191,8 @@ pub struct Encoder {
 impl Encoder {
    /// Start here
    #[must_use]
-    pub fn new() -> Self {
+    // Assumptions about color spaces shouldn't be made since it can't reliably be deduced automatically.
+    pub fn new(color_space: ColorSpace) -> Self {


Please keep the public API of ravif backwards-compatible. This crate's biggest user is the image crate, and I'd like to stay compatible with it. Users of the image crate are slow to upgrade it, so if I made a semver-major release of ravif, it would take quite a while before majority of its users got it. I prefer to be able to release improvements to all existing users quickly.

This unfortunately means that the old use of ColorSpace needs to stay for now. You could rename this one to InputColorSpace, or maybe ColorGamut?.

If it makes anything simpler, I suggest not supporting P3/2020 in 8-bit. There isn't enough precision in 8 bits for this to make sense, and I plan to remove 8-bit support eventually. You could just add dedicated non-generic 10-bit encoding functions that support wide gamut, and leave 8-bit code to rot.

kornelski · 2024-12-26T18:35:26Z

ravif/src/av1encoder.rs

@@ -110,11 +219,12 @@ impl Encoder {
    #[doc(hidden)]
    #[deprecated(note = "Renamed to with_bit_depth")]
    pub fn with_depth(self, depth: Option<u8>) -> Self {
-        self.with_bit_depth(depth.map(|d| if d >= 10 { BitDepth::Ten } else { BitDepth::Eight }).unwrap_or(BitDepth::Auto))
+        self.with_bit_depth(depth.map(|d| if d >= 10 { BitDepth::Ten } else { BitDepth::Eight }).unwrap_or(BitDepth::Ten))


Auto is important for me.

kornelski · 2024-12-26T18:37:24Z

ravif/src/av1encoder.rs

-        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));


This looks buggy. You've removed comment that this function is for 8-bit inputs only, but check against 255 looks like a bug for 10-bit depths. Please write a unit test to prove that this is not buggy.

kornelski · 2024-12-26T20:25:02Z

ravif/src/av1encoder.rs

@@ -284,83 +370,45 @@ impl Encoder {
    ///
    /// returns AVIF file, size of color metadata
    #[inline]
-    pub fn encode_rgb(&self, buffer: Img<&[RGB8]>) -> Result<EncodedImage, Error> {
+    pub fn encode_rgb<P: Pixel + Default>(&self, buffer: Img<&[Rgb<P>]>) -> Result<EncodedImage, Error> {


I don't want to expose the messy rav1e::Pixel in the public API.

Please keep encode_rgb unchanged for API backwards-compatibility, and add a separate method like encode_rgb_10bit that takes Rgb<u16>.

kornelski · 2024-12-26T21:17:19Z

ravif/src/dirtyalpha.rs

    let mut out = Vec::with_capacity(img.width() * img.height());
    loop9::loop9_img(img, |_, _, top, mid, bot| {
-        out.push(if mid.curr.a == 255 {
+        out.push(if mid.curr.a == P::cast_from(255) {


This is a bug for 10-bit depths

kornelski · 2024-12-26T21:18:22Z

ravif/src/dirtyalpha.rs

-            let mut avg = sum.map(|c| (c / 9) as u8);
-            if mid.curr.a == 0 {
-                avg.with_alpha(0)
+            let sum: Rgb<P> = chain(&top, &mid, &bot).map(|px| px.rgb()).sum();


This is a bug for 8-bit depths. It's a sum of 9 pixels, so it will overflow u8.

kornelski · 2024-12-26T21:19:05Z

ravif/src/dirtyalpha.rs

+    T: Pixel + Default,
+{
+    let alpha = Into::<u32>::into(alpha);
+    let rounded = Into::<u32>::into(px) * alpha / 255 * 255;


This is a bug for 10-bit depths

kornelski · 2024-12-26T21:19:19Z

ravif/src/dirtyalpha.rs

-    let low = ((rounded + 16) / alpha) as u8;
-    let hi = ((rounded + 239) / alpha) as u8;
+    let low = (rounded + 16) / alpha;
+    let hi = (rounded + 239) / alpha;


This is a bug for 10-bit depths too

kornelski · 2024-12-26T21:52:40Z

src/main.rs

+        .arg(Arg::new("color-space")
+            .short('C')
+            .long("color-space")
+            .value_name("srgb/displayp3/rec2020pq")


This will not work for the cavif tool. This command-line flag should not exist.

Input images for this program are not read with naive byte-by-byte copying of pixel values. All input images are color-managed and explicitly converted to sRGB, including destroying gamut of images in Rec.2020 and DisplayP3.

If you then take the always-sRGB color data and label it as P3 in AVIF metadata, then decoders will be fooled into interpreting values as in P3, and incorrectly stretch them over a wide gamut, giving a falsely boosted saturation with shifted primaries. Or when reading such AVIF for an sRGB screen, the P3 to sRGB conversion will crush the values and make them clearly different from the actual sRGB input.

To make such hacky relabelling work, users would need to generate P3 files, and then incorrectly relabel pixels as being in sRGB, to create an encoding error in the opposite way of the encoding error caused by this option.
If this was text, it would be like requiring users to type â€™ mojibake to get ’ printed in the output.

That's not a good practice, and that's not a good application interface.

For the ravif crate, it's fine to take user-supplied label, since it takes raw pixel data, so it's up to the caller to generate correct colors or decode input images properly themselves.

But cavif does reading of images itself, so support for wide-gamut inputs unfortunately requires changing how image loading works.

hannes-vernooij added 4 commits December 15, 2024 17:22

Default to 10 bit encoding

56ad947

Support different bit depths in convert_alpha

a8e10d4

Add support for Display P3, and Rec.2020 + PQ

c4e20dc

MarijnS95 reviewed Dec 23, 2024

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

MarijnS95 reviewed Dec 23, 2024

View reviewed changes

ravif/src/av1encoder.rs Show resolved Hide resolved

hannes-vernooij force-pushed the wide-gamut-support branch from f933360 to b5a9219 Compare December 24, 2024 11:26

hannes-vernooij marked this pull request as ready for review December 24, 2024 11:27

code quality improvements

e01354c

hannes-vernooij force-pushed the wide-gamut-support branch from b5a9219 to e01354c Compare December 24, 2024 11:34

kornelski requested changes Dec 26, 2024

View reviewed changes

MarijnS95 mentioned this pull request Dec 27, 2024

HDR support #10

Open

kornelski mentioned this pull request Dec 27, 2024

Support encoding 10-bit input data #94

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `DisplayP3` and `BT.2020 + PQ` #96

Support `DisplayP3` and `BT.2020 + PQ` #96

hannes-vernooij commented Dec 23, 2024 •

edited

Loading

kornelski Dec 26, 2024

kornelski Dec 26, 2024

kornelski Dec 26, 2024

kornelski Dec 26, 2024

kornelski Dec 26, 2024

kornelski Dec 26, 2024

kornelski Dec 26, 2024

kornelski Dec 26, 2024

kornelski Dec 26, 2024

kornelski Dec 26, 2024

Support DisplayP3 and BT.2020 + PQ #96

Are you sure you want to change the base?

Support DisplayP3 and BT.2020 + PQ #96

Conversation

hannes-vernooij commented Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Support `DisplayP3` and `BT.2020 + PQ` #96

Support `DisplayP3` and `BT.2020 + PQ` #96

hannes-vernooij commented Dec 23, 2024 •

edited

Loading