Warn on unknown chunk handling (instead of err out); add continuous integration - #51
Warn on unknown chunk handling (instead of err out); add continuous integration#51ronaldtse wants to merge 3 commits into
Conversation
…norma/pngcheck-metanorma#1) According to https://www.w3.org/TR/2003/REC-PNG-20031110/ 15.2.3 a) & b), an unknown chunk type is not treated as an error. Contribution from Maxim Samsonov (@maxirmx) of Ribose (@riboseinc/@metanorma)
…ixes pnggroup#50 * Windows Makefiles based on those from Greg Roelofs <newt@pobox.com> and John Bowler <jbowler@acm.org> Contribution from Ronald Tse (@ronaldtse) of Ribose (@riboseinc/@metanorma)
jbowler
left a comment
jbowler
left a comment
There was a problem hiding this comment.
Unknown public chunks should not occur. So far as I am aware all public chunks are now integrated into the pnggroup version of pngcheck and, indeed, the new ones were integrated before PNGv3 became a recommendation.
So this change is wrong; unknown but public chunks are hard errors whether critical or ancillary.
pngcheck is not a PNG decoder, it is a png checker
|
I don't know anything about the github change or the add to .gitignore (it's cmake specific, but there is no CMakeLists.txt in the project...) It seems to me that they are different issues. The core change, however, is not about "unknown" chunks, which were, I believe, correctly handled, it's about unexpected public chunks which should never occur. If there is a public chunk which is not supported it should be added and an issue/PR should identify the public chunk that is missing but this PR does not say which public chunk caused the problem (I do know that there is one :-) |
|
@jbowler currently Apple-created PNGs contain the "iDOT" public chunk which is not defined in the registry. (the iDOT chunk was mentioned in metanorma/pngcheck-metanorma#1 that was linked in #49, which admittedly in hindsight was hard to locate...) Description of the iDOT chunk: This is an example screenshot image I took on macOS to test: 
Before the patch: $ ./pngcheck test-files/sample-screenshot.png
test-files/sample-screenshot.png illegal (unless recently approved) unknown, public chunk iDOT
ERROR: test-files/sample-screenshot.pngIn this PR: $ ./pngcheck test-files/sample-screenshot.png
test-files/sample-screenshot.png illegal (unless recently approved) unknown, public chunk iDOT
WARN: test-files/sample-screenshot.png (494x212, 32-bit RGB+alpha, non-interlaced, static, 94.5%).It has been reported to the W3C PNG group back in 2021, there is no formal resolution yet as of now, but seems that it is being considered: From @ProgramMax :
Happy to remove the patch to pngcheck.c given it is now clear that pngcheck is meant to fail the Apple-created PNGs. Apologies for assuming otherwise. Originally we were using the return code from |
Not sure if this is what you meant but the file does exist...? Lines 1 to 66 in 31362d8 |
|
Closing in favor of #52, which only adds the workflows. |
|
I said:
Ah, Cosmin added it (in March), my mistake I hadn't noticed it for some reason. |
|
iDOT is considered in the current W3C discussions: But releasing PNG files with iDOT in is strictly non-conformant; the files should either never be made public or, better, should be made public along with the specification using a private chunk with a signature, e.g. @svgeesus, @ProgramMax maybe this needs to be stated in bigger letters in the specification? It seems unlikely that iDOT will be adopted verbatim given as there is no public specification; it may be patented/trademarked(difficult with that name) and/or copyrighted. My current working approach is something I call baND which is like maRK type 0 and possibly iDOT, see the link to the reverse engineering here: The difference is that baND is generalised for interlaced images and for APNG frames (interlaced or not). It's also always at the end of the file (just before IEND) so that it can be found easily and reliably. In any case the problem with pngcheck is not that it flags a major error in the file but that it stops parsing at that moment and does not consider the remaining chunks. It's easy to fix a file with iDOT; it can be loaded into an editor like TweakPNG and the chunk deleted or it can be run through a command line program which has the ability to delete specific chunks. |
2 participants
This PR fixes two issues:
Description on #49
pngcheckwas incorrectly treating unknown chunk types as errors, but PNG Specification 15.2.3 a) & b) specifies that unknown chunk types should not be treated as errors unless they are critical.pngcheck.cis modified to properly handle unknown chunk types to warn on non-critical unknown chunks and only err out if it is an unknown and critical chunk.The fix was created by Maxim Samsonov (@maxirmx) of Ribose (@riboseinc).
Description on #50
The current repository did not implement continuous integration and it is hard to ensure that the latest code and build methods work across the multitude of supported platforms.
We have added comprehensive workflows using GitHub Actions that test all supported build methods across multiple platforms, including:
In accordance to INSTALL.md which describes CMake and Makefiles as both accepted/supported methods to build, two workflows are created:
build-cmake.ymltests the CMake approach on all supported platformsbuild-makefile.ymltests the Makefile approach on all supported platformsBoth workflows share a build matrix configuration which is encoded at
matrix.json.Specifically:
The Windows Makefiles made a return (containing edits from @groelofs and @jbowler) to support the Makefile builds
CMake builds use the following setup:
vcpkgintegration with arm64 supportMakefile builds use the following setup:
makenmakewithMakefile.w32makewithMakefile.mingw(32/64-bit)An additional workflow for packaging a release at GitHub Releases is added which is triggered by pushing of a version tag (i.e.
v*).Caveat:
Credits