@unitarou, @ProgramMax, @svgeesus, @jbowler, @peterkaczorowski:
Please review 😁

Wonderful! A merge with independent APNG support has just been prepared for GiMP, but the team is very willing to prepare a version compatible with the new LIBPNG. I’ll resume work on wxWidgets and my extensions for KiCad.

Great news. Thank you, Cosmin!

Wonderful! A merge with independent APNG support has just been prepared for GiMP, but the team is very willing to prepare a version compatible with the new LIBPNG. I’ll resume work on wxWidgets and my extensions for KiCad.

It's sooo nice to see the enthusiasm, @peterkaczorowski :-)

Mind you, this comes with the important caveat that we are in the earliest alpha stage of libpng v2. This PR is going to be literally the first feature added, and it will still take a while until this brand new libpng thingy becomes ready for prime time.

I added you to the list of reviewers, but please spread the word over to those projects, too: anybody who wants to contribute a review is hereby invited to contribute a review.

@ctruta: this should be checked in already shouldn't it? My understanding of the git development process is that a branch is created for a set of changes ("develop" in this case) and then changes are submitted and checked in. To start the process you have to actually check the changes in to the branch...

@ctruta: The changes use png_structp and png_infop. The rest of libpng uses restrict points (png_structrp and png_inforp). The changes also don't use png_const_structrp and png_const_inforp. The former affects png_get_next_frame_fcTL where the compiler cannot know that the return arguments are not aliases into png_info, the latter affects pretty much everything.

This is a point where consistency is important. Just change all the png_structp and png_infop occurences in the new code to png_const...rp and then back-fix the ones that can't be const.

There's no handling for out-of-order or damaged sequence numbers. Better than what is being done move the sequence number iterator on read into the app space, then it can choose how to handle the error.

It is also better to allow the frames to be saved. As it is an editor can't fix up a damaged APNG using libpng.

In any case png_ensure_sequence_number just calls png_error; that doesn't work! It needs to call png_chunk_benign_error followed or preceded by png_crc_finish otherwise an ancillary chunk will make a PNG completely unreadable by libpng! Think about what happens with an app that doesn't handle APNG. If an APNG has a bogus fcTL before IDAT (sequence number not 0) the whole PNG will be undisplayable; there's no way of skipping the bogus and unused chunk. PNGs that can currently be displayed in 1.6 will suddenly not be displayable in 2.0

I did some tests with clock.png (above, I got it from the link) I didn't apply @MaartenBent's patch but the programs don't call the new APIs. These programs work fine when linked against 1.6.49 but both pngcp and pngimage now error out with:

clock.png: error(libpng): write: Not enough frames written

This is because the acTL is being read into the png_info but the fcTL and fdAT chunks are, apparently, never read and neither are they added to the unknown chunks (at least in the pngcp case.) Basically two programs that used to work with 1.6.49 now actually error out (leaving a truncated PNG) in 2.0.0.

It's standard, and unfortunate, that adding "known" chunks breaks programs that rely on the unknown handling; the chunks are "known" by libpng but not the app so apps that use png_info from read to write a PNG can end up doing the wrong thing with the new chunks.

pngcrush correctly removes the "unsafe to copy" acTL along with all the fcTL and fdAT chunks.

I strongly recommend disabling APNG handling by default. It's no significant work for an app to call an "enable APNG" API (e.g. png_set_option) since it has to call all the complex "next_frame" APIs and allocate buffers for the frames anyway. If you don't like that it could be enabled automagically when the app calls png_get_acTL but that's a little more complicated internally.

CDfcTL in pngrutil.c is wrong (not a result of this change): "25U" should be "26U"; it's a typo and it is in 1.6 as well. The "handle" functions for those two chunks should just invoke the "unknown" handling; this will discard them by default. This will avoid the warning/error if the sequence number is wrong; if the chunks are being skipped why issue a stream of warnings? I've already made the point out png_error'ing out on an ancillary chunk which is not used.

