Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PNGv3 colourspace precedence rules conformance #645

Open
wants to merge 1 commit into
base: libpng16
Choose a base branch
from

Conversation

jbowler
Copy link
Contributor

@jbowler jbowler commented Jan 26, 2025

This is a major change required by the new PNGv3 colour chunk precedence
rules. It does not change the libpng API (png.h) however it changes
the following handling of PNG files:

IFF the PNG file contains colour space information it changes from the
libpng v3 behaviour to the now compulsory PNG v3 behaviour:

  1. libpng no longer invalidates colour space chunks because they are
    inconsistent.
  2. libpng no longer responds to the "png_get_" APIs positively if they
    are not present in the PNG but can be deduced from the colour space
    chunks that are present.

This is a major change required by the new PNGv3 colour chunk precedence
rules.  It **does not** change the libpng API (png.h) however it changes
the following handling of PNG files:

IFF the PNG file contains colour space information it changes from the
libpng v3 behaviour to the now compulsory PNG v3 behaviour:

1) libpng no longer invalidates colour space chunks because they are
   inconsistent.
2) libpng no longer responds to the "png_get_" APIs positively if they
   are not present in the PNG but can be deduced from the colour space
   chunks that are present.
@jbowler
Copy link
Contributor Author

jbowler commented Jan 26, 2025

@ctruta: tests I performed:

  1. Full cmake and configure make {test|check} with compiler options:
    -Wall -Wextra -Wundef -Wwrite-strings -Wpointer-arith -Wmissing-declarations -Wstrict-prototypes -Wmissing-prototypes -Wno-implicit-fallthrough --std=c90 -pedantic
  2. The same build across multiple architectures but without the run of the test programs. Results show some failures but nothing to do with this change:
  1. aarch64: possible bug, nothing to do with this change.
  2. powerpc64: cmake build fails, configure doesn't. Failure to compile the right files, cmake bug.
  3. arm7a-neon: as previously reported (fixed in 1.8)
  1. minconfig: I ran all contrib/conftest/*.dfa on amd64 and resolved all the issues so at least for those test cases minconfig should be good.

This is good to go and needs to go.

@jbowler
Copy link
Contributor Author

jbowler commented Jan 26, 2025

NOTES: These are in the unsquashed commits but then so is a lot of in-out-shake-it-all-about distraction...

The basic strategy I adopted was to eliminate png_colorspace, which only existed to implement the old rules, or more correctly the old lack of rules.

This, as I suggested before, removes rather a lot of LoC. LoC counts before and after, the LoC metric is [;,]; I didn't exclude comments (and I added a lot of course):

metric: cat *.[ch] | tr -cd ';,' | wc -c

results: #645: 24401
1.6.46: 25281

So that's 5% of the code gone. Pretty good for a weeks work IMO.

sub-strategy: introduce a log in png_struct of the chunks in the file; on read this is the chunks read regardless of the png_info where they got stored. On write (NYI) it would just be the chunks written, but there is only one body of write code so the gain is much less there.

Given the log when asked a question, like "what is the gamma dude", simply troll through the chunks in the prescribed manner and deduce it. The code, deliberately, does not feel constrained to deduce the result on the fly, although my last revision (the one here) did do that for the chunks in the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant