Add support for reading and writing the cICP chunk #565
Conversation
This chunk was added in the third edition of the PNG specification and contains Coding Independent Code Points (related to color space description). It is fairly simple as it only contains four fields of one byte each: Colour Primaries, Transfer Function, Matrix Coefficients, Video Full Range Flag. The test file originally comes from the related WPT test case: https://github.com/web-platform-tests/wpt/blob/master/png/support/cicp-display-p3.png Note that I reencoded the file to make it match libpng's default encoding parameters (it only modifies the IDAT chunk).
There was a problem hiding this comment.
Mostly correct in-so-far as it goes however I think there is a major bug in pngrutil.c. To quote September spec (I don't think it has changed since):
https://www.w3.org/TR/png-3/#cICP-chunk
When the cICP chunk is present, decoders that recognize it SHALL ignore the following chunks:
[iCCP](https://www.w3.org/TR/png-3/#11iCCP)
[gAMA](https://www.w3.org/TR/png-3/#11gAMA)
[cHRM](https://www.w3.org/TR/png-3/#11cHRM)
[sRGB](https://www.w3.org/TR/png-3/#11sRGB)
In fact libpng adopts a failsafe approach; when conflicting colour space information is present libpng invalidates the colour space. See the handling for the other colour space chunks. In particular "png_handle_sRGB" is your friend because it is so simple.
You need to add the requirement for colour space support to pnglibconf.dfa as a result (also, stylistically, cICP is a colour space chunk!)
There are quite a few other issues; things like error checking that are missing. I've only mentioned the one obvious issue that I see with well formed chunks; you certainly need to review png_set_cICP!
Broadly I suggest you review png_colorspace_sync, change all the spellings of "colour" to "color" (this is not a foolish consistency), add the error checking to png_set_cICP, and work out how to make png_get_cHRM_XYZ to work.
It's a spec issue but the valid values for all four of the fields are not specified in the current PNG-3 specification. That's a big issue because those values affect how libpng handles the colour space. If you don't solve that issue then there isn't a lot of point to this PR because any app that is prepared to do the work can already do it with the unknown chunk handling!
There was a problem hiding this comment.
@ctruta: note that PNG-3 has not yet even been submitted as a PR (Proposed Recommendation) because some parts of the spec refer to documents that are themselves drafts and, in fact, cannot even be read by mere mortals such as me.
|
To be honest, I realized that I might need to do more work regarding the validation and in case of conflicting information, but I also wanted to take the temperature here before investing more time in a PR. I'm happy to see your positive answer, thanks for that!
When I wrote this PR, I was wondering about which approach to take. What should we do if the client doesn't support cICP but does support other color profile chunks, I guess it depends on where we draw the line of the "decoder". Thanks for shedding some light on that.
I forced myself to do that for consistency with the spec, but I'm firing
To clarify, you want a user to be able to call
I'm not sure to get you, it seems pretty well specified AFAICT. Should I wait for ctruta's answer before resuming my work? |
|
This is a long response because it is about the details of design and implementation. There's a TL;DR section at the end, after the PATENTS section, which I separated out because it is off topic so far as design and implementation are concerned and a rant but it does address your last but one comment. I suggest separating that stuff out to the separate comment I made to @ctruta
That is a big problem and one I didn't mention since I can't see a solution other than complete support. libpng does not know what the application supports or does not support and deducing this from app "png_get_" calls is tricky, nevertheless this is part of one possible approach. The three "pure" approaches I see (each approach does not preclude the others) are:
Given those three options my recommendation is to start with a minimalist (2); broadly the code in this PR with a minimalist png_colorspace_sync extension (no invalidation of the other chunks) and error checking in png_{handle,set}_{cICP,mDCv}. It would be possible to build on this in a future major release to accommodate full-range PQ and HL and even narrow-range images. The signaling values could be handled on a single image using an alpha of 0 or with float/_Float16 output by replacing them with two NaN values (there are only two narrow range signaling values represented as 0 and 255 in 8-bit, 0 and 65280 in 16-bit [EDIT: narrow range images only, so the "shift scaling" equations in H.273.8.3 apply) This comment I made was misleading and incomplete:
At best I over-simplified. If you look at "png_colorspace_sync" you will see it only does anything on read and all it does is copy the "png_colorspace" struct in png_struct to the passed in png_info then recalculate the relevant chunk "valid" flags. The png_handle routines for the colorspace chunks; gAMA, cHRM, sRGB, iCCP and now, potentially, cICP and mDCv fill in the members of png_colorspace with the values they have by calling one or other of the png_colorspace_set_ functions; see pngpriv.h, try searching through pngrutil.c for the string "png_colorspace_set_". This stuff only happens on read. The write code does, in fact, allow PNG files to be constructed in whatever way the caller of the write code wants. It only validates the individual chunk contents not whether the chunk makes any sense or is valid where it is written. In fact technically png_set_ is only a part of the write API; it's not part of the read API but it is called internally by the internal (non-API) function png_handle_. The result of calling png_set_ while reading a file is not defined, I think I might have made it error out in one version of libpng 1.7. Likewise png_get_ is a part of the read API and should not be used from write (though it can be and doing so is harmless). IRC the colorspace handling was not like this in 1.5. It was pretty much a complete mess in 1.5 and was certainly unmaintainable. In 1.6 (approximately) I rewrote it to introduce the png_colorspace as a place in png_struct to gather together all the colorspace information that came from the chunks. In the very original architecture the read code filled in one or more png_info structs provided by the app then the app had to extract all the relevant values and store the relevant data in completely separate fields in png_struct (png_read_struct) before attempting to read the image. This maybe would be ok when there was only gAMA and cHRM but when sRGB and then iCCP were introduced the app would have had to check three different chunks to find the PNG file gamma or chromaticity. So at some point png_handle_gAMA started making a copy of the png_info gamma value in the png_struct it was passed. Now the app only had to supply the screen gamma. See the comments on png_set_gamma (in png.h). The endpoint (cHRM) handling does not suffer from this problem because libpng does not supply any code for handling different encoding endpoints; it all has to happen in the app. There is no "png_set_endpoints", just a set of png_get_cHRM functions (search for png_get_cHRM in png.h). Most important, while libpng does use the file gamma during image read when required, it never uses the end point values. However you are correct about the net result; because the png_info (the one filled in before IDAT) has records of both gamma and the endpoints and the png_get_ APIs use these values. IRC prior to 1.6 they did not; the png_info contained values from the original chunks, the png_struct contained the potentially different values which would be used during read. You can see why I tried to simplify this :-( There is a poison pill in the transfer function handling. It was first created by the addition of sRGB. The recommendation was that the encoder write gAMA and cHRM as well, but if it did not apps which did gamma correction by calling png_set_gamma on the result of png_get_gAMA would stop gamma correcting. This was potentially a big deal at the time. Nevertheless PNG-3 reintroduces the same poison pill; now sRGB images can be identified using cICP and, to make it worse, the spec even says how to do this for an sRGB image and instructs decoders to ignore gAMA and sRGB! JB sticks head in bucket of water. The net result is that encoders which write cICP are likely not to include even the sRGB chunk (only 13 bytes) if they write cICP for sRGB (valid, only 16 bytes). Approach (2) allows libpng to avoid swallowing the poison pill by only doing cICP handling (the "SHALL ignore" part I quoted previously) iff png_get_cICP (or some equivalent API) is called. Howver this only works if the relevant transfer functions are recognized regardless in png_handle_cICP. In fact the available "ColourPrimaries" (first byte of cICP) are all simple RGB endpoints defined as chromaticities (same as cHRM) and libpng already contains the (non-trivial) code to convert these to CIEXYZTRIPLE (Windows). This is table 2 in this link: https://www.itu.int/rec/dologin_pub.asp?lang=e&id=T-REC-H.273-202107-S!!PDF-E&type=items As I discuss below in the TL;DR section that link is not the one from the spec. Nevertheless the current (inaccessible) document is unlikely to have changed from using chromaticities and if it did it will be to XYZ (the conversion from XYZ to xyY is trivial, chromaticities are just xyY without the Y and with a checksum.) So "doing" the endpoints in png_colorspace_* is a no-brainer for any cICP. The "transfer characteristics" in section 8.2 are a big deal. In fact section 8.2 and following includes handling for narrow range images (it defines the scaling to 16 or 8 bits, the only options in PNG) however many of them are defined in terms of power law (gamma) encoding or modified power law where the modification is a linear segment at the start. The latter is, in fact, what is assumed inside libpng when interpreting gAMA. A little bit of work should be sufficient to come up with appropriate gAMA values which result in the correct transform within libpng. Indeed I think the modified power law encodings within the table can be directly accommodated within libpng even though the only transfer function currently supported is the sRGB/REC 709 modification. Other variants are currently handled in the way Apple implemented the modification on Macintosh computers [at least according to Poynton's lectures around 2000]. PATENTS My understanding is that using endpoints ((0,1),(0,0),(1,0)) as in colour primary method 10 was patented. It may have expired but I still don't do that. There's no guarantee that the methods in REC-273 are not patented. I think the code needs to be set up to allow specific endpoint and transfer function handling to be disabled. That's more important if the novel transfer functions are implemented; libpng does not interpret the endpoints and I suspect we don't violate a patent by simply reporting the values. Implementing the transfer functions might be a big deal, but approximating the modified power law transforms using the existing code shouldn't create a problem. The existing code dates back to the inception of PNG; 1993 or maybe 1994, I haven't received any word that the modifications since have violated anyone's patent, but the copyright holder (@ctruta) has to comment on the current status. TL;DR - the rest of this post is process, not implementation, related:
They don't let me read that document. Your link was to the PNG-3 spec, not the document. Here's the actual link to the document: https://www.itu.int/rec/T-REC-H.273-202309-P/en Observe these words:
The document is (one page before the dull foad above, emphasis added), "In force (prepublished)". I've been relying on this "superseded" document from 2021-07-14: https://www.itu.int/rec/dologin_pub.asp?lang=e&id=T-REC-H.273-202107-S!!PDF-E&type=items That is a real link to a real document. So far as I am concerned everything else is hoopla. I said, "Valid values for all four of the fields are not specified in the current PNG-3 specification." What I meant was that the values are not specified in the actual text of PNG-3. Instead they are specified in a reference, a level of indirection, which I may not read. This is part of the basis of my separate comment to @ctruta. I think there are a couple of other examples of this use of indirection to remove the opportunity to view the spec from, as I put it before, "mere mortals." The other part to this is that H.273 is subject to very rapid industry driven revision; they revised it in 2016, 2021 and, now 2023. That's because it is a hot area of competition IRL so there's a lot of money there. The PNG-3 spec could have frozen the indirection to a specific revision of H.273, like the one FOSS developers are permitted to access, but by giving a link which is to the "latest revision" the very definition of PNG can be changed at the whim of anyone with enough money or, even, assigned power! |
|
Whoa. Somehow I am only seeing this today. I'm going to review the code first so I am not influenced by the conversation. Then I'll go through the conversation. Give me a little bit. |
|
Greetings! I'm happy to report that my on-again-off-again presence to this project is back to on-again.
Ok then, I won't say what I think about this commit 😜 |
There was a problem hiding this comment.
In general, it looks good.
I do have some suggested changes.
Also, I'm curious if we need to care about PNG_COLORSPACE_HAVE_INTENT and the likes. For example, if the cICP chunk claims to be sRGB should we check that and pretend a sRGB chunk was found for older clients? Should we:
colorspace->flags |= (PNG_COLORSPACE_HAVE_ENDPOINTS|PNG_COLORSPACE_ENDPOINTS_MATCH_sRGB);
I think not. I think an old client should be oblivious to cICP. They will be anyway because they haven't defined PNG_cICP_SUPPORTED.
But if a client does define PNG_cICP_SUPPORTED maybe we should.
It is a bit unclear to me because I'm not sure the use case. The spec says if a cICP chunk is found, it should ignore other color space chunks. So for libpng to partially parse the CICP data just to allow other color space chunk emulation seems like a use case we should be actively avoiding.
So I think we don't need that stuff. But I wanted to call it out and think about it.
| png_bytep transfer_function, png_bytep matrix_coefficients, | ||
| png_bytep video_full_range_flag) | ||
| { | ||
| png_debug1(1, "in %s retrieval function", "cICP"); |
There was a problem hiding this comment.
Not an important comment. Just an interesting observation that I may not have considered before:
All the common "in %s retrieval function" strings are probably collapsed into one literal. So this ends up saving some space at the cost of a bit of debug run-time.
An interesting trade off.
There was a problem hiding this comment.
That stuff isn't compiled in; see pngdebug.h It's not supported, or, more accurately, it's maintainer only and pulls in unexpected stuff. I've never even tried to enable it! Compilers use string table compaction algorithms. The one I know was implemented as a unix command based on finding common trailing substrings. Use "readelf -S .libs/libpng16.so.16.44.0" to see the section sizes; the bad news is the .text segment (i.e. the code) which is close to 1/4MByte, the string table is 12532 bytes (this is on a 64-bit Intel/AMD system).
| { | ||
| png_byte buf[4]; | ||
|
|
||
| png_debug(1, "in png_handle_cICP"); |
There was a problem hiding this comment.
Interestingly, the string literal sharing doesn't happen here.
| if ((png_ptr->mode & PNG_HAVE_IDAT) != 0) | ||
| png_ptr->mode |= PNG_AFTER_IDAT; |
There was a problem hiding this comment.
I don't think this is the right place to set this.
Rather, this is where we should confirm the IDAT hasn't yet arrived.
if ((png_ptr->mode & (PNG_HAVE_IDAT|PNG_HAVE_PLTE)) != 0)
{
png_crc_finish(png_ptr, length);
png_chunk_benign_error(png_ptr, "out of place");
return;
}
There was a problem hiding this comment.
Good catch. The chunk placement has to be checked first.
This code is extremely unmaintainable; it's a state machine implemented by copy'n'paste and then replicated in pngpread.c. Any non-IDAT chunk which interrupts the IDAT stream, even if misplaced, should terminate the IDAT stream but it looks like the only ones that do are the ones that are valid after IDAT.
But take a look at line 129 of png.c; line 132 should have already set the flag before calling any of the ancillary png_handle_ routines... But then I suspect line 129 can never succeed.
|
|
||
| if (png_crc_finish(png_ptr, 0) != 0) | ||
| return; | ||
|
|
There was a problem hiding this comment.
Insert after the read & finish:
/* If a colorspace error has already been output skip this chunk */
if ((png_ptr->colorspace.flags & PNG_COLORSPACE_INVALID) != 0)
return;
|
Going through some of the commentary:
That is also why I think it is okay for libpng to not handle the CICP values. There can be separate CICP code that a client updates independently of libpng. Also, CICP values are used in other image formats, too. So the client might use one CICP handler for all of their image formats. If libpng provided its own handler, it would just duplicate work. The separatation of CICP values from the spec & parsing is very intentional.
I was trying to find which spec this is. (It's been a while. Maybe it is already fixed.) But I got hit with a sudden bout of dizziness. I'll return to this later. |
These flags record whether the PNG file contained the relevant piece of information. This is not new code, indeed I thought @LucasChollet had not implemented the calls required (png_colorspace_set_...). The point about png_colorspace is that the information comes from multiple different chunks and some of those only provide partial information. For example gAMA defines the three cICP code points {matrix,transfer,full-range} but cHRM defines the cICP "ColourPrimaries" code point. sRGB and iCCP define the rendering intent (this is, in fact, Michael Stokes design) but other chunks (cICP and in some sense gAMA+cHRM) self identify sRGB images yet do not define the rendering intent. Consequently it is necessary to maintain the information as well as reasonably possible; png_colorspace does that. This is also the cause of the "poison pill". If a chunk defines values held in png_colorspace without updating png_colorspace (for example if it is ignored) application code which relies on those known but not parsed values will display/process the image incorrectly.
No, there's an already implemented function (png_colorpsace_set_sRGB) for that and it is very important to call the function, not DIY; prior to png_colorspace everything was DIY and, because the same data came from different chunks DIY means code replication. That was a large part of the mess.
That macro is defined by the builder of the library and @LucasChollet's change defaults it to "on". If you've done a "gh pr" build it and look at the contents of the generated file "pnglibconf.h". All application code will be oblivious to cICP initially because the app (etc) code does not call png_get_cICP. There's no way round this; the app has to ask, it can't be told! What is more the app has to do something to get any colorspace handling at all.
If the app asks for gamma correction it doesn't expect it to only happen if a gAMA chunk is present. It expects it to happen when PNG file contains gamma information! If the app asks for the encoding endpoints it expects to get that information if it is in the PNG! I guess a future major release could change the names of the png_get_ APIs to not use the chunk name, but why? Likewise "png_get_cICP" could be replaced by four APIs to get the H.273 code points and the full-range flag. That would make things more clear, but png_get_gAMA and png_get_cHRM just return the single piece of data requested so it will be really confusing if they suddenly change to no longer working if only sRGB or cICP are present (i.e. not gAMA and cHRM). Also it's a change, an API change to png_get_sRGB. libpng has to parse the whole of cICP; png_get_cICP, regardless of how it is implemented, will return the parsed values and if those values have not been checked for validity libpng will get bugs. This has happened to us many, many times; even eXIf chunks are semi-checked and gAMA, cHRM, sRGB and even iCCP are all validated quite extensively because otherwise the apps or other library code crashes and this gets reported as a bug in libpng! |
|
I think the default for PNG_cICP_SUPPORTED should be off because it relies quite heavily on the library user explicitly handling wide color gamuts and HDR, which is not the default.
I hear what you're saying. The only correct way to handle the CICP information is for the library user to be aware of it and handle it. It is also tricky because the cICP chunk could specify an HDR color space which doesn't use gamma. An unaware app might read the updated primary locations but not get an updated gamma, resulting in incorrect colors anyway. Or, there is future concern about the potential for 4 primaries. So maybe we just push the first 3 into RGB, which isn't correct. And the unaware app is again wrong. We could parse CICP and update primaries when it applies. But the behavior of only sometimes updating seems problematic to me. |
That's my option [1]; the app (or library) does all the work inside the unknown handling. It just works. But I think you don't understand how libpng is installed in most desktop computer systems (not Windows, which does not use libpng, or at least did not when I last worked for Microsoft). On those systems such as UN*X systems including Linux and the BSDs and derivatives, libpng is installed as a shared library; a DLL. That means the OS vendor builds a single instance of libpng with a single set of compiler options and that is it. On those systems the code for most libraries is also distributed in DLLs; think Qt for example. Applications are also typically distributed as binaries. On my own dev system (gentoo running KDE as the window system) there 43 separate packages which depend directly on the installed libpng including stuff like qtqui, openjpeg, opencv and qtwebengine. qtgui is itself a DLL and there are 107 packages which depend on it. The full closure of all the libpng dependences gets very large very rapidly; on my system I have 1592 separate packages that depend directly or indirectly on the libpng package; not libpng, the actual package which installs libpng as a DLL. Google Chrome uses the installed libpng DLL. IRL there are also app and library authors who distribute pre-compiled binaries that run on many of these operating systems; you can build one CPU family specific binary and have it run on any number of different Linux and even BS distributions. The vendors who do this rely on the operating system distributions supporting the full API on each DLL. If just a few of these systems have a libpng with PNG_cICP_SUPPORTED not defined in pnglibconf.h then, because these are binary distributions, the vendors will have to build without using the API on systems where it is defined. They can and will do what I suggested; use the unknown chunk handling code rather than any built in support. What is more this is unlikely to change; why would they change to using libpng "support" if it just does what you suggest when they already have code which does as much but works on all versions of libpng, even libpng-1.5 and earlier? Once one significant package has done this it is set in stone. What you are proposing clobbers the unknown handling of the three chunks involved; those chunks no longer get treated as unknown and that breaks apps and libraries which handle them via the unknown chunk mechanism. In other words turning the option on or off breaks ABI compatibility as well as API compatibility and that is not an option in a minor release; there is a choice, [1] use the unknown handling or [2] or [3] make the chunks known. Once made it can't be changed. If PNG-3 is adopted before there is support in libpng adding support becomes a major release of libpng. |
|
Is @VELUCA out there? It would really help if you could comment. The gwenview behavior I referred to above comes from qtwebengine however the actual code is something you wrote in png_image_reader.cc. You are explicitly calling png_set_keep_unknown_chunks for the three chunks (cICP, cLLi and mDCv). The comment says, "[W]hen libpng starts supporting cICP/cLLi chunks explicitly, remove this code." I don't see how you can do this without borking your downstreams as discussed above; some version of libpng might support the chunks, at which point the "set_keep_unknown_chunks" becomes a no-op (there is a way round this) but you can't guarantee which version of libpng you have unless the change is in a major release. I.e. the change might be made in libpng-1.6 but you will never know you have libpng-1.6 with the change because you only know at runtime (via png_access_version_number()) and your code will never load to even call that if it includes an unresolved reference to the relevant png_get_ APIs. I know and I'm sure you can work out how to fix this but the fix means you don't call the APIs... |
|
You're right that I wasn't thinking about the dynamic linking case. Although, your wording of "you don't understand" is condescending and problematic. This is the second time I had to call you out on this behavior. Please be more careful. That kind of behavior makes it harder for people to work with you. Projects like this are big and need multiple people. If it comes down to a moderator's choice of "I can let these people leave or I can ban this one person", you are making yourself a target. I now favor default on but this also means it needs to have no side effects for apps which do not understand the cICP chunk. I read through all the above comments now. My responses here are grouped into topics. I tried to put similarly themed topics near each other: Color space chunks causing invalidationWe should definitely not invalidate when given conflicting color information between cICP and others. If we do, we're going to run into problems. Suppose cICP indicates a HDR color space which cannot be represented using any other chunks. The only way to avoid conflict would be for no other color space chunks to exist. That means a cICP-unaware app will treat it like sRGB by default. But sRGB is very far from HDR. The image author could have provided color space chunks as fallback to at least get closer to HDR. That is why we specified a priority for color space chunks. (If the chunks contained the same color space info, a priority wouldn't be needed.) Further, an app running an old, pre-cICP libpng will see the closer, more accurate image if extra chunks are provided. Until they upgrade libpng and it breaks. So now there would be an incentive to avoid upgrading and file bugs against libpng. Meaning if we enforce this, image authors and libpng users will have a worse experience. That will hurt the road toward adoption. The upside in this trade-off is apps could automatically get upgraded color space handling when it applies. But this only applies if the app wouldn't have to do anything. In the case of HDR (and thus cICP), it does have to do something. So the potential upside can't happen anyway. Or we could choose to only sometimes updated the shared color space information, when it does apply. I would be willing to consider this path. But this means the behavior is inconsistent. So said another way, the always-invalidate goes against the spec and is worse for everyone. Shared color space info
Only if the cICP chunk doesn't update the currently-shared color space information. Which is incompatible with the next thing you said: Interface design
I agree with the ability for apps to get the final gamma or pixel values. But that should be a separate API on top of the parsing API. The app should be allowed to know if this information came from gAMA or not. If the only API available to apps hides this info, we're forcing every app to adhere to whatever assumptions we make. An example app is TweakPNG. But more broadly, for the color space chunk priority to work an app would need to know "This gamma was set but isn't the actual value we want". This idea is also incompatible with the safe-to-copy bit of PNG. If the app doesn't know where this gamma value came from, it can't write it back out the same way. I'll grant that no apps I know of actually care about the safe-to-copy mechanism. But that is a problem that should be addressed in the spec. And libpng is supposed to be a reference library to the spec, right? Really, there is a world of difference between png_get_gAMA() and png_get_gamma(). png_get_gAMA() misrepresenting what is in the file is a problem. That shouldn't have happened. Forcing unknown chunk handlingRequiring the app to handle cICP via unknown chunk doesn't make sense to me. Why should libpng handle any chunks at all then?? Just make the app do it every time. I understand that apps which currently use unknown chunk handling won't pick up a now-known chunk. This is another interface problem. Perhaps there should have been an override handling option rather than an unknown handling option. To fix that, perhaps we could put PNG-3 features behind a flag at least temporarily? It would still have a problem where apps need to update at the same time as the system library. But it would provide a big on/off switch for distros that are building all these apps. That would let them enable it all at the same time. Linked spec hidden from "mere mortals"The H.273 spec is indeed still locked down. But it previously was and will again be free and open. I don't know why they lock it down during an update. Don't worry--we're very strict about only referencing free & open specs. We've even been able to convince others to make non-free specs free so we could reference it. |
There was a problem hiding this comment.
The file is invalid and should fail; cICP is following PLTE. See table 6 in PNG-3. I suggest you just add an sRGB cICP to pngtest.png (in the correct place; run pngtest a couple of times to sort pngout.png).
Gwenview reports this error (using the current, unpatched, libpng):
"Gwenview cannot apply color profile on QImage::Format_Indexed8 images."
I'm guessing Qt6 already has support for cICP but I'll investigate.
UPDATE: I see what happened. I hadn't realized you had run it through pngtest (I should have known given that I know pngtest fails on chunk-reorder!) Your call to png_write_cICP is after the call to png_write_info_before_PLTE. I moved it to line 140 (i.e. inside the PNG_COLORSPACE_SUPPORTED, after png_write_gAMA_fixed) and that fixed the problem (since the read code doesn't notice the original error).
You can fix the file easily with TweakPNG (which preserves but is capable of altering the order).
FYI the three individual png_write_info_... calls can be called with three different png_info structures as can the three sequential png_read_info_ APIs but maybe not the progressive reader. Doing that (using three png_infos) is the only way to control where chunks which may appear in multiple locations appear.
I think you have the wrong veluca :-) |
Ha! There's only one jbowler :-) (Ironic: there was a DA in CA at one time, but [s]he is not jbowler.com)
It's in the code of QtWebEngine, which bundles the Chromium code in turn. On my system both are built against the installed libpng DLL; not doing this makes it very very difficult to test changes to libpng by running the app! I didn't scan the code in detail because I try to avoid bug-fixing commercial code (there is a danger of later being accused of a copyright violation) but it looks like @the-real-veluca (always a good prefix to grab on Google) did, in fact, get it right and my argument was fallacious being based on a recurring misunderstanding of the libpng unknown chunk handling. The code should be robust, but I suspect something to do with the APNG parsing breaks it. (It is known to be broken with the APNG patch). |
|
I think John's suggested fix needs to happen here. Are there any unresolved issues still? |
|
I was quite occupied lately and haven't had the time to work more on this. I should be able to resume my work this week or the next one. |
|
Friendly ping. |
|
I landed it in the branch After all looks good from my point of view, I can then apply them to the |
|
It's going to cause API changes. If someone calls rgb_to_gray on an image with cICP then they will typically end up with a black image. This is a very common operation; most of the AI stuff seems to end up using it because the AI code mostly doesn't handle/use colour. An API add is also an API change. An app can't use a new API added to a libpng after 1.x.0 safely, so apps don't. This is why it is so important to have libpng 1.8.0 - it can coexist with 1.6.0, people who get black images will just have to relink against 1.8.0. 1.8.0 will, if my changes are accepted, have fixes for the inability of the code to handle any (screen) gamma more than about 2.5 (the results get increasing out-of-whack with increasing gamma; the lower intensity grays coming out black). Some fixup can be made for cICP such as just keeping the green channel, or even doing it properly (since I could...) What matters for me at the moment is getting the backlog of pull requests pulled into libpng 1.8, then I can start working on the backlog of issues and reintroduce the more complex requests that I pulled because it was just getting too confusing. |
|
Could the default be sRGB? That is how a cICP-unaware app should behave anyway, right? |
|
What I mean is if the image did not provide cICP, rgb_to_gray wouldn't produce a black image, right? Why not keep that behavior? libpng isn't able to handle cICP on its own, anyway. Normally, you would tag the texture with say Display P3 and just fill it with the stored, unaltered values. The OS knows how much HDR headroom to provide and what the monitor's capabilities are. libpng doesn't know these things. Nor should it. If there are multiple monitors, libpng doesn't know what monitor is being used to display this content. If the goal is to convert rgb to grayscale and preserve luminosity, it would still need to know what the OS is doing, what the monitor is capable of... |
|
Also, if the device has a light sensor (phone, tablet, my laptop even) the OS will adjust HDR headroom and luminosity on the fly. All of this is happening on the GPU. It wouldn't make sense for the user to call rgb_to_gray on every light sensor update. So any colorspace-aware conversion we would be doing is an awkward fit. |
If the calculation is done using the current libpng arithmetic it gets done with about 15-bit precision. That can preserve a dynamic range of about 8EV before banding starts. Below about 11EV everything turns black. Those figures are relative to the "1.0", the maximum luminance. So for PQ that's a low limit of about 4cd/m^2; divide by 2000. This might be OK but the issue is that we don't expect to see "1.0" that much, if at all. This is a bug in libpng (see the end of this post.) Doing it correctly would mean doing the full inverse OETF, then the calculation, then the OETF, so that's not going to happen any time soon. @ProgramMax: note that this is happening as a transform of the original scene from colour to black and white; it's got nothing to do with the EOTF. If the calculation is done using the libpng default behavior it gets done assuming that the values are already linear. In effect it does what Poynton has traditionally complained about, see the start of the first paragraph here: https://poynton.ca/papers/SMPTE_98_YYZ_Luma/index.html libpng doesn't need to know the underlying encoding; the result is always the "luma" calculation he describes. libpng just needs to know the r,g,b endpoints in xy space because those define "luma". However this still results in "black" images or, at best, seriously darkened images. This is because if the app does ask for rgb_to_gray it must chose to either preserve cICP (which is wrong) or junk it. Two possibilities:
In both cases libpng has, apparently, destroyed the image. In the second case (the current behavior) an image that displayed correctly because the display system interpreted cICP correctly now displays as sRGB (or something) because cICP has been removed. In the first case cICP is still there but it's wrong because the actual encoding of the pixel values has been completely modified, destroyed, by the luma calculation. The easy fix is to disable png_set_rgb_to_gray and png_set_background (used to remove the alpha channel) if cICP is present, or if cICP is present and indicates HL or PQ or, indeed, a narrow range image. The even easier fix in libpng-1.8 is to just remove the two APIs and see what happens. The actual implementation of both is broken anyway; there are at least two known bugs and both are difficult to fix. |
That's what libpng does and always has done. Originally it required the app to provide the correct calculation but defaulted to the 709 endpoints (which are the same as sRGB).
That needs to be asked on png-mng-implement. I've always maintained that libpng should not be doing image processing but it was doing so and therefore I also maintained the code... I personally vote for getting libpng out of this business; remove those APIs and remove png_set_quantize. The simplified API provides a sufficient subset of the same functionality by supporting swaps between the 5 image formats but in a way that is, I think, pretty clear (and clearly limited). Removing the underlying APIs and the code associated with them would reduce the size of libpng, make it more maintainable and eliminate a bug-farm. @ctruta: this needs to be decided pretty much today. I'm not inclined to fix any more of the bugs in the low level code unless there is a clear decision on the way forward; it is at least 1 month of work. This is particularly true because since it actually seems to be opposed by the W3C and is something I have always opposed I don't think there is a strong motivation for keeping it. Certainly I'm not motivated. |
|
You definitely have my personal approval to remove image processing code. :D Although, I guess I should go read the png-mng-implement discussions to get more input. |
That's a viable approach for libpng-ng. Indeed it's consistently used across the industry for API incompatible changes in major versions (Qt6 has a qt5compat DLL). I'm not sure exactly how jpeg-ng did it but I believe there were a group of compatibility functions. There's no discussion on png-mng-implement. I don't think it's been brought up explicitly before; just odd comments by me when discussing the existing APIs and changes or bugs pretty much along the lines of what I said above. There should be an explicit discussion somewhere about removing APIs and which to remove. |
|
In principle, I would agree to remove anything from the libpng API if it doesn't break any of the PNG-supporting applications out there. Moreover, I would agree to remove parts from the libpng API if they impose complications, even if they do break some PNG applications that can be fixed, as long as those PNG applications do indeed get fixed. |
Uh huh, but you can't make that a precondition because many many of the libpng supporting applications aren't FOSS; we can't even see the source code. The principal problem is https://www.w3.org/TR/png-3/#13Alpha-channel-processing However if a hypothetical app calls Currently the only solution I can see is to require No apps will fail but the behavior, the output image, will change if those apps rely on libpng to handle It won't remove any code it will just remove some of the current failure conditions. The double-gamma-correction bug would still be there (the one that causes the |
|
@LucasChollet I too was working on adding CICP support to PNG and came across your PR. I have raised PR #626 for adding read/write support for mDCv and cLLi chunks in PNG, which is similar to the code changes for cICP. So, I wanted to know when are you planning to merge the cICP support into the main linpng16. |
|
@Miloni2010: this PR has already been merged into libpng18. I don't know why it is still open however @LucasChollet did express a desire to do more work on it and that hasn't happened yet. The PNG v3 changes cannot be released until PNG v3 itself is released; the example of changing the other two chunks to "unsafe-to-copy" is a perfect example of why. So libpng18 release is effectively blocked until the full ISO process has run its course, i.e. until the current proposal (or, more likely, a modified version) becomes a "REC". There's missing discussion about how to handle the colorspacesync stuff. The syncrhonisation is a requirement generated by the fact that the read code does not have access to the png_info (can there be two?) used when reading the pre-IDAT information. In general a png_colorspace should only contain information which is actually required to do the line decoding. libpng does not have any support (at present) for "tone mapping" so none of the three new chunks are required. The only issue is that iCCP itself zaps the colorspace at least according to the current specification. |
|
Ehhh I would flip the order of operations on that. For normal users, PNG v3 things shouldn't be available. I agree. I think the solution would be a compiler flag to opt into "beta". |
That's something entirely new in the discussion. I've already tried one possible implementation where an explicit call to png_get_fFOO was required to flip on the handling of the fFOO chunk. It sort of looked like it might work but was way too much development work; notice that Cosmin has rejected most of my bug fixes, the only stuff going in is pretty much build system fixes and, apparently, the PNG v3 stuff... However:
That's completely supported. The handling for each chunk can be turned on and off. But why the complexity? It just works with libpng 1.8 - people can build and install the alpha today. All we need is for the W3C to walk the spec through the process and for satisfactory implementations to be provided. Until that happens any amount of dev work can be done in the alpha, subject to Cosmin's approval of course. He did check in a cICP implementation which, as the comments above show, is still a work-in-progress. The same can be done with the metadata chunks. This is a standard, universally (I hope) understood development methodology; all bets are off until the latest version is actually released! Everyone (I hope) understands all of this. Anyone can build and install libpng 1.8 on their fave OSD and experiment and it can be done as a regular DLL alongside libpng 1.6 (or, indeed, any other libpng). When it gets released it coexists with the "old" version until developers do any work they need. Why reinvent the wheel? |
|
Ah. I think I follow. If someone is building their own libpng they can use the 1.8 branch to get "beta" features. |
There was a problem hiding this comment.
We should add 99, 73, 67, 80, '\0', /* cICP */ in line 1421.
Also while setting the values in png_set_cICP, shouldn't we check for negative values too along with checking png_ptr or info_ptr in NULL-
if(colour_primaries < 0 || transfer_function < 0 || matrix_coefficients < 0 || video_full_range_flag < 0)
return;
There was a problem hiding this comment.
Yes. That's a bug. After line 1407 I believe (the list is sorted alphabetically) - are you up-to-date with the HEAD of libpng-1.8?
There was a problem hiding this comment.
Here in line 290, we should add #if defined(PNG_READ_cICP_SUPPORTED) to png_read_buffer condition.
Shouldn't we check for negative values in png_handle_cICP as below ?
if(colour_primaries < 0 || transfer_function < 0 || matrix_coefficients < 0 || video_full_range_flag < 0)
{
png_chunk_benign_error(png_ptr, "invalid values");
return;
}
There was a problem hiding this comment.
png_read_buffer is not called from png_handle_cICP; it uses a stack allocated buffer. The only things calling png_read_buffer should be those things where the read buffer is of unbounded size (variable length with no maximum) and even then they don't all call png_read_buffer (e.g. png_handle_eXIf). I checked the #if list, it seems correct. png_get_eXIf should probably be calling it too, I can't see why it was done that way (I didn't write it.)
The range checking on chunk parameters should happen in png_set_CHNK, the checking in png_handle_CHNK handles just errors that occur in the stream encoding. Specifically where a value is outside the range of the PNG format. That's not possible for a byte and there is no checking in png_set_cICP because the values are passed in as png_byte.
That is the problem; the parameters should be 'int' as in png_set_IHDR then your observation is correct.
There was a problem hiding this comment.
The same code change as above must be made at two places, we should add the following at line 858
#ifdef PNG_READ_cICP_SUPPORTED
else if (chunk_name == png_cICP)
png_handle_cICP(png_ptr, info_ptr, length);
#endif
We should also add the cICP chunk hex at line 1647:
#ifdef PNG_READ_cICP_SUPPORTED
99, 73, 67, 80, '\0', /* cICP */
#endif
There was a problem hiding this comment.
I don't understand the first comment; the code you suggest adding is already present at line 185 of HEAD in png_read_info. Line 858 is in png_read_end but cICP has to occur before both PLTE and IDAT.
So far as the simplified API is concerned the API has no way to process cICP so it should be skipped, unless someone wants to add the code and a general solution is not possible. That was one of the WIP issues though; if someone wants to add handling for rgb-to-gray and/or the encoding transforms fine. rgb-to-gray is ok but I think attempting any handling of the encoding transform will just result in confusion.
Hello,
This PR adds support for the cICP chunk, which is an addition from the third version of the PNG specification.
I'll happily make requested changes. Let me know what's required.
Friendly ping @ProgramMax, as you might be interested to review.