I hacked the first fcTL in "clock.png" to have an invalid sequence number (1) then tried pngcrush again; remember pngcrush correctly removes acTL, fcTL and fdAT. pngcrush errors out with "Out-of-order sequence number in fcTL or fdAT".

This is a major bug; it's an ancillary chunk and the app isn't going to use it. Preventing it being read by, I think, any app that doesn't have special handling for the APNG chunks is wrong. It's a png_chunk_benign_error, just like having a bad length hIST or, indeed, tRNS.

AND Chrome displays it correctly (as a static image). Just use TweakPNG to change the first fcTL sequence number (I don't know if clock.png is copyright so I won't post the hacked version.)

BUG (serious): the chunks_to_ignore table in pngset.c (png_set_keep_unknown_chunks) needs to be updated to include the new known chunks, acTL, fcTL and fdAT. See the comment above the table: This table must be updated every time a new ancillary chunk is added.

Fixing this will probably unbork a number of apps that would otherwise be borked. I'm working on a separate PR for pngcp that will, necessarily, include the change but it can be done anyway by anyone; the table is stored alphabetically.

#710 fixes both the pngcp handling of the Apple banding chunk (this was wrong in 1.6 as well) and the handling of APNG when #706 is applied. The contrib/tools/pngcp.c fix can be applied to 1.6 (with adjustment for the different C dialect), the pngset.c change is not required though it is harmless (APNG chunks are not "known" in 1.6).

The pngset.c change is required iff APNG support is added to 2.0 but is harmless regardless; every known chunk must be added to the list in pngset.c!

Spurious and confusing code

In pngread.c an "else if" mess has been added for acTL, fcTL and fdAT. This should not have been done because the code falls through to png_handle_chunk at the end (the final else clause, immediately after the if-else mess) and this has entries for acTL, fcTL and fdAT already; these (table entries) can no longer be reached.

In pngpread.c the same thing happens for png_handle_{acTL,fcTL} but the code is written in a way which is just plain confusing; instead of using the macro PNG_PUSH_SAVE_BUFFER_IF_FULL like everything else in the relevant function it hand expands it inline in the code!

This is just plain confusing, even more so because png_handle_{acTL,fcTL and fdAT} are defined in pngrutil.c but not put in the table.

Note that is no other png_handle_ function in the whole of libpng that is not static in pngrutil.c; they existed at one point in 1.6 but they have all been eliminated now. Reintroducing such functions with a different type is really confusing.

fdAT handling needs to be consistent with IDAT. There are only two things that can happen; process it in the same way as IDAT or treat it as unknown. Hand crafting a completely new way of doing this doesn't make any sense to me; it's already done for IDAT so do it the same way!

acTL and fcTL should not need special treatment. Repeating checks in png_handle_{acTL,fcTL} that are already in png_handle_chunk is unnecessary and confusing code duplication.

Test case: Take "clock.png" and use TweakPNG to add a tEXt chunk before the third frame fcTL (sequence number 3, the second fcTL after IDAT). The resultant APNG animates fine with Chrome and is to spec, however the code around 4406 of pngrutil.c "ignores" the chunk and skips it, with a warning which should certainly not be there because the APNG is valid.

Those chunks need to be png_handle_chunked so that they can be put into a png_info. Since the code sequence (png_read_IDAT_data) can only be reached from png_read_row and that is an API call some way is needed to get a png_info in for stuff encountered between frames.

cf my comment elsewhere on the KiCAD issue of needing labels on frames. I don't think I suggested anything but I'm thinking:

laBL <sequence number> <UTF8-text>

So <sequence number> matches the fcTL and, ideally, laBL is written directly before the corresponding fcTL. The same issue exists with respect to "banding" of frames. Having the encoder build a complete banding table with file offsets for not just rows in IDAT but every frame is unreasonable; it's better to allow baND chunks before each fcTL as they are unlikely to be needed until the previous frames have been decoded (though I have other ways round this).

Anyway, here are the conditions for the code:

