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 mDCv and cLLi chunks #626

Open
wants to merge 2 commits into
base: libpng16
Choose a base branch
from
Open

Add support for reading and writing the mDCv and cLLi chunks #626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c0fb6a7
add mdcv, clli support
Miloni2010 Nov 17, 2024
1c0d0ba
add test file
Miloni2010 Nov 17, 2024
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
2 changes: 1 addition & 1 deletion png.c
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.

There should be no changes to either png_struct::colorspace or png_info::colorspace firstly because at present the structs are metadata (informational not authoritative) secondly because app involvement is required; there is no API to say because we don't know what the output colour volume is and thirdly, perhaps most important, there is no cICP handling in ::colorspace yet.

I can't see any way of making the colorspace handling useful without an enormous amount of work so this should be removed pending review of what the W3C actually want.

Original file line number Diff line number Diff line change
Expand Up @@ -1150,7 +1150,7 @@ png_colorspace_sync_info(png_const_structrp png_ptr, png_inforp info_ptr)
{
/* Everything is invalid */
info_ptr->valid &= ~(PNG_INFO_gAMA|PNG_INFO_cHRM|PNG_INFO_sRGB|
PNG_INFO_iCCP);
PNG_INFO_iCCP|PNG_INFO_cLLi|PNG_INFO_mDCv);

# ifdef PNG_COLORSPACE_SUPPORTED
/* Clean up the iCCP profile now if it won't be used. */
Expand Down
37 changes: 36 additions & 1 deletion png.h
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.

How the data is stored should be internal. The spec is a bit of a mess because it introduces a new way of storing a colour but note how png_xy is defined in pngstruct.h then look at png_get_cHRM in pngget.c; libpng for the most part does not return results in structures. It returns them via individual integral or floating point values.

Introducing a new and weirdly different representation of what is effectively the same as a cHRM chunk (so far as the colours are concerned) is just going to be plain confusing; I think it is confusing in both the W3C proposal and the libpng API but I only really care about the API.

Looking at the Blink code I can see that that code handles this stuff as a 24 byte array; the suggested API is just going to massively complicate at least the current Blink code. One possible API is just to return a pointer to the buffer, that will be a lot easier for the existing implementation that I've examined. If I were the person responsible for making the Blink code compatible with libpng 1.8 I would do nothing; it will just work. What really matters is whether the API is comprehensible to people who haven't encountered HDR or wide gamut images before.

A better approach IMO is to unify with cHRM and return, at the caller's option, either double precision FP values or png_fixed_point (so two APIs). I looked at this before and came to the conclusion that this is the least confusing approach.

It's up for discussion. I'm against introducing a new struct at this point but I might be in favour of introducing general representations of both colour and illumination level (using SI units of course).

Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,25 @@ typedef png_time * png_timep;
typedef const png_time * png_const_timep;
typedef png_time * * png_timepp;

#ifdef PNG_mDCv_SUPPORTED
typedef struct png_mdcv_struct
{
png_uint_16 chromaticity_redx; /* for use in display primaries */
png_uint_16 chromaticity_redy ;
png_uint_16 chromaticity_greenx;
png_uint_16 chromaticity_greeny;
png_uint_16 chromaticity_bluex;
png_uint_16 chromaticity_bluey;
png_uint_16 chromaticity_whitex; /* for use white point */
png_uint_16 chromaticity_whitey;
png_uint_32 max_Lum; /* max, min master display luminance */
png_uint_32 min_Lum;
} png_mdcv;
typedef png_mdcv * png_mdcvp;
typedef const png_mdcv * png_const_mdcvp;
typedef png_mdcv * * png_mdcvpp;
#endif

#if defined(PNG_STORE_UNKNOWN_CHUNKS_SUPPORTED) ||\
defined(PNG_USER_CHUNKS_SUPPORTED)
/* png_unknown_chunk is a structure to hold queued chunks for which there is
Expand Down Expand Up @@ -744,6 +763,8 @@ 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_mDCv 0x40000U /* mDCv chunk */
#define PNG_INFO_cLLi 0x80000U /* cLLi chunk */

