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 CICP #383

Open
sophie-h opened this issue Feb 15, 2023 · 5 comments
Open

Support CICP #383

sophie-h opened this issue Feb 15, 2023 · 5 comments

Comments

@sophie-h
Copy link
Contributor

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
Copy link
Member

HeroicKatora commented Feb 15, 2023

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link

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

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

No branches or pull requests

3 participants