APNG is designed to allow incremental display of frames before the entire datastream has been read. This implies that some errors may not be detected until partway through the animation. It is strongly recommended that when any error is encountered decoders should discard all subsequent frames, stop the animation, and revert to displaying the static image. A decoder which detects an error before the animation has started should display the static image. An error message may be displayed to the user if appropriate.

Decoders shall treat out-of-order APNG chunks as an error. APNG-aware PNG editors should restore them to correct order, using the sequence numbers.

Three of those recommendations are simply unimplementable with the current PR:

A decoder which detects an error before the animation has started should display the static image.

So my bogus fcTL before IDAT must be fixed.

It is strongly recommended that when any error is encountered decoders should discard all subsequent frames, stop the animation, and revert to displaying the static image.

So png_error is simply unacceptable.

APNG-aware PNG editors should restore them to correct order, using the sequence numbers.

So an editor needs to be able to extract out-of-order frames. While this can be done in the manner I demonstrated with AREATK that approach does not handle decoding of individual frames; the editor would have to write a new APNG first, possibly dropping incomplete frames.

The AREATK solution was to read the whole APNG then just get the APNG chunks in order. That would work but it has to load the whole APNG first. An alternative is to buffer fcTL/fdAT in the unknown list then, when the next fcTL is found start decoding of the previous frame. If the fdAT are incomplete by the end of the APNG then the frame could be discarded or played with, most likely, only part of it rendering as the app preferred.

I've merged all the PRs I've submitted into:

https://github.com/jbowler/libpng/tree/libpng2-apng

I added @MaartenBent 's fix for png_read_reinit when png_read_update_info is not called along with the fix for png_get_rowbytes in the contrary case where it is called.

I reviewed the png_get_ functions, the png_get_next_frame... accessors are pure code bloat; they aren't necessary because they just duplicate the code in png_get_next_frame_fcTL and the caller needs all of the data sometime. Nothing else in PNG does this; the non-chunk accessors pull data out of png_struct, not png_info and it is weirdly confusing to have undocumented API duplication like this; which one should be used?

png_set_... these should be (void) functions and need to call png_chunk_report. The error messages are inconsistently wordy; libpng lacks any i18n and the checks need to be concise and understandable because of that (libpng1 is very inconsistent, I'm trying to fix that!)

Bear in mind that these checks are often simply blurted out to the application user so, for example, "Ignoring wasteful fcTL BLEND_OP_OVER in opaque images" is not going to be useful. Also that warning is erroneous; does libpng issue a warning if the filter on the first line of a pass is UP? Only NONE, SUB and AVG are useful on the first line, but it is not appropriate for the code which read a PNG to output diagnostics about the unknown code which wrote the PNG!

The optimisation is also incorrect; see png_set_next_frame_fcTL and note that it is checking info_ptr for tRNS. That doesn't work with two png_infos and possibly not with png_set_expand_tRNS (I think it might but that would be an infelicity, or, arguably, a bug.)

There's a major bug in the new code to clear flags in png_struct::mode; that cannot happen and just does not work. It changes the whole way the parser (both progressive and sequential) works and only seems to have been done to implement png_handle_fcTL and reuse the row reading code.

Clearly the row reading code can be changed to use different flags and I think it should be changed in that way: the design of "mode" is to store information about the position within the PNG stream WRT critical chunks, these are not critical chunks so removing the "AFTER_IDAT" flag is going to, as a minimum, mess up tests for chunks which must occur before IDAT. What happens if I add a cICP after IDAT in an APNG? That's not valid because of the positioning rules, but those check for !(png_struct::mode & PNG_HAVE_IDAT) In fact I think IEND will always fail.

@jbowler wrote:

@MaartenBent I think your patch is good but it's clear that at least one quarter of the common cases used by application writers haven't been tested by either you or @ctruta (or other APNG developers).

John's assumption is correct. (Speaking for myself, about myself, to be clear.) Under the <air-quotes>good excuse</air-quotes> that we're in the development branch, right at the start of libpng-2, and that we have a lot of things to test and a long way to go, I'm landing this in, including the suggested changes.

On the other hand, as far as I know, this APNG patch has been long in use in various APNG-supporting software, most notably in Mozilla. For that reason, I don't consider it to be bleeding-edge-untested, either.

FYI, the CI verifications for the master branch (which stands for libpng-1.6.x) and for the develop branch (which stands for libpng-2.x) are here:
https://ci.appveyor.com/project/ctruta/libpng/

Thank you everyone for your participation :-)