/* 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 @@ -2093,6 +2114,20 @@ PNG_EXPORT(159, void, png_set_iCCP, (png_const_structrp png_ptr,
png_const_bytep profile, png_uint_32 proflen));
#endif

#ifdef PNG_mDCv_SUPPORTED
PNG_EXPORT(250, png_uint_32, png_get_mDCv, (png_const_structrp png_ptr,
png_inforp info_ptr, png_mdcvp *mastering_display_color_volume));
PNG_EXPORT(251, void, png_set_mDCv, (png_const_structrp png_ptr,
png_inforp info_ptr, png_const_mdcvp mastering_display_color_volume));
#endif

#ifdef PNG_cLLi_SUPPORTED
PNG_EXPORT(252, png_uint_32, png_get_cLLi, (png_const_structrp png_ptr,
png_inforp info_ptr, png_uint_32 *max_cll, png_uint_32 *max_fall));
PNG_EXPORT(253, void, png_set_cLLi, (png_const_structrp png_ptr,
png_inforp info_ptr, png_uint_32 max_cll, png_uint_32 max_fall));
#endif

#ifdef PNG_sPLT_SUPPORTED
PNG_EXPORT(160, int, png_get_sPLT, (png_const_structrp png_ptr,
png_inforp info_ptr, png_sPLT_tpp entries));
Expand Down Expand Up @@ -3237,7 +3272,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(253);
#endif

#ifdef __cplusplus
Expand Down
42 changes: 42 additions & 0 deletions pngget.c
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.

I would rather not see any more of this "writeable pointer into an internal structure" stuff introduced. It's a security nightmare, at least that is what happens every time I see it. This is a second though perhaps most compelling reason for returning the values as integers.

An alternative, while it may be inconsistent, is to require the caller to allocate the buffer. This is much much more like every other piece of code out there where the caller passes a pointer to a struct typically allocated on the stack. This is also a powerful argument for the alternative of requiring a 24 byte buffer and just filling it it; png.h exposes png_get_uint16 so, while perhaps weird, it provides all the support that, so far as I can determine, @ProgramMax is looking for, without giving out pointers to structs with an undefined layout.

Horrible. Better to give up and implement libpng 2; the unknown handling already supports the last paragraph...

Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,48 @@ png_get_iCCP(png_const_structrp png_ptr, png_inforp info_ptr,
}
#endif

#ifdef PNG_mDCv_SUPPORTED
png_uint_32 PNGAPI
png_get_mDCv(png_const_structrp png_ptr, png_inforp info_ptr,
png_mdcvp *mastering_display_color_volume)
{
png_debug1(1, "in %s retrieval function", "mDCv");

if (png_ptr != NULL && info_ptr != NULL &&
(info_ptr->valid & PNG_INFO_mDCv) != 0 && mastering_display_color_volume != NULL)
{
*mastering_display_color_volume = &(info_ptr->mastering_display_color_volume);
return (PNG_INFO_mDCv);
}

return (0);
}
#endif /* mDCv */

#ifdef PNG_cLLi_SUPPORTED
png_uint_32 PNGAPI
png_get_cLLi(png_const_structrp png_ptr, png_inforp info_ptr,
png_uint_32 *max_cll, png_uint_32 *max_fall)
{
png_debug1(1, "in %s retrieval function", "cLLi");

if (png_ptr != NULL && info_ptr != NULL &&
(info_ptr->valid & PNG_INFO_cLLi) != 0)
{
if (max_cll != NULL)
*max_cll = info_ptr->maximum_content_light_level;

if (max_fall != NULL)
*max_fall = info_ptr->maximum_frame_average_light_level ;

return (PNG_INFO_cLLi);
}

return (0);

}
#endif /* cLLi */

