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.

Sign up for GitHub

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

Jump to bottom

Add support for reading and writing the cICP chunk #565

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

Add support for reading and writing the cICP chunk #565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -812,6 +812,10 @@ if(PNG_TESTS AND PNG_SHARED)
COMMAND pngtest
FILES "${PNGTEST_PNG}")

png_add_test(NAME pngtest-cicp
COMMAND pngtest
FILES "${CMAKE_CURRENT_SOURCE_DIR}/cicp-display-p3_reencoded.png")

add_executable(pngvalid ${pngvalid_sources})
target_link_libraries(pngvalid PRIVATE png_shared)

Expand Down
Binary file added cicp-display-p3_reencoded.png
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

The file is invalid and should fail; cICP is following PLTE. See table 6 in PNG-3. I suggest you just add an sRGB cICP to pngtest.png (in the correct place; run pngtest a couple of times to sort pngout.png).

Gwenview reports this error (using the current, unpatched, libpng):

"Gwenview cannot apply color profile on QImage::Format_Indexed8 images."

I'm guessing Qt6 already has support for cICP but I'll investigate.

UPDATE: I see what happened. I hadn't realized you had run it through pngtest (I should have known given that I know pngtest fails on chunk-reorder!) Your call to png_write_cICP is after the call to png_write_info_before_PLTE. I moved it to line 140 (i.e. inside the PNG_COLORSPACE_SUPPORTED, after png_write_gAMA_fixed) and that fixed the problem (since the read code doesn't notice the original error).

You can fix the file easily with TweakPNG (which preserves but is capable of altering the order).

FYI the three individual png_write_info_... calls can be called with three different png_info structures as can the three sequential png_read_info_ APIs but maybe not the progressive reader. Doing that (using three png_infos) is the only way to control where chunks which may appear in multiple locations appear.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
14 changes: 13 additions & 1 deletion png.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,7 @@ typedef png_unknown_chunk * * png_unknown_chunkpp;
#define PNG_INFO_sCAL 0x4000U /* ESR, 1.0.6 */
#define PNG_INFO_IDAT 0x8000U /* ESR, 1.0.6 */
#define PNG_INFO_eXIf 0x10000U /* GR-P, 1.6.31 */
#define PNG_INFO_cICP 0x20000U

/* This is used for the transformation routines, as some of them
* change these values for the row. It also should enable using
Expand Down Expand Up @@ -1974,6 +1975,17 @@ PNG_FIXED_EXPORT(233, void, png_set_cHRM_XYZ_fixed, (png_const_structrp png_ptr,
png_fixed_point int_blue_Z))
#endif

#ifdef PNG_cICP_SUPPORTED
PNG_EXPORT(250, png_uint_32, png_get_cICP, (png_const_structrp png_ptr,
png_inforp info_ptr, png_bytep colour_primaries,
png_bytep transfer_function, png_bytep matrix_coefficients,
png_bytep video_full_range_flag));
PNG_EXPORT(251, void, png_set_cICP, (png_const_structrp png_ptr,
png_inforp info_ptr, png_byte colour_primaries,
png_byte transfer_function, png_byte matrix_coefficients,
png_byte video_full_range_flag));
#endif

#ifdef PNG_eXIf_SUPPORTED
PNG_EXPORT(246, png_uint_32, png_get_eXIf, (png_const_structrp png_ptr,
png_inforp info_ptr, png_bytep *exif));
Expand Down Expand Up @@ -3238,7 +3250,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(249);
PNG_EXPORT_LAST_ORDINAL(251);
#endif

#ifdef __cplusplus
Expand Down
25 changes: 25 additions & 0 deletions pngget.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -785,6 +785,31 @@ png_get_sPLT(png_const_structrp png_ptr, png_inforp info_ptr,
}
#endif

#ifdef PNG_cICP_SUPPORTED
png_uint_32 PNGAPI
png_get_cICP(png_const_structrp png_ptr,
png_inforp info_ptr, png_bytep colour_primaries,
png_bytep transfer_function, png_bytep matrix_coefficients,
png_bytep video_full_range_flag)
{
png_debug1(1, "in %s retrieval function", "cICP");
Copy link
Contributor

Choose a reason for hiding this comment

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

Not an important comment. Just an interesting observation that I may not have considered before:
All the common "in %s retrieval function" strings are probably collapsed into one literal. So this ends up saving some space at the cost of a bit of debug run-time.

An interesting trade off.

Copy link
Contributor

Choose a reason for hiding this comment

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

That stuff isn't compiled in; see pngdebug.h It's not supported, or, more accurately, it's maintainer only and pulls in unexpected stuff. I've never even tried to enable it! Compilers use string table compaction algorithms. The one I know was implemented as a unix command based on finding common trailing substrings. Use "readelf -S .libs/libpng16.so.16.44.0" to see the section sizes; the bad news is the .text segment (i.e. the code) which is close to 1/4MByte, the string table is 12532 bytes (this is on a 64-bit Intel/AMD system).


if (png_ptr != NULL && info_ptr != NULL &&
(info_ptr->valid & PNG_INFO_cICP) != 0 &&
colour_primaries != NULL && transfer_function != NULL &&
matrix_coefficients != NULL && video_full_range_flag != NULL)
{
*colour_primaries = info_ptr->cicp_colour_primaries;
*transfer_function = info_ptr->cicp_transfer_function;
*matrix_coefficients = info_ptr->cicp_matrix_coefficients;
*video_full_range_flag = info_ptr->cicp_video_full_range_flag;
return (PNG_INFO_cICP);
}

return (0);
}
#endif

#ifdef PNG_eXIf_SUPPORTED
png_uint_32 PNGAPI
png_get_eXIf(png_const_structrp png_ptr, png_inforp info_ptr,
Expand Down
8 changes: 8 additions & 0 deletions pnginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,14 @@ struct png_info_def
png_colorspace colorspace;
#endif

#ifdef PNG_cICP_SUPPORTED
/* cICP chunk data */
png_byte cicp_colour_primaries;
png_byte cicp_transfer_function;
png_byte cicp_matrix_coefficients;
png_byte cicp_video_full_range_flag;
#endif