@svgeesus @ctruta The GIMP approach (in the development branch, 3.1.2) is the same as I have suggested before but (unlike AREATK) it is stateful so it uses the "unknown chunk callback" to store the frames as the PNG is read. It's clean and a good example of how to do it. It's a simple modification of the main GIMP PNG read code in 3.0.4, Max Stepin is quoted as the reference for the APNG implementation.

The patch appears to be from the sourceforge web site. There is also a version for GIMP with is covered by GPL-2/GPL-3

The patch itself does not contain a copyright; SourceForge lists the license as "zlib/libpng" but that's not in the patch anywhere either.

That aside I looked at the original patch (from sourceforge) applies to 1.6. The way it works is a bit of a problem because it seems certain it will break GIMP. At one point it was known to cause problems for Chromium as well. This comes back to what I've said before about adding new "known" chunks however I'm pretty sure that this particular breakage can be avoided.

@jbowler

The GIMP team started working on APNG support before @ctruta made his code public. GIMP 3.1.2 is part of the development branch, so I believe that APNG support will soon be unified and aligned with the PNG team's prior consultations.

@peterkaczorowski this is the code:

https://sourceforge.net/p/libpng-apng/code/ci/master/tree/

It's been available since 1.2.29, possibly earlier elsewhere. It's the code in the NetScape/Mozilla browser, I think. It's available on many systems. The problem is it causes problems to some existing code, not least Chromium but it's pretty much always been there.

Better, here is the code in git:

https://github.com/mozilla-firefox/firefox/tree/main/media/libpng

The authors still aren't there for sure because the log shows changes that are apparently part of the changes in a new release checked in separately via a patch, however this file would seem to be authorative:

https://github.com/mozilla-firefox/firefox/blob/main/media/libpng/MOZCHANGES

This modified version of libpng code adds animated PNG support and is
released under the same license as the upstream library. The modifications
are Copyright (c) 2006-2007 Andrew Smith, Copyright (c) 2008-2017 Max Stepin,
and are delimited by "#ifdef PNG_APNG_SUPPORTED / #endif" directives
surrounding them in the modified libpng source files.

@jbowler wrote:

https://sourceforge.net/p/libpng-apng/code/ci/master/tree/

For the record: this is what I applied.

https://sourceforge.net/projects/libpng-apng/files/libpng16/1.6.49/

I started with a temporary application to my local libpng16 branch. Then I did a cherry-pick with an obligatory rework onto the develop branch. And then, finally, the subsequent cleanup commit, and the fixes that everybody else contributed. I credited @unitarou (Daisuke Nishikawa) in the AUTHORS file.

Better, here is the code in git:

https://github.com/mozilla-firefox/firefox/tree/main/media/libpng

The authors still aren't there for sure because the log shows changes that are apparently part of the changes in a new release checked in separately via a patch, however this file would seem to be authorative:

https://github.com/mozilla-firefox/firefox/blob/main/media/libpng/MOZCHANGES

Thank you for pointing it out. The AUTHORS file must be updated for sure.

@ctruta wrote:

I started with a temporary application to my local libpng16 branch. Then I did a cherry-pick with an obligatory rework >onto the develop branch. And then, finally, the subsequent cleanup commit, and the fixes that everybody else >contributed. I credited @unitarou (Daisuke Nishikawa) in the AUTHORS file.

Hello!
I'm APNG-patch for libpng author on sourceforge. I just created account @unshumikan on github.
@unitarou is another person with the same name. It is common name in Japan.

I'm happy it's merged.

Arigatō, @unshumikan 👍
Will ping you for the future developments.