Add support for reading and writing the mDCv and cLLi chunks #626
Conversation
|
FWIW, the PNG Working Group will soon be discussing possibly changing mDCv to mDCV & cLLi to CLLI. |
There was a problem hiding this comment.
There should be no changes to either png_struct::colorspace or png_info::colorspace firstly because at present the structs are metadata (informational not authoritative) secondly because app involvement is required; there is no API to say because we don't know what the output colour volume is and thirdly, perhaps most important, there is no cICP handling in ::colorspace yet.
I can't see any way of making the colorspace handling useful without an enormous amount of work so this should be removed pending review of what the W3C actually want.
There was a problem hiding this comment.
How the data is stored should be internal. The spec is a bit of a mess because it introduces a new way of storing a colour but note how png_xy is defined in pngstruct.h then look at png_get_cHRM in pngget.c; libpng for the most part does not return results in structures. It returns them via individual integral or floating point values.
Introducing a new and weirdly different representation of what is effectively the same as a cHRM chunk (so far as the colours are concerned) is just going to be plain confusing; I think it is confusing in both the W3C proposal and the libpng API but I only really care about the API.
Looking at the Blink code I can see that that code handles this stuff as a 24 byte array; the suggested API is just going to massively complicate at least the current Blink code. One possible API is just to return a pointer to the buffer, that will be a lot easier for the existing implementation that I've examined. If I were the person responsible for making the Blink code compatible with libpng 1.8 I would do nothing; it will just work. What really matters is whether the API is comprehensible to people who haven't encountered HDR or wide gamut images before.
A better approach IMO is to unify with cHRM and return, at the caller's option, either double precision FP values or png_fixed_point (so two APIs). I looked at this before and came to the conclusion that this is the least confusing approach.
It's up for discussion. I'm against introducing a new struct at this point but I might be in favour of introducing general representations of both colour and illumination level (using SI units of course).
There was a problem hiding this comment.
I would rather not see any more of this "writeable pointer into an internal structure" stuff introduced. It's a security nightmare, at least that is what happens every time I see it. This is a second though perhaps most compelling reason for returning the values as integers.
An alternative, while it may be inconsistent, is to require the caller to allocate the buffer. This is much much more like every other piece of code out there where the caller passes a pointer to a struct typically allocated on the stack. This is also a powerful argument for the alternative of requiring a 24 byte buffer and just filling it it; png.h exposes png_get_uint16 so, while perhaps weird, it provides all the support that, so far as I can determine, @ProgramMax is looking for, without giving out pointers to structs with an undefined layout.
Horrible. Better to give up and implement libpng 2; the unknown handling already supports the last paragraph...
There was a problem hiding this comment.
Ordering constraints WRT PLTE cannot be imposed on "safe-to-copy" chunks, so the "out of place" checks are incorrect. This is why the chunks' safe-to-copy bit is up for review.
There was a problem hiding this comment.
You need to compile with either gcc or the equivalent of "-Wall -Wextra -Werror". Testing an unsigned value for being less than 0 can reasonably produce a warning.
It is also the case that chromaticity values less than 0 are reasonable; see the other discussions about the ACES profiles. Note what happens with either x or y values greater than 50000; the representation is opportunistic but such spurious grasps of 24 bytes in a PNG stream should not be exposed to the callers of libpng. There may be more justification for movies but I'm somewhat doubtful.
A good set of test images needs to include codes with values of 50,000 and 50,001; as a minimum I can see an integer overflow here and that will be a security issue, but that's not a libpng issue since I can dump the same bogus data into other streams.
I think you can drop the mDCv checks and I note that you didn't include them in the cLLi code even though the representation is (I believe) identical.
There was a problem hiding this comment.
You could change this to "disabled" (IRC); cf @ProgramMax's comments elsewhere, but in that case I suggest doing the same for cICP.
There was a problem hiding this comment.
IRC this file is machine generated, I suggest deleting the change. @ctruta may care to comment (he removed the machine generated files).
There was a problem hiding this comment.
@ctruta: this file should be removed until RC
Hello,
This PR introduces support for the mDCv and cLLi chunks in PNG files, enabling the reading and writing of HDR-related metadata. These chunks were added in the third edition of the PNG specification.
The Mastering Display Color Volume (mDCv) chunk includes the following fields: Mastering display color primary chromaticities, Mastering display white point chromaticity, Mastering display maximum luminance and Mastering display minimum luminance.
The Content Light Level Information (cLLi) chunk contains two values crucial for HDR video: Maximum Content Light Level and Maximum Frame-Average Light Level.
I'm happy to make the necessary changes. Please let me know what is required.
Friendly ping - @ProgramMax, @jbowler, @ctruta you guys may be interested in reviewing this since these changes are quite similar to the support for the cICP chunk.