#ifdef PNG_iCCP_SUPPORTED
/* iCCP chunk data. */
png_charp iccp_name; /* profile name */
Expand Down
8 changes: 8 additions & 0 deletions pngpread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,14 @@ png_push_read_chunk(png_structrp png_ptr, png_inforp info_ptr)
png_handle_cHRM(png_ptr, info_ptr, png_ptr->push_length);
}

#endif
#ifdef PNG_READ_cICP_SUPPORTED
else if (png_ptr->chunk_name == png_cICP)
{
PNG_PUSH_SAVE_BUFFER_IF_FULL
png_handle_cICP(png_ptr, info_ptr, png_ptr->push_length);
}

#endif
#ifdef PNG_READ_eXIf_SUPPORTED
else if (png_ptr->chunk_name == png_eXIf)
Expand Down
12 changes: 12 additions & 0 deletions pngpriv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,6 +876,7 @@
#define png_PLTE PNG_U32( 80, 76, 84, 69)
#define png_bKGD PNG_U32( 98, 75, 71, 68)
#define png_cHRM PNG_U32( 99, 72, 82, 77)
#define png_cICP PNG_U32( 99, 73, 67, 80)
#define png_eXIf PNG_U32(101, 88, 73, 102) /* registered July 2017 */
#define png_fRAc PNG_U32(102, 82, 65, 99) /* registered, not defined */
#define png_gAMA PNG_U32(103, 65, 77, 65)
Expand Down Expand Up @@ -1172,6 +1173,12 @@ PNG_INTERNAL_FUNCTION(void,png_write_cHRM_fixed,(png_structrp png_ptr,
/* The xy value must have been previously validated */
#endif

#ifdef PNG_WRITE_cICP_SUPPORTED
PNG_INTERNAL_FUNCTION(void,png_write_cICP,(png_structrp png_ptr,
png_byte colour_primaries, png_byte transfer_function,
png_byte matrix_coefficients, png_byte video_full_range_flag), PNG_EMPTY);
#endif

#ifdef PNG_WRITE_sRGB_SUPPORTED
PNG_INTERNAL_FUNCTION(void,png_write_sRGB,(png_structrp png_ptr,
int intent),PNG_EMPTY);
Expand Down Expand Up @@ -1515,6 +1522,11 @@ PNG_INTERNAL_FUNCTION(void,png_handle_cHRM,(png_structrp png_ptr,
png_inforp info_ptr, png_uint_32 length),PNG_EMPTY);
#endif

#ifdef PNG_READ_cICP_SUPPORTED
PNG_INTERNAL_FUNCTION(void,png_handle_cICP,(png_structrp png_ptr,
png_inforp info_ptr, png_uint_32 length),PNG_EMPTY);
#endif

#ifdef PNG_READ_eXIf_SUPPORTED
PNG_INTERNAL_FUNCTION(void,png_handle_eXIf,(png_structrp png_ptr,
png_inforp info_ptr, png_uint_32 length),PNG_EMPTY);
Expand Down
4 changes: 4 additions & 0 deletions pngread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,10 @@ png_read_info(png_structrp png_ptr, png_inforp info_ptr)
else if (chunk_name == png_cHRM)
png_handle_cHRM(png_ptr, info_ptr, length);
#endif
#ifdef PNG_READ_cICP_SUPPORTED
else if (chunk_name == png_cICP)
png_handle_cICP(png_ptr, info_ptr, length);
#endif

#ifdef PNG_READ_eXIf_SUPPORTED
else if (chunk_name == png_eXIf)
Expand Down
37 changes: 37 additions & 0 deletions pngrutil.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2032,6 +2032,43 @@ png_handle_bKGD(png_structrp png_ptr, png_inforp info_ptr, png_uint_32 length)
}
#endif

#ifdef PNG_READ_cICP_SUPPORTED
void /* PRIVATE */
png_handle_cICP(png_structrp png_ptr, png_inforp info_ptr, png_uint_32 length)
{
png_byte buf[4];

png_debug(1, "in png_handle_cICP");
Copy link
Contributor

Choose a reason for hiding this comment

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

Interestingly, the string literal sharing doesn't happen here.


if ((png_ptr->mode & PNG_HAVE_IHDR) == 0)
png_chunk_error(png_ptr, "missing IHDR");

else if (info_ptr != NULL && (info_ptr->valid & PNG_INFO_cICP) != 0)
{
png_crc_finish(png_ptr, length);
png_chunk_benign_error(png_ptr, "duplicate");
return;
}

if ((png_ptr->mode & PNG_HAVE_IDAT) != 0)
png_ptr->mode |= PNG_AFTER_IDAT;
Comment on lines +2053 to +2054
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the right place to set this.

Rather, this is where we should confirm the IDAT hasn't yet arrived.

if ((png_ptr->mode & (PNG_HAVE_IDAT|PNG_HAVE_PLTE)) != 0)
{
png_crc_finish(png_ptr, length);
png_chunk_benign_error(png_ptr, "out of place");
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. The chunk placement has to be checked first.

This code is extremely unmaintainable; it's a state machine implemented by copy'n'paste and then replicated in pngpread.c. Any non-IDAT chunk which interrupts the IDAT stream, even if misplaced, should terminate the IDAT stream but it looks like the only ones that do are the ones that are valid after IDAT.

But take a look at line 129 of png.c; line 132 should have already set the flag before calling any of the ancillary png_handle_ routines... But then I suspect line 129 can never succeed.


if (length != 4)
{
png_crc_finish(png_ptr, length);
png_chunk_benign_error(png_ptr, "invalid");
return;
}

png_crc_read(png_ptr, buf, 4);

if (png_crc_finish(png_ptr, 0) != 0)
return;

Copy link
Contributor

Choose a reason for hiding this comment

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

Insert after the read & finish:

/* If a colorspace error has already been output skip this chunk */
if ((png_ptr->colorspace.flags & PNG_COLORSPACE_INVALID) != 0)
return;

png_set_cICP(png_ptr, info_ptr, buf[0], buf[1], buf[2], buf[3]);
}
#endif

#ifdef PNG_READ_eXIf_SUPPORTED
void /* PRIVATE */
png_handle_eXIf(png_structrp png_ptr, png_inforp info_ptr, png_uint_32 length)
Expand Down
20 changes: 20 additions & 0 deletions pngset.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,26 @@ png_set_cHRM_XYZ(png_const_structrp png_ptr, png_inforp info_ptr, double red_X,

#endif /* cHRM */

#ifdef PNG_cICP_SUPPORTED
void PNGAPI
png_set_cICP(png_const_structrp png_ptr,
png_inforp info_ptr, png_byte colour_primaries,
png_byte transfer_function, png_byte matrix_coefficients,
png_byte video_full_range_flag)
{
png_debug1(1, "in %s storage function", "cICP");

if (png_ptr == NULL || info_ptr == NULL)
return;

info_ptr->cicp_colour_primaries = colour_primaries;
info_ptr->cicp_transfer_function = transfer_function;
info_ptr->cicp_matrix_coefficients = matrix_coefficients;
info_ptr->cicp_video_full_range_flag = video_full_range_flag;
info_ptr->valid |= PNG_INFO_cICP;
}
#endif /* cICP */

#ifdef PNG_eXIf_SUPPORTED
void PNGAPI
png_set_eXIf(png_const_structrp png_ptr, png_inforp info_ptr,
Expand Down
17 changes: 17 additions & 0 deletions pngtest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1206,6 +1206,23 @@ test_one_file(const char *inname, const char *outname)
png_set_bKGD(write_ptr, write_info_ptr, background);
}
#endif
#ifdef PNG_cICP_SUPPORTED
{
png_byte colour_primaries;
png_byte transfer_function;
png_byte matrix_coefficients;
png_byte video_full_range_flag;

if (png_get_cICP(read_ptr, read_info_ptr, &colour_primaries,
&transfer_function, &matrix_coefficients,
&video_full_range_flag) != 0)
#ifdef PNG_WRITE_cICP_SUPPORTED
png_set_cICP(write_ptr, write_info_ptr, colour_primaries,
transfer_function, matrix_coefficients,
video_full_range_flag);
#endif
}
#endif
#ifdef PNG_READ_eXIf_SUPPORTED
{
png_bytep exif = NULL;
Expand Down
10 changes: 10 additions & 0 deletions pngwrite.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,16 @@ png_write_info(png_structrp png_ptr, png_const_inforp info_ptr)
png_write_bKGD(png_ptr, &(info_ptr->background), info_ptr->color_type);
#endif

#ifdef PNG_WRITE_cICP_SUPPORTED
if ((info_ptr->valid & PNG_INFO_cICP) != 0)
{
png_write_cICP(png_ptr, info_ptr->cicp_colour_primaries,
info_ptr->cicp_transfer_function,
info_ptr->cicp_matrix_coefficients,
info_ptr->cicp_video_full_range_flag);
}
#endif

#ifdef PNG_WRITE_eXIf_SUPPORTED
if ((info_ptr->valid & PNG_INFO_eXIf) != 0)
{
Expand Down
20 changes: 20 additions & 0 deletions pngwutil.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1471,6 +1471,26 @@ png_write_bKGD(png_structrp png_ptr, png_const_color_16p back, int color_type)
}
#endif

#ifdef PNG_WRITE_cICP_SUPPORTED
/* Write the cICP data */
void /* PRIVATE */
png_write_cICP(png_structrp png_ptr,
png_byte colour_primaries, png_byte transfer_function,
png_byte matrix_coefficients, png_byte video_full_range_flag)
{
png_debug(1, "in png_write_cICP");

png_write_chunk_header(png_ptr, png_cICP, 4);

png_write_chunk_data(png_ptr, &colour_primaries, 1);
png_write_chunk_data(png_ptr, &transfer_function, 1);
png_write_chunk_data(png_ptr, &matrix_coefficients, 1);
png_write_chunk_data(png_ptr, &video_full_range_flag, 1);

png_write_chunk_end(png_ptr);
}
#endif

#ifdef PNG_WRITE_eXIf_SUPPORTED
/* Write the Exif data */
void /* PRIVATE */
Expand Down
1 change: 1 addition & 0 deletions scripts/pnglibconf.dfa
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,6 +846,7 @@ setting IDAT_READ_SIZE default PNG_ZBUF_SIZE
# Ancillary chunks
chunk bKGD
chunk cHRM enables COLORSPACE
chunk cICP
chunk eXIf
chunk gAMA enables GAMMA
chunk hIST
Expand Down
3 changes: 3 additions & 0 deletions scripts/pnglibconf.h.prebuilt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
#define PNG_READ_USER_TRANSFORM_SUPPORTED
#define PNG_READ_bKGD_SUPPORTED
#define PNG_READ_cHRM_SUPPORTED
#define PNG_READ_cICP_SUPPORTED
#define PNG_READ_eXIf_SUPPORTED
#define PNG_READ_gAMA_SUPPORTED
#define PNG_READ_hIST_SUPPORTED
Expand Down Expand Up @@ -158,6 +159,7 @@
#define PNG_WRITE_WEIGHTED_FILTER_SUPPORTED
#define PNG_WRITE_bKGD_SUPPORTED
#define PNG_WRITE_cHRM_SUPPORTED
#define PNG_WRITE_cICP_SUPPORTED
#define PNG_WRITE_eXIf_SUPPORTED
#define PNG_WRITE_gAMA_SUPPORTED
#define PNG_WRITE_hIST_SUPPORTED
Expand All @@ -176,6 +178,7 @@
#define PNG_WRITE_zTXt_SUPPORTED
#define PNG_bKGD_SUPPORTED
#define PNG_cHRM_SUPPORTED
#define PNG_cICP_SUPPORTED
#define PNG_eXIf_SUPPORTED
#define PNG_gAMA_SUPPORTED
#define PNG_hIST_SUPPORTED
Expand Down
2 changes: 2 additions & 0 deletions scripts/symbols.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,3 +253,5 @@ EXPORTS
png_set_eXIf @247
png_get_eXIf_1 @248
png_set_eXIf_1 @249
png_get_cICP @250
png_set_cICP @251