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

PNG v3: Support for mDCV and cLLI #635

Merged
merged 2 commits into from
Jan 8, 2025
Merged

Conversation

jbowler
Copy link
Contributor

@jbowler jbowler commented Jan 4, 2025

This adds APIs to get/set the two remaining new PNG-v3 colour space
chunks. The mDCV API matches that of cHRM. Both chunks support
floating point APIs (all values in the two chunks are real numbers).

Both chunk have a new encoded type, a 4dp fixed point number, which
cannot be represented in the existing png_fixed_point type so a
png_uint_32 is used.

Test examples for cICP, cLLI and mDCV are now in 'pngtest.png' and a
necessary change to the pngunknown test has been made to accomodate the
additions.

Signed-off-by: John Bowler [email protected]

This adds APIs to get/set the two remaining new PNG-v3 colour space
chunks.  The mDCV API matches that of cHRM.  Both chunks support
floating point APIs (all values in the two chunks are real numbers).

Both chunk have a new encoded type, a 4dp fixed point number, which
cannot be represented in the existing png_fixed_point type so a
png_uint_32 is used.

Test examples for cICP, cLLI and mDCV are now in 'pngtest.png' and a
necessary change to the pngunknown test has been made to accomodate the
additions.

Signed-off-by: John Bowler <[email protected]>
@jbowler
Copy link
Contributor Author

jbowler commented Jan 4, 2025

This should apply ok to libpng18 too. As the CI message says the API for mDCV matches that for cHRM. This is essential because if a PNG with just gAMA+cHRM is converted to cICP using a different encoding, e.g. converting sRGB to REC 2020 (any variant) the cHRM chunk provides the values for mDCV. It would be error prone, very inconvenient and totally unnecessary if apps had to convert from one chromaticity encoding to another. Fortunately mDCV is a subset of cHRM so the use of the cHRM data types (in particular png_fixed_point) merely requires range checks in png_set_mDCV_fixed (range checks where are necessary for floating point anyway.)

Tested with the new pngtest.png which revealed that pngunknown needs to be updated for new chunks too...

Apart from the png.h change these files are machine generated.

Signed-off-by: John Bowler <[email protected]>
Copy link
Member

@ctruta ctruta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks, @jbowler. But before we move on, here are my off-topic post-review comments.

@@ -3346,7 +3346,7 @@ PNG_EXPORT(244, int, png_set_option, (png_structrp png_ptr, int option,
* one to use is one more than this.)
*/
#ifdef PNG_EXPORT_LAST_ORDINAL
PNG_EXPORT_LAST_ORDINAL(251);
PNG_EXPORT_LAST_ORDINAL(259);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we'll git-merge all this into the libpng18 branch as planned, but beyond that, these ordinals gotta go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PNG_EXPORT_LAST_ORDINAL(259);
PNG_EXPORT_LAST_ORDINAL(259);

At least the script (only in the configure build) that checks the ordinals then faults the build unless the machine-generated files have been updated too.

1.8; a simple change that is unlikely to cause merge conflicts against 1.6 since I believe it's only a few lines. Remove the symbol-by-ordinal support. Throw that PoS against the wall and see what breaks. But don't make any other change to those damned macros because if it has to be reintroduced the result would be a nightmare bug farm.

Completely off topic: the same applies to pnglibconf.h.prebuilt, but generate that in the tarball for the "approved" Linux config. Just don't check it in autoconf or cmake builds that use it.

I think you said this years ago, but if it wasn't you it wasn't me or Glenn either yet it is still correct: The machine generated files should not be in the Git.

png_get_mDCV @256
png_get_mDCV_fixed @257
png_set_mDCV @258
png_set_mDCV_fixed @259
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... this @259 and all of its friends must be no more by the time we publish libpng-1.8.0.

@ctruta ctruta closed this Jan 8, 2025
@ctruta ctruta reopened this Jan 8, 2025
@ctruta ctruta merged commit f753baa into pnggroup:libpng16 Jan 8, 2025
2 checks passed
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.

2 participants