Add eXIf support to push mode

[ProgramMax]

libpng already supports eXIf as of v1.6.31. However, it seems like support was added for normal mode and not added to push mode.

Notice PNG_READ_eXIf_SUPPORTED is in pngread.c: https://github.com/pnggroup/libpng/blob/libpng16/pngread.c#L178 but is missing from pngpread.c: https://github.com/pnggroup/libpng/blob/libpng16/pngpread.c#L274

This commit adds eXIf support to push mode.

[ProgramMax]

Looking at other chunks near by, some use png_ptr->chunk_name and others use chunk_name. But given that "chunk_name = png_ptr->chunk_name;" above, I don't think it'll matter much.

As far as placement, I wanted to follow the existing eXIf handling code. Here, you'll see it follows cHRM handling: https://github.com/pnggroup/libpng/blob/libpng16/pngread.c#L178 And here, too: https://github.com/pnggroup/libpng/blob/libpng16/pngread.c#L855

[jbowler]

Yes. My choice of order was not determined by any great merit. My tests with libpng eXIf handling completely disabled have run their course and demonstrated that nothing I have uses eXIf... More details are in https://github.com/pnggroup/libpng/issues/534

So in those 34 packages there is not a single call to the libpng eXIf APIs, we may get more information from bigger builds.

I don't think there is any issue with the WG hacking and chopping as desired.

So far as the libpng code is concerned it really is a switch statement hidden in if/then/else. A good compiler might determine that. The issue with the mixed use of what should be a single local const is that there are few good compilers on the block; a hard man or whatever. The code turns into a hash table lookup; it's slightly slower than the equivalent hard compiler rewrite but it is more maintainable IMO.

[jbowler]

@ctruta: I suggest that it is appropriate to merge this unless there is some reason you know of why enabling eXIf handling in the progressive reader will cause problems...

[ProgramMax]

FWIW, the PNG WG had marked eXIf support as 'at risk' because only Safari supported it. I made a Chromium patch to solve the eXIf 'at risk' problem: https://chromium-review.googlesource.com/c/chromium/src/+/5249880?checksRunsSelected=Chromium%20Binary%20Size,linux-rel That is what lead me to the missing eXIf support in pngpread.c.

I say that because I think it shouldn't cause problems. The Chromium patch seems fine and passes its tests.

[jbowler]

Some test images would help :-) I notice that libpng will sometimes rewrite an image with the eXIf moved to be after the PLTE and tRNS but before the IDAT. This isn't guaranteed, it's just the way the code is written at present, it happens if the entire image is read (e.g. png_read_png) and only then written.

The simplified API just drops the information; it's not intended for use in a "PNG editor", rather in an image processing application and so it only returns the image converted to sRGB/linear as requested or flagged as not convertible.

[ctruta]

