Remove cHRM check to accomodate ACES AP1 #579
Conversation
ACES AP1 has a red endpoint with a negative Z, this triggers the checks in libpng that ensure that x, y and z (chromaticities) are all >=0. This removes the checks on the sign of the chromaticities since it is valid to use negative values for any of them and converts the "internal" error code return to external (because the internal cases correspond to negative x, y or z.) Signed-off-by: John Bowler <jbowler@acm.org>
|
REVIEW REQUIRED: @mbechard |
|
@fuzzers: please review this with fuzzed cHRM values. |
|
@mm2 the original (way, way back; 10, 15 years) issue was produced by a PNG with a cHRM which had colinear (IRC) rgb xy points. This was before any of the checks and caused issues somewhere (it was a long time ago), possibly CIEXYZ with NaN values. Somehow the result was a crash in lcms (possibly the author of the bug passed signalling NaN values in; I don't remember lcms supporting endpoints specified as chromaticities.) This change should retain the check on colinear values but will allow cHRM chunks which result in CIEXYZ triples (to use the Windows terminology) with negative Z values, at least for the ACES AP1 and AP0 colour spaces. |
|
If verified this closes #578 |
Thanks for let me know. FYI, ICC addressed use of negative XYZ many years ago |
|
Yep, looks good to me and I'm able to write->read a PNG using AP1 primaries. |
Yes; negative XYZ were always meaningful. Unfortunately the original PNG spec defined the chromaticity values as "unsigned". This doesn't arise in the adaptation case because the negatives come from the adaptation and it doesn't arise with ACES AP1 (that was just my over-enthusiastic error checks). It does arise in PNG with AP0 because the red y value is negative and significantly so; the rounding hack I suggested to @mbechard won't help. @ctruta; this really needs to go into libpng16; it's an unexploded bomb that only hasn't been reported before because no one (apparently) has generated PNG files with the ACES chromaticities. That's going to change as a side effect of the W3C initiatives; these will make PNG more appropriate to video workflows and will, indirectly, encourage the use of wide gamut colour spaces. It is a bug fix, nope, not security but then surely neither are changes to parts of the build system that have never worked? I suspect @mbechard might support me in pushing for this. |
|
Yes for sure. Although I'm currently defaulting to ICC profiles if they are available, I think that will go away for the more common color spaces (ACES/DCI-P3/Rec2020) as they are easily defined and manipulated with matrices, which allows them to be done on the GPU. This will be important for high-res image sequence playback where I suspect running the image through an ICC transform on the CPU will end up being the bottleneck for playback performance. Being able to use the cHRM instead of ICC will be optimal in the future, when it's appropriate. |
The only one that I'm aware of which you haven't checked in to libpng16 is #557 I'll keep looking. |
ACES AP1 has a red endpoint with a negative Z, this triggers the checks
in libpng that ensure that x, y and z (chromaticities) are all >=0.
This removes the checks on the sign of the chromaticities since it is
valid to use negative values for any of them and converts the "internal"
error code return to external (because the internal cases correspond to
negative x, y or z.)
Signed-off-by: John Bowler jbowler@acm.org