APNG patch by Daisuke Nishikawa

[peterkaczorowski]

No description provided.

[MaartenBent]

Thanks for the patch. I left some comments, mostly related to generating the config files.

Maybe you can leave a link to the source of the patch as well, both in the PR and in the commit message: https://sourceforge.net/projects/libpng-apng/

[MaartenBent]

I don't think this is part of the patch. It removes our fix for building with Visual Studio 2008. This version is not supported anymore so it is not really a problem to remove this, but it would be better to do it in a separate commit.

[peterkaczorowski]

corrected in commit: 6af7391

[MaartenBent]

Definitions have been removed from this file, which seems wrong. Please use the official method to create this file as described in
https://github.com/wxWidgets/wxWidgets/blob/master/docs/contributing/how-to-update-third-party-library.md#libpng

[peterkaczorowski]

DONE in commit: ab9d40f and 482db7a (I forgot about PNG_ZLIB_VERNUM)

[MaartenBent]

Many defines have been added to this file, which seems wrong. It causes at least one build warning: png.c(2785,9): warning C4005: 'png_fp_add': macro redefinition
Please use the official method to create this file as described in
https://github.com/wxWidgets/wxWidgets/blob/master/docs/contributing/how-to-update-third-party-library.md#libpng

[peterkaczorowski]

DONE in commit: ab9d40f

[peterkaczorowski]

@MaartenBent,

Thank you! I'm going to apply your comments in to forked source during second half of the day.

Best,
Peter
PS: Thank you for your support!

[peterkaczorowski]

APNG patch credits added in commit: 902b882

[peterkaczorowski]

@MaartenBent,

I have made the changes you requested. They were done in several commits. If it's okay, we can move forward. If not, I can fork and create a pull request again. Let me know.

Best,
Peter

[MaartenBent]

Thanks for the fixes. There is no need for a new pull request. When merging the PR, all commits can be squashed into 1.

I only have a remark about 902b882 (APNG patch credits). I did not mean to add an entry to the CHANGES file.
This will surely cause merge conflicts every time we want to update libpng. Because it is inserted in the same location as the new libpng changelog entry.
So please revert this.

I only meant to put a link in the description (first message) of this PR, because currently it is empty: No description provided.. When people look back at this PR they might have no idea what this is and where it comes from.

When squashing/merging this PR, @vadz can probably include the link in the commit message.

[MaartenBent]

And just for reference, APNG patch for libpng on SourceForge is licensed under zlib/libpng License, so there should be no issue having this patch applied here and used in wxWidgets.

[vadz]

Thanks for the PR, it looks nice and tidy (although I definitely didn't check it in details), but my main question is how are we going to maintain this, i.e. update it when we upgrade to the next libpng version or if/when the new version of this patch is made?

More generally, I'm not in a hurry to merge this because now that it's here, you can make a PR to the main repository using it, to show how it is going to be actually used, and this will be the important part. TIA!

[peterkaczorowski]

@MaartenBent,

I did not mean to add an entry to the CHANGES file.

The file has been restored as in the forked version, commit: 0e8b65a

Let me know if there's anything else I need to do, or if I should create wxapng2 and upload the changes with comments again in the correct order as you requested.

[peterkaczorowski]

@vadz

... but my main question is how are we going to maintain this, i.e. update it when we upgrade to the next libpng version or if/when the new version of this patch is made?

Well... That is a very good question. At this point, version 1.6.47 has already been released. I have the impression that these changes are more like small fixes and additions rather than revolutionary modifications. I’d also like to remind you that just a few weeks ago, wxWidgets was running without any issues on the libpng library from 2019.

Currently, I’m fairly engaged, so with your guidance, I could probably take care of this. Maybe it's worth contacting the author of the patch (I can do it) and mentioning the work being done here with wxWidgets. His involvement would definitely help support this effort.

We could establish a setup where wxWidgets can be compiled with either the vanilla libpng or libpng with the APNG patch. In that case, the source tree could have two directories: src/png (wx_ version) and src/apng (wx_ + APNG patch version). Then, wxWidgets could be compiled with -DwxPNG_APNG_SUPPORTED=ON or --enable-apng-support.

[MaartenBent]

Let me know if there's anything else I need to do, or if I should create wxapng2 and upload the changes with comments again in the correct order as you requested.

No, this PR is fine. You can now start adding support for multiple images in wxPNGHandler as suggested in wxWidgets/wxWidgets#25106 (comment). And/or discuss in that issue how exactly apng can be integrated in wxWidgets.

how are we going to maintain this, i.e. update it when we upgrade to the next libpng version or if/when the new version of this patch is made?

Ideally when we upgrade libpng, there will be an apng patch available for that version. But if there is not, the only thing that seems to needs updating are the PNG_EXPORT ordinals.

the source tree could have two directories

There is no need for multiple sources. We can just add a build option to disable apng, and change this patch to:

+#ifdef wxUSE_LIBPNG_WITH_APNG
#define PNG_APNG_SUPPORTED
#define PNG_READ_APNG_SUPPORTED
#define PNG_WRITE_APNG_SUPPORTED
+#endif

[peterkaczorowski]

No, this PR is fine. You can now start adding support for multiple images in wxPNGHandler as suggested in wxWidgets/wxWidgets#25106 (comment). And/or discuss in that issue how exactly apng can be integrated in wxWidgets.

Ok. It should take a 7-day sprint to do it.

[MaartenBent]

Maybe this can be useful for you.
While working on (animated) WebP support (wxWidgets/wxWidgets#25205), I updated the image sample to support stepping through the frames of an animated image (gif, webp) (this commit: wxWidgets/wxWidgets@2d703f2).

It should work for APNG too once you add wxPNGHandler::DoGetImageCount and support the index parameter in wxPNGHandler::LoadFile.

[peterkaczorowski]

Maybe this can be useful for you. While working on (animated) WebP support (wxWidgets/wxWidgets#25205), I updated the image sample to support stepping through the frames of an animated image (gif, webp) (this commit: wxWidgets/wxWidgets@2d703f2).

It should work for APNG too once you add wxPNGHandler::DoGetImageCount and support the index parameter in wxPNGHandler::LoadFile.

👍

Overall, I already reviewed the code two weeks ago, but after two comments from one person, I had to take a breather... I enjoy working in a positive atmosphere. Thanks for the suggestions. I have a few projects here, and as soon as I wrap things up there, I'll sit down and do it.