Add method to expose CICP in image decoder - #3070
Conversation
|
How does this handle different CICP priorities vs. other color information? For PNG the block is by specification supposed to supersede other color information so that an ICC profile becomes a mere auxiliary hint whereas in other formats the role is reversed. I'm also not sure how to deal with errors in these cases. The ICC can itself contain a CICP which must be consistent with the profile itself. While that is consistently specified the situation of encountering unsupported by well-formed data here is odd. We don't want to error but the ImageReader could conceivably:
|
I've updated the The decoder implementation should ensure this is the case; if not, then users of the trait (like ImageReader) risk choosing arbitrarily.
The PNG decoder returns Ok(None) for the ICC profile when a cICP tag is present; it could also return a synthetic ICC profile derived from the cICP tag. (And for For a format where the ICC profile takes priority, if any ICC profile is present, then the decoder could just return an UnsupportedError in |
I find that a bit odd. A major incentive for the structural change of |
That being said, I'm certainly not certain that this PR is the right way to handle the issue. One alternative I've been thinking of is a
This is tricky, because image format specs may conflict with metadata specs. DPX explicitly permits external metadata but recommends that the main file metadata (like orientation or CICP codes) override any present in external metadata (which presumably includes XMP and EXIF), except when left undefined; JXL files always ignore EXIF orientation, as should AVIF. |
|
It is indeed tricky, which is why we should be doing it right. Regarding non-icc/cicp color space information in combination with standard ICC data we could have a separate method for any profile synthesized by a decoder. At least that would fit with I continue to be weirded out by JXL and heif directions, this time for explicitly refusing to push complexity through dedicated standards as if that removes said complexity from reality, but reading through the ''justification'' given in the AVIF thread I seem to at least understand what collective mind parasite got us there—so now we have more complexity, yay. Since: "Metadata of type Exif as specified by JEITA CP-3451 may be present in the file and shall be output by the MIAF reader" but we must always heed "irot and imir" and it's definitely wrong to apply twice. Requiring "no image transformations […] indicated in the Exif metadata" as opposed to presuming equivalence is absolutely asinine. So now re-encoding such an image into another format is no longer simple, requiring processing of of metadata to scrub orientation; and suddenly a re-encoder must deeply understand every kind of metadata to modify it accordingly but of course it never does, evidently imagmagick does not, and then you get broken images. (Sidenote that CSS's |
e1138c2 to
0773197
Compare
I've updated the design of the MR to expose ICC profiles and CICP value through new (I've made the changes somewhat quickly to get the core ideas out, so the decoder implementations may need a bit more work.) |
8b9a192 to
3295663
Compare
I was indeed missing a few implementations; now every format which could expose an ICC profile should now have the (Edit: I may tweak decoder implementations over the next few days to help make it clearer how this might play out, but the current change to the ImageDecoder API is certainly ready to review.) |
bde8429 to
6b29cf9
Compare
|
I do like the new sketch better than the previous for sure, just the duplication in conversion between different color profile representations is rather annoying. It's of course plausible for (many older) formats with much more special representations but will just happen to be a lot of close-but-not-duplicate code otherwise especially for |
20632cd to
cd3ecb6
Compare
RunDevelopment
left a comment
There was a problem hiding this comment.
Just some thoughts. I don't know a lot about color, so please take what I say with a grain of salt.
|
|
||
| /// FXPT2DOT30, 2.30 fixed-point | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| struct Fxpt2Dot30(u32); |
There was a problem hiding this comment.
It seems to me microsoft followed its own convention where the sign is declared as part of the fixed point format, so instead of Q1.30 this is Q2.30, as these seem to be signed types in microsoft SDK. That's actually makes sense, since those coordinates might be negative.
There was a problem hiding this comment.
The CIE XYZ color space is constructed to ensure X,Y,Z are always nonnegative, see https://en.wikipedia.org/wiki/CIE_1931_color_space#CIE_XYZ_color_space, so I don't think this is likely. The definition of xyY also ensures x+y+z=1, so I expect x and y to only be valid if they are in the range [0,1]. Within this range, the high bit of 2.30 fixed point will never be set, so for valid color primaries it shouldn't make a difference whether FXPT2DOT30 is signed or not. So I think it is safe to make this change, and have done so.
There was a problem hiding this comment.
Out-of-gamut colors are well-defined, algebraically at least, even though they may have no physical meaning. Afterall, some "subtractive" part is pretty much how the experimental setup for color matching CIE XYZ was defined in the first place and why the observer functions are in part negative. It's not even true that all colors lie in 1931's XYZ given that the blue and red monochromatic light sources were rather narrower than appropriate. (Should I be buying CIE 170:2-2015 for 190€? Idk).
At the end of the day, primaries are just base points so that the linear interpolation region covers more the physically possible color space sensibly. If you want to reduce clipping of computational artifacts it's not absurd to have negative x or y coordinates. And clearly the winsdk considers out-of-gamut definitions to be possible.
1f153fb to
691b1c1
Compare
Decoders for formats that use CICP integer values may need to perform these mappings, and they are most easily kept in sync with the enums if implemented in `image`.
This should make no difference for valid primaries, whose xy coordinates are expected to be between 0 and 1. It matches the signedness of the corresponding Windows type.
Color metadata continues to evolve; this design tries to support future evolution by constructing an opaque intermediate DecodedColorProfile type that can be extended to support a wider range of format-side and and library-side color profiles in the future.
In most cases this is done forwarding to the old icc_profile method, but formats with synthetic ICC or CICP profiles are more complicated.
This adds a method which exposes the image color space information as CICP values when possible. This is immediately useful for decoders for formats like PNG, which directly supports CICP; but also for specialized formats like HDR (uses linear transfer function, although the official default color primaries are weird) and QOI (has a linear colorspace option that I expect is used for non-color data, assuming it gets used at all).
Edit: the current revision also updates the way ICC color profiles are communicated.
This PR is somewhat minimal; I've not added support for CICP in AVIF as that requires dependency upgrades; or tried to map the full set of PNG color chunks (gAMA, cHRM, sRGB) to CICP when possible; or looked at to what extent various other formats (BMP, JPEG, WEBP, Farbfeld, OpenEXR, TIFF) could have color space information implementable as CICP. But this (along with synthesizing ICC profiles as needed, similar to the way the BMP decoder does it) is something I'd rather defer if there is no reason to expect it to break the core design.
This would close #2985.