#ifdef PNG_sPLT_SUPPORTED
int PNGAPI
png_get_sPLT(png_const_structrp png_ptr, png_inforp info_ptr,
Expand Down
14 changes: 14 additions & 0 deletions pnginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,20 @@ struct png_info_def
png_bytep iccp_profile; /* International Color Consortium profile data */
png_uint_32 iccp_proflen; /* ICC profile data length */
#endif

#ifdef PNG_mDCv_SUPPORTED
/* The mDCv chunk characterizes the Mastering Display Color Volume (mDCv)
* used at the point of content creation holds the last time the displayed image
* data was modified. See the png_mdcv struct for the contents of this struct.
*/
png_mdcv mastering_display_color_volume;
#endif

#ifdef PNG_cLLi_SUPPORTED
/* cLLi chunk data. */
png_uint_32 maximum_content_light_level;
png_uint_32 maximum_frame_average_light_level;
#endif

#ifdef PNG_TEXT_SUPPORTED
/* The tEXt, and zTXt chunks contain human-readable textual data in
Expand Down
16 changes: 16 additions & 0 deletions pngpread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,22 @@ png_push_read_chunk(png_structrp png_ptr, png_inforp info_ptr)
png_handle_iCCP(png_ptr, info_ptr, png_ptr->push_length);
}

#endif
#ifdef PNG_READ_mDCv_SUPPORTED
else if (png_ptr->chunk_name == png_mDCv)
{
PNG_PUSH_SAVE_BUFFER_IF_FULL
png_handle_mDCv(png_ptr, info_ptr, png_ptr->push_length);
}

#endif
#ifdef PNG_READ_cLLi_SUPPORTED
else if (png_ptr->chunk_name == png_cLLi)
{
PNG_PUSH_SAVE_BUFFER_IF_FULL
png_handle_cLLi(png_ptr, info_ptr, png_ptr->push_length);
}

#endif
#ifdef PNG_READ_sPLT_SUPPORTED
else if (chunk_name == png_sPLT)
Expand Down
22 changes: 22 additions & 0 deletions pngpriv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,8 @@
#define png_gIFx PNG_U32(103, 73, 70, 120)
#define png_hIST PNG_U32(104, 73, 83, 84)
#define png_iCCP PNG_U32(105, 67, 67, 80)
#define png_mDCv PNG_U32(109, 68, 67, 118) /* mDCv */
#define png_cLLi PNG_U32( 99, 76, 76, 105) /* cLLi */
#define png_iTXt PNG_U32(105, 84, 88, 116)
#define png_oFFs PNG_U32(111, 70, 70, 115)
#define png_pCAL PNG_U32(112, 67, 65, 76)
Expand Down Expand Up @@ -1149,6 +1151,16 @@ PNG_INTERNAL_FUNCTION(void,png_write_iCCP,(png_structrp png_ptr,
*/
#endif

#ifdef PNG_WRITE_mDCv_SUPPORTED
PNG_INTERNAL_FUNCTION(void,png_write_mDCv,(png_structrp png_ptr,
png_const_mdcvp mastering_display_color_volume), PNG_EMPTY);
#endif

#ifdef PNG_WRITE_cLLi_SUPPORTED
PNG_INTERNAL_FUNCTION(void,png_write_cLLi,(png_structrp png_ptr,
png_uint_32 maximum_content_light_level, png_uint_32 maximum_frame_average_light_level), PNG_EMPTY);
#endif

#ifdef PNG_WRITE_sPLT_SUPPORTED
PNG_INTERNAL_FUNCTION(void,png_write_sPLT,(png_structrp png_ptr,
png_const_sPLT_tp palette),PNG_EMPTY);
Expand Down Expand Up @@ -1493,6 +1505,16 @@ PNG_INTERNAL_FUNCTION(void,png_handle_iCCP,(png_structrp png_ptr,
png_inforp info_ptr, png_uint_32 length),PNG_EMPTY);
#endif /* READ_iCCP */

#ifdef PNG_READ_mDCv_SUPPORTED
PNG_INTERNAL_FUNCTION(void,png_handle_mDCv,(png_structrp png_ptr,
png_inforp info_ptr, png_uint_32 length),PNG_EMPTY);
#endif /* READ_mDCv */

#ifdef PNG_READ_cLLi_SUPPORTED
PNG_INTERNAL_FUNCTION(void,png_handle_cLLi,(png_structrp png_ptr,
png_inforp info_ptr, png_uint_32 length),PNG_EMPTY);
#endif /* READ_cLLi */

#ifdef PNG_READ_iTXt_SUPPORTED
PNG_INTERNAL_FUNCTION(void,png_handle_iTXt,(png_structrp png_ptr,
png_inforp info_ptr, png_uint_32 length),PNG_EMPTY);
Expand Down
26 changes: 26 additions & 0 deletions pngread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,16 @@ png_read_info(png_structrp png_ptr, png_inforp info_ptr)
png_handle_iCCP(png_ptr, info_ptr, length);
#endif

#ifdef PNG_READ_mDCv_SUPPORTED
else if (chunk_name == png_mDCv)
png_handle_mDCv(png_ptr, info_ptr, length);
#endif

#ifdef PNG_READ_cLLi_SUPPORTED
else if (chunk_name == png_cLLi)
png_handle_cLLi(png_ptr, info_ptr, length);
#endif

#ifdef PNG_READ_sPLT_SUPPORTED
else if (chunk_name == png_sPLT)
png_handle_sPLT(png_ptr, info_ptr, length);
Expand Down Expand Up @@ -900,6 +910,16 @@ png_read_end(png_structrp png_ptr, png_inforp info_ptr)
else if (chunk_name == png_iCCP)
png_handle_iCCP(png_ptr, info_ptr, length);
#endif

#ifdef PNG_READ_mDCv_SUPPORTED
else if (chunk_name == png_mDCv)
png_handle_mDCv(png_ptr, info_ptr, length);
#endif

#ifdef PNG_READ_cLLi_SUPPORTED
else if (chunk_name == png_cLLi)
png_handle_cLLi(png_ptr, info_ptr, length);
#endif

#ifdef PNG_READ_sPLT_SUPPORTED
else if (chunk_name == png_sPLT)
Expand Down Expand Up @@ -1638,6 +1658,12 @@ png_image_skip_unused_chunks(png_structrp png_ptr)
103, 65, 77, 65, '\0', /* gAMA */
# ifdef PNG_READ_iCCP_SUPPORTED
105, 67, 67, 80, '\0', /* iCCP */
# endif
# ifdef PNG_READ_mDCv_SUPPORTED
109, 68, 67, 118, '\0', /* mDCv */
# endif
# ifdef PNG_READ_cLLi_SUPPORTED
99, 76, 76, 105, '\0', /* cLLi */
# endif
115, 66, 73, 84, '\0', /* sBIT */
115, 82, 71, 66, '\0', /* sRGB */
Expand Down
89 changes: 89 additions & 0 deletions pngrutil.c
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.

Ordering constraints WRT PLTE cannot be imposed on "safe-to-copy" chunks, so the "out of place" checks are incorrect. This is why the chunks' safe-to-copy bit is up for review.

Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,7 @@ png_crc_error(png_structrp png_ptr)
#if defined(PNG_READ_iCCP_SUPPORTED) || defined(PNG_READ_iTXt_SUPPORTED) ||\
defined(PNG_READ_pCAL_SUPPORTED) || defined(PNG_READ_sCAL_SUPPORTED) ||\
defined(PNG_READ_sPLT_SUPPORTED) || defined(PNG_READ_tEXt_SUPPORTED) ||\
defined(PNG_READ_mDCv_SUPPORTED) || defined(PNG_READ_cLLi_SUPPORTED) ||\
defined(PNG_READ_zTXt_SUPPORTED) || defined(PNG_SEQUENTIAL_READ_SUPPORTED)
/* Manage the read buffer; this simply reallocates the buffer if it is not small
* enough (or if it is not allocated). The routine returns a pointer to the
Expand Down Expand Up @@ -1647,6 +1648,94 @@ png_handle_iCCP(png_structrp png_ptr, png_inforp info_ptr, png_uint_32 length)
}
#endif /* READ_iCCP */

#ifdef PNG_READ_mDCv_SUPPORTED
void /* PRIVATE */
png_handle_mDCv(png_structrp png_ptr, png_inforp info_ptr, png_uint_32 length)
{
png_byte buf[24];
png_mdcv mastering_display_color_volume;

png_debug(1, "in png_handle_mDCv");

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

else 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;
}

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

png_crc_read(png_ptr, buf, 24);

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

// Extract the information
mastering_display_color_volume.chromaticity_redx = png_get_uint_16(buf);
mastering_display_color_volume.chromaticity_redy = png_get_uint_16(buf + 2);
mastering_display_color_volume.chromaticity_greenx = png_get_uint_16(buf + 4);
mastering_display_color_volume.chromaticity_greeny = png_get_uint_16(buf + 6);
mastering_display_color_volume.chromaticity_bluex = png_get_uint_16(buf + 8);
mastering_display_color_volume.chromaticity_bluey = png_get_uint_16(buf + 10);
mastering_display_color_volume.chromaticity_whitex = png_get_uint_16(buf + 12);
mastering_display_color_volume.chromaticity_whitey = png_get_uint_16(buf + 14);
mastering_display_color_volume.max_Lum = png_get_uint_32(buf + 16);
mastering_display_color_volume.min_Lum = png_get_uint_32(buf + 20);

png_set_mDCv(png_ptr, info_ptr, &mastering_display_color_volume);

}
#endif /* READ_mDCv */

#ifdef PNG_READ_cLLi_SUPPORTED
void /* PRIVATE */
png_handle_cLLi(png_structrp png_ptr, png_inforp info_ptr, png_uint_32 length)
{
png_byte buf[8];
png_uint_32 max_cll, max_fall;

png_debug(1, "in png_handle_cLLi");

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

else 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;
}

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

png_crc_read(png_ptr, buf, 8);

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

// Extract the information
max_cll = png_get_uint_32(buf);
max_fall = png_get_uint_32(buf + 4);

png_set_cLLi(png_ptr, info_ptr, max_cll, max_fall);

}
#endif /* READ_cLLi */

#ifdef PNG_READ_sPLT_SUPPORTED
void /* PRIVATE */
png_handle_sPLT(png_structrp png_ptr, png_inforp info_ptr, png_uint_32 length)
Expand Down
Loading