Add support for reading and writing the cICP chunk

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.

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!

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.

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.

change all the spellings of "colour" to "color"

I forced myself to do that for consistency with the spec, but I'm firing sed right away, let's get rid of it.

and work out how to make png_get_cHRM_XYZ to work.

To clarify, you want a user to be able to call png_set_cICP and then get the corresponding values when calling png_get_cHRM_XYZ afterward?

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.

I'm not sure to get you, it seems pretty well specified AFAICT. Matrix Coefficients and Video Full Range flag are explicitly specified in the PNG spec, but all of them come from this spec ITU-T-H.273. What do you think is lacking?

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

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.

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 "pngget" 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:

The simplest; leave all the work to the app. This is fully supported already by the unknown chunk mechanism. An app can simply look at the result of "png_get_unknown_chunks" and check for cICP and mDCv (required to handle the PQ transfer function in cICP). This is the way APNG is supported in libpng 1.6 (and, most likely, future releases). It makes sense for cICP because parts of cICP - narrow range images and the PQ and HL transfer functions - are incompatible with the current libpng RGB code and the 1.6 APIs. This means that apps will end up having to do the heavy lifting if support is implemented in 1.6 (@ctruta has to comment on this; where and whether this stuff gets implemented.)

The downside to this approach is that there is no "png_handle_cICP" and this is where the chunk order verification happens.

Better support but retaining the potential for a compatible change (an add) to libpng 1.6. This would use the code you have at present with the error checking but still without the full transfer function handling. The error checking would not include invalidation of the other colorspace chunks. Both cICP and mDCv would be ignored until an appropriate API is called by the app; png_get_cICP is the obvious one. After such a call libpng would swap into "cICP support" mode where all the non-working transforms and pngget APIs are disabled; they would png_app_error out if called.

In this approach the app still does the heavy lifting but there is some opportunity to extend support to the cICP settings which do not require video signal processing (i.e. full range images) and do not have transfer functions that cannot be supported with the current fixed point RGB API.

Full support. This requires a major release because the only sensible way of supporting PQ, HL and narrow range images is by exposing a floating point RGB (etc) API as well as changing the transform code internally to be 100% float (or _Float16, if available).

Given those three options my recommendation is to start with a minimalist (2); broadly the code in this PR with a minimalist png_colorspacesync 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:

and work out how to make png_get_cHRM_XYZ to work.

To clarify, you want a user to be able to call png_set_cICP and then get the corresponding values when calling png_get_cHRM_XYZ afterward?

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_colorspaceset functions; see pngpriv.h, try searching through pngrutil.c for the string "png_colorspaceset".

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 pngset 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 pnghandle. The result of calling pngset while reading a file is not defined, I think I might have made it error out in one version of libpng 1.7. Likewise pngget 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 pngget 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 pngcolorspace* 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:

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.

I'm not sure to get you, it seems pretty well specified AFAICT. Matrix Coefficients and Video Full Range flag are explicitly specified in the PNG spec, but all of them come from this spec ITU-T-H.273. What do you think is lacking?

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:

Access : Locked Document restricted to TIES users [ITU-T]

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. Sorry about the delay.

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.

I'm going to review the code first so I am not influenced by the conversation.

Ok then, I won't say what I think about this commit 😜

Going through some of the commentary:

I think it is okay for libpng to not handle the CICP values. The spec deliberately doesn't say the values or which edition of the CICP spec to use. This is so a new CICP spec can be released (adding new color spaces) without the PNG spec needing to be updated to reflect it.

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.

The PNG spec references another spec that previously was free and open. However, that spec is in the process of getting an update. They took down the old spec and block access to the new spec until it is done. Once it is done, it will be free and open. I personally wish they left the old one up but noted a new one was in the works. But oh well. This problem will sort itself out.

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.

Also, I'm curious if we need to care about PNG_COLORSPACE_HAVE_INTENT and the likes

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_colorspaceset...).

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.

Should we: colorspace->flags |= (PNG_COLORSPACE_HAVE_ENDPOINTS|PNG_COLORSPACE_ENDPOINTS_MATCH_sRGB);

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.

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.

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.

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.

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 pngget 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.

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.

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 pngget APIs to not use the chunk name, but why?

I hear what you're saying. But as a counter-example, libpng shouldn't change the color values of the pixels to map it into the color space cICP specifies. Libpng won't know if the user is even capable of wider color spaces. Nor if they're doing any tone mapping or adjusting for viewing conditions (IE bright outside vs. dark room). This is unlike how gAMA and iCCP could behave.

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.

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.

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 pngget 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 invalidation

We 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

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.

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

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 pngget APIs to not use the chunk name, but why

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 handling

Requiring 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.

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 pngget 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...

I think you have the wrong veluca :-) Not sure where you found that comment, but it was originally written in the Chromium source, which bundles libpng - in that context, there is no dynamic linking, and the libpng version is a fixed known one, so it is indeed possible to eventually rely on it.

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)

Not sure where you found that comment, but it was originally written in the Chromium source, which bundles libpng - in that context, there is no dynamic linking, and the libpng version is a fixed known one, so it is indeed possible to eventually rely on it.

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. But after that, I think this might be ready to land.

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. I think all the main issues are resolved, but I'm sure I'll have more questions 😅.

pnggroup / libpng