I am absolutely not the one to be in the way of supporting EXIF (or anything else that's related to PNG-3), but I do want to tell you that the push mode has been ostensibly left to bitrot and is a candidate for removal from the still-elusive "libpng-ng".

The so-called "push reader" was added quite early to libpng, either by Mozilla or at least for the benefit of Mozilla, and it was later adopted by others, including WebKit and Chromium. However, nowadays, none of these Big Three (or Big Four, if one insists on counting Microsoft Edge separately from Chromium) is using it, to the best of my knowledge.

Also, nobody came forward with proposals like "my app is using the libpng progressive reader so please update it, here's my PR".

While I want to state my intention to NOT break the world with API removals from libpng, I also want to give a try to the removal of the progressive reader, in the hope that "Nobody Cares (tm)".

[jbowler]

That's interesting because I regard the progressive reader as architecturally preferable to the sequential reader; while it's possible to work round the blocking reads in the sequential reader using coroutines or MT that's a lot of app-side work.

[ProgramMax]

I think I'm mixing up terminology. Chromium-based browsers use pngpread.c. That extra "p" is for progressive, not push, right? (The whole file is wrapped in #ifdef PNG_PROGRESSIVE_READ_SUPPORTED)

I think I mixed up that this file is for push mode.

Whatever the case, pngpread.c seems to be missing eXIf support. And Chromium-based browsers use pngpread.c.

[ctruta]

That's interesting because I regard the progressive reader as architecturally preferable to the sequential reader; while it's possible to work round the blocking reads in the sequential reader using coroutines or MT that's a lot of app-side work.

Ok @jbowler, but I though that your own png_image does the same, except better. From what I've seen in code from web browsers, png_image is what they use nowadays.

Here's the thing: we have three readers in libpng, which is <conjecture>more than actually needed</conjecture>.

[ProgramMax]

I should add: The link I gave in my comment above is why I created this pull request. In order to add eXIf support to Chromium-based browsers, I had to add support in pngpread.c. I wanted to upstream those changes rather than have them be a Chromium-only change.

So if you're considering removing pngpread.c, I am confident that Chromium would care. :)

The Chromium project recently tested moving to Wuffs for its image decoders. I worked with Nigel (the Wuffs creator) a bit at Google and was excited for this move. But it was abandoned because it was difficult to get buy-in on a change that was perceived as scary. People in Chromium rightfully feel that image decoders are a large attack vector and don't want to rock the boat with so many user's security.

[ctruta]

Whatever the case, pngpread.c seems to be missing eXIf support. And Chromium-based browsers use pngpread.c.

Ok, Chris, you know what? I haven't stared at Chromium code in years, and now is the time to refresh my memory. Please forgive and forget what I wrote before. I'll get back to you.

[ProgramMax]

No worries. Happy to work with you on this. I'm semi-familiar with Chromium code and happy to help you with it. Likewise, I would appreciate your help learning libpng code.

A handy tool for navigating Chromium's code is to use their code search: https://source.chromium.org/chromium

Here is where you'll find the PNG decoder Chromium uses: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/png/?q=image-decoders%2Fpng&ss=chromium

[ProgramMax]

(I also agree with the general feeling that 3 readers is too many, and would be happy to work alongside libpng to migrate Chromium as needed. But like I mentioned, that might be a difficult one to get buy-in.)

[jbowler]

Ok @jbowler, but I though that your own png_image does the same, except better. From what I've seen in code from web browsers, png_image is what they use nowadays.

There are only two actual readers: pngread.c (the sequential reader, which does IO through a callback which may not fail) and pngpread.c, the progressive reader, which doesn't do IO; it gets called repeatedly by the app with more data each time.

"png_image" (the simplified API) and png_read_png call the sequential read code (so they block on FILE* IO).

I don't see how web browsers can use the png_image interface if APNG is supported unless the patch changes the png_image read code to allow have APNG IDAT handling (fDAT?)

Here's the thing: we have three readers in libpng, which is <conjecture>more than actually needed</conjecture>.

There are four separate APIs; png_read_image, png_read_png, the sequential read code and, separately, the progressive read code. The two high level APIs could be swapped to the progressive read code and the sequential read APIs could be implemented using the progressive reader, but not the other way round. (That code is in my libpng 1.7, somewhere.)

[ProgramMax]

Oh, another link: https://source.chromium.org/chromium/chromium/src/+/main:third_party/libpng/?q=third_party%2Flibpng

The PNG decoder link I gave above is just the wrapper Chromium puts around libpng. Everything inside third_party/ is an external library Chromium uses. This link is to libpng. While inside third_party/, you'll find README.chromium which lists what library version is used and what changes were made to it.

[jbowler]

The APNG patch is still at 1.6.40, it's here along with .zip files of the modified libpng:

https://sourceforge.net/projects/apng/

[jbowler]

On my system these libraries also use the progressive reader:

1. The "gdk-pixbuf" X11 library
2. The "gst-plugins-libpng" media plugin.

[LeonScroggins]

Android also uses the progressive reader.

[ProgramMax]

For those who don't know, @LeonScroggins above is the OWNER (Chromium terminology for able to approve code changes in an area / responsible for) for Chromium's copy of libpng and Chromium's image decoders. And I guess also Android.

[jbowler]

qtwebengine also uses the progressive reader because of SkPngCodec.

I'm going to open an issue for this because the discussion here is valuable and shouldn't be lost when this pull request is closed.

[ctruta]

In the light of the recent demonstration of me being unable to tell the difference between Chromium and Gecko, I stand corrected.

The commit is in, even if GitHub doesn't seem to know it. Thank you @ProgramMax