search

image-rs / image-png

PNG decoding and encoding library in pure Rust
https://docs.rs/png
Apache License 2.0
362 stars 143 forks source link

Support CICP #383

Closed sophie-h closed 2 days ago

sophie-h commented 1 year ago

Would it be possible to add CICP support while this feature is 'only' part of the Working Draft?

https://www.w3.org/TR/png-3/#cICP-chunk

HeroicKatora commented 1 year ago

Given the dominant prior art (or posterior art?) of other formats choosing that triple for color space representations, yes, certainly. Should be simple to add? Similar to icc/gama/chrm it'd be better not to delete any of the chunk decodings but simply offer it as an alternative.

sophie-h commented 1 year ago

I think there is one major conceptual question for me: What to do with color-matrix values other than RGB (0 iirc).

I guess, at least at image-rs level it's required to transform to RGB to fit into the current DynamicImage logic? So it would have to apply the specified matrix transform. However, would we change the CICP field in this case? Theoretically, those questions already apply for AVIF already, but since it's currently limited to 8bit in images-rs anyways, it does not seem to be practically relevant.

On the other hand, GTK will happily apply the color-matrix values on the raw data and might even pass the original texture through if it's the right format for the compositor/monitor. So there might be arguments for having the raw option available. However, I think it is not that's not that performance relevant for still images.

HeroicKatora commented 1 year ago

I wouldn't worry about the transformation into rgb in image. The png crate returns the raw color components and image's DynamicImage is about as wrong as it is currently when ignoring the chunk. More importantly, the Transformation options should get a bit of scrutiny. Is expanding 0xab to 0xabab the right transform for all OETFs? Not fully but then again it's still in the intended interval so maybe good enough.

In the midterm we're also moving image to preserving the raw color values for as long as possible, and only collecting the metadata of intended transforms. Then these can be composed with the output transform and applied by either the compositor, the monitor, or manually. This is more correct and more performant.

HeroicKatora commented 1 year ago

What to do with color-matrix values other than RGB (0 iirc).

And to answer that concretely: I'd return it in an enum and maybe add methods to resolve those enum variants into XYZ primaries. Or not if there's any open questions, it can also be done at a later point.

veluca93 commented 1 year ago

IIRC at the moment the spec says that matrix coefficients should be 0. I believe (unless something changed in the meantime) that Chrome will refuse to interpret anything else. OTOH the spec says that the full-range flag can be 0 too, but Chrome doesn't support that (~and if it were up to me, it never will~), and I don't see why one would ever want to do video-range RGB anyway in 2023...

Implementation of cicp in the JXL PNG en/decoder: https://github.com/libjxl/libjxl/blob/main/lib/extras/enc/apng.cc#L161 https://github.com/libjxl/libjxl/blob/main/lib/extras/dec/apng.cc#L89 In Chrome: https://chromium-review.googlesource.com/c/chromium/src/+/3705739

karip commented 3 days ago

I think that this just got implemented: https://github.com/image-rs/image-png/pull/529