diff mbox series

[FFmpeg-devel,v7,1/2] avcodec/pngenc: support writing iCCP chunks

Message ID 20220328152612.95684-1-ffmpeg@haasn.xyz
State New
Headers show
Series [FFmpeg-devel,v7,1/2] avcodec/pngenc: support writing iCCP chunks | expand

Commit Message

Niklas Haas March 28, 2022, 3:26 p.m. UTC 
From: Niklas Haas <git@haasn.dev>

We re-use the PNGEncContext.zstream for deflate-related operations.
Other than that, the code is pretty straightforward. Special care needs
to be taken to avoid writing more than 79 characters of the profile
description (the maximum supported).

To write the (dynamically sized) deflate-encoded data, we allocate extra
space in the packet and use that directly as a scratch buffer. Modify
png_write_chunk slightly to allow pre-writing the chunk contents like
this.

Also add a FATE transcode test to ensure that the ICC profile gets
encoded correctly.

Signed-off-by: Niklas Haas <git@haasn.dev>
---
 libavcodec/pngenc.c    | 91 +++++++++++++++++++++++++++++++++++++++++-
 tests/fate/image.mak   | 10 ++++-
 tests/ref/fate/png-icc | 43 ++++++++++++++++++++
 3 files changed, 140 insertions(+), 4 deletions(-)
 create mode 100644 tests/ref/fate/png-icc

Comments

Andreas Rheinhardt April 1, 2022, 1:31 p.m. UTC | #1 
Niklas Haas:
> From: Niklas Haas <git@haasn.dev>
> 
> We re-use the PNGEncContext.zstream for deflate-related operations.
> Other than that, the code is pretty straightforward. Special care needs
> to be taken to avoid writing more than 79 characters of the profile
> description (the maximum supported).
> 
> To write the (dynamically sized) deflate-encoded data, we allocate extra
> space in the packet and use that directly as a scratch buffer. Modify
> png_write_chunk slightly to allow pre-writing the chunk contents like
> this.
> 
> Also add a FATE transcode test to ensure that the ICC profile gets
> encoded correctly.
> 
> Signed-off-by: Niklas Haas <git@haasn.dev>
> ---
>  libavcodec/pngenc.c    | 91 +++++++++++++++++++++++++++++++++++++++++-
>  tests/fate/image.mak   | 10 ++++-
>  tests/ref/fate/png-icc | 43 ++++++++++++++++++++
>  3 files changed, 140 insertions(+), 4 deletions(-)
>  create mode 100644 tests/ref/fate/png-icc
> 
> diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c
> index f67f90cd14..1571673f7c 100644
> --- a/libavcodec/pngenc.c
> +++ b/libavcodec/pngenc.c
> @@ -236,7 +236,8 @@ static void png_write_chunk(uint8_t **f, uint32_t tag,
>      bytestream_put_be32(f, av_bswap32(tag));
>      if (length > 0) {
>          crc = av_crc(crc_table, crc, buf, length);
> -        memcpy(*f, buf, length);
> +        if (*f != buf)
> +            memcpy(*f, buf, length);
>          *f += length;
>      }
>      bytestream_put_be32(f, ~crc);
> @@ -345,10 +346,54 @@ static int png_get_gama(enum AVColorTransferCharacteristic trc, uint8_t *buf)
>      return 1;
>  }
>  
> +static int png_write_iccp(AVCodecContext *avctx, const AVFrameSideData *sd)

Passing the PNGEncContext would be more natural.

> +{
> +    PNGEncContext *s = avctx->priv_data;
> +    z_stream *const zstream = &s->zstream.zstream;
> +    const AVDictionaryEntry *entry;
> +    const char *name;
> +    uint8_t *start, *buf;
> +    int ret;
> +
> +    if (!sd || !sd->size)
> +        return 0;
> +    zstream->next_in  = sd->data;
> +    zstream->avail_in = sd->size;
> +
> +    /* write the chunk contents first */
> +    start = s->bytestream + 8; /* make room for iCCP tag + length */
> +    buf = start;
> +
> +    /* profile description */
> +    entry = av_dict_get(sd->metadata, "name", NULL, 0);
> +    name = (entry && entry->value[0]) ? entry->value : "icc";
> +    for (int i = 0;; i++) {
> +        char c = (i == 79) ? 0 : name[i];
> +        bytestream_put_byte(&buf, c);
> +        if (!c)
> +            break;
> +    }
> +
> +    /* compression method and profile data */
> +    bytestream_put_byte(&buf, 0);
> +    zstream->next_out  = buf;
> +    zstream->avail_out = s->bytestream_end - buf;
> +    ret = deflate(zstream, Z_FINISH);
> +    deflateReset(zstream);
> +    if (ret != Z_STREAM_END)
> +        return AVERROR_EXTERNAL;
> +
> +    /* rewind to the start and write the chunk header/crc */
> +    png_write_chunk(&s->bytestream, MKTAG('i', 'C', 'C', 'P'), start,
> +                    zstream->next_out - start);
> +    return 0;
> +}
> +
>  static int encode_headers(AVCodecContext *avctx, const AVFrame *pict)
>  {
>      AVFrameSideData *side_data;
>      PNGEncContext *s = avctx->priv_data;
> +    int ret;
>  
>      /* write png header */
>      AV_WB32(s->buf, avctx->width);
> @@ -401,7 +446,13 @@ static int encode_headers(AVCodecContext *avctx, const AVFrame *pict)
>      if (png_get_gama(pict->color_trc, s->buf))
>          png_write_chunk(&s->bytestream, MKTAG('g', 'A', 'M', 'A'), s->buf, 4);
>  
> -    /* put the palette if needed */
> +    side_data = av_frame_get_side_data(pict, AV_FRAME_DATA_ICC_PROFILE);
> +    if ((ret = png_write_iccp(avctx, side_data))) {
> +        av_log(avctx, AV_LOG_WARNING, "Failed writing iCCP chunk\n");
> +        return ret;
> +    }
> +
> +    /* put the palette if needed, must be after colorspace information */
>      if (s->color_type == PNG_COLOR_TYPE_PALETTE) {
>          int has_alpha, alpha, i;
>          unsigned int v;
> @@ -525,6 +576,38 @@ the_end:
>      return ret;
>  }
>  
> +static int add_icc_profile_size(AVCodecContext *avctx, const AVFrame *pict,
> +                                size_t *max_packet_size)

Since db57a5370bd37105d389a45b04bf4970802407ec the callers'
max_packet_size are not size_t any more, but always 64bit (so that
there's no truncation in case size_t is 32bit).

> +{
> +    PNGEncContext *s = avctx->priv_data;
> +    const AVFrameSideData *sd;
> +    const int hdr_size = 128;
> +    size_t new_pkt_size;
> +    uLong bound;
> +
> +    if (!pict)
> +        return 0;
> +    sd = av_frame_get_side_data(pict, AV_FRAME_DATA_ICC_PROFILE);
> +    if (!sd || !sd->size)
> +        return 0;
> +    if (sd->size > ULONG_MAX)

ULONG_MAX is the maximum of unsigned long, yet deflateBound uses uLong.
The latter is a currently typedef for unsigned long, but do we want to
rely on that? The ordinary way to check for whethe a value can be
represented in a type is by "if (sd->size != (uLong)sd->size)"

> +        goto overflow;
> +
> +    bound = deflateBound(&s->zstream.zstream, sd->size);
> +    if (bound > INT32_MAX - hdr_size)
> +        goto overflow;
> +
> +    new_pkt_size = *max_packet_size + bound + hdr_size;
> +    if (new_pkt_size < *max_packet_size)
> +        goto overflow;
> +    *max_packet_size = new_pkt_size;
> +    return 0;
> +
> +overflow:
> +    av_log(avctx, AV_LOG_WARNING, "ICC profile too large\n");

AV_LOG_WARNING makes no sense given that you error out afterwards.
(And anyway: Is a log-message really needed for something that will
never happen in reality?)

> +    return AVERROR_INVALIDDATA;
> +}
> +
>  static int encode_png(AVCodecContext *avctx, AVPacket *pkt,
>                        const AVFrame *pict, int *got_packet)
>  {
> @@ -541,6 +624,8 @@ static int encode_png(AVCodecContext *avctx, AVPacket *pkt,
>              enc_row_size +
>              12 * (((int64_t)enc_row_size + IOBUF_SIZE - 1) / IOBUF_SIZE) // IDAT * ceil(enc_row_size / IOBUF_SIZE)
>          );
> +    if ((ret = add_icc_profile_size(avctx, pict, &max_packet_size)))
> +        return ret;
>      ret = ff_alloc_packet(avctx, pkt, max_packet_size);
>      if (ret < 0)
>          return ret;
> @@ -870,6 +955,8 @@ static int encode_apng(AVCodecContext *avctx, AVPacket *pkt,
>              enc_row_size +
>              (4 + 12) * (((int64_t)enc_row_size + IOBUF_SIZE - 1) / IOBUF_SIZE) // fdAT * ceil(enc_row_size / IOBUF_SIZE)
>          );
> +    if ((ret = add_icc_profile_size(avctx, pict, &max_packet_size)))
> +        return ret;
>      if (max_packet_size > INT_MAX)
>          return AVERROR(ENOMEM);
>  
> diff --git a/tests/fate/image.mak b/tests/fate/image.mak
> index 573d398915..c6374c7d8a 100644
> --- a/tests/fate/image.mak
> +++ b/tests/fate/image.mak
> @@ -385,11 +385,15 @@ FATE_PNG_PROBE += fate-png-side-data
>  fate-png-side-data: CMD = run ffprobe$(PROGSSUF)$(EXESUF) -show_frames \
>      -i $(TARGET_SAMPLES)/png1/lena-int_rgb24.png
>  
> +FATE_PNG_TRANSCODE-$(call ENCDEC, PNG, IMAGE2) += fate-png-icc
> +fate-png-icc: CMD = transcode png_pipe $(TARGET_SAMPLES)/png1/lena-int_rgb24.png image2 "-c png" "" "" "-show_frames"
> +
>  FATE_PNG-$(call DEMDEC, IMAGE2, PNG) += $(FATE_PNG)
>  FATE_PNG_PROBE-$(call DEMDEC, IMAGE2, PNG) += $(FATE_PNG_PROBE)
>  FATE_IMAGE += $(FATE_PNG-yes)
>  FATE_IMAGE_PROBE += $(FATE_PNG_PROBE-yes)
> -fate-png: $(FATE_PNG-yes) $(FATE_PNG_PROBE-yes)
> +FATE_IMAGE_TRANSCODE += $(FATE_PNG_TRANSCODE-yes)
> +fate-png: $(FATE_PNG-yes) $(FATE_PNG_PROBE-yes) $(FATE_PNG_TRANSCODE-yes)
>  
>  FATE_IMAGE-$(call DEMDEC, IMAGE2, PTX) += fate-ptx
>  fate-ptx: CMD = framecrc -i $(TARGET_SAMPLES)/ptx/_113kw_pic.ptx -pix_fmt rgb24 -vf scale
> @@ -551,8 +555,10 @@ fate-xbm: $(FATE_XBM-yes)
>  
>  FATE_IMAGE += $(FATE_IMAGE-yes)
>  FATE_IMAGE_PROBE += $(FATE_IMAGE_PROBE-yes)
> +FATE_IMAGE_TRANSCODE += $(FATE_IMAGE_TRANSCODE-yes)
>  
>  FATE_SAMPLES_FFMPEG += $(FATE_IMAGE)
>  FATE_SAMPLES_FFPROBE += $(FATE_IMAGE_PROBE)
> +FATE_SAMPLES_FFMPEG_FFPROBE += $(FATE_IMAGE_TRANSCODE)
>  
> -fate-image: $(FATE_IMAGE) $(FATE_IMAGE_PROBE)
> +fate-image: $(FATE_IMAGE) $(FATE_IMAGE_PROBE) $(FATE_IMAGE_TRANSCODE)
> diff --git a/tests/ref/fate/png-icc b/tests/ref/fate/png-icc
> new file mode 100644
> index 0000000000..542bb76f9a
> --- /dev/null
> +++ b/tests/ref/fate/png-icc
> @@ -0,0 +1,43 @@
> +a50d37a0e72bddea2fcbba6fb773e2a0 *tests/data/fate/png-icc.image2
> +49397 tests/data/fate/png-icc.image2
> +#tb 0: 1/25
> +#media_type 0: video
> +#codec_id 0: rawvideo
> +#dimensions 0: 128x128
> +#sar 0: 2835/2835
> +0,          0,          0,        1,    49152, 0xe0013dee
> +[FRAME]
> +media_type=video
> +stream_index=0
> +key_frame=1
> +pts=0
> +pts_time=0.000000
> +pkt_dts=0
> +pkt_dts_time=0.000000
> +best_effort_timestamp=0
> +best_effort_timestamp_time=0.000000
> +pkt_duration=1
> +pkt_duration_time=0.040000
> +pkt_pos=0
> +pkt_size=49397
> +width=128
> +height=128
> +pix_fmt=rgb24
> +sample_aspect_ratio=1:1
> +pict_type=I
> +coded_picture_number=0
> +display_picture_number=0
> +interlaced_frame=0
> +top_field_first=0
> +repeat_pict=0
> +color_range=pc
> +color_space=unknown
> +color_primaries=unknown
> +color_transfer=unknown
> +chroma_location=unspecified
> +[SIDE_DATA]
> +side_data_type=ICC profile
> +name=Photoshop ICC profile
> +size=3144
> +[/SIDE_DATA]
> +[/FRAME]
Niklas Haas April 5, 2022, 10:59 a.m. UTC | #2 
On Fri, 01 Apr 2022 15:31:16 +0200 Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> Passing the PNGEncContext would be more natural.

Changed.

> Since db57a5370bd37105d389a45b04bf4970802407ec the callers'
> max_packet_size are not size_t any more, but always 64bit (so that
> there's no truncation in case size_t is 32bit).

Changed.

> ULONG_MAX is the maximum of unsigned long, yet deflateBound uses uLong.
> The latter is a currently typedef for unsigned long, but do we want to
> rely on that? The ordinary way to check for whethe a value can be
> represented in a type is by "if (sd->size != (uLong)sd->size)"

Changed. I didn't know about this trick, thanks.

> AV_LOG_WARNING makes no sense given that you error out afterwards.
> (And anyway: Is a log-message really needed for something that will
> never happen in reality?)

I think you're right. For JPEG it makes sense to keep around an error
printout, because the JPEG profile limit is quite small in comparison
(16 MB - the largest ICC profiles I have encountered in the wild are on
the order of megabytes as well).

But PNG supports up to 4 GB ICC profiles, which strains all conceivable
credibility. Somebody would have to be deliberately trying to hit this
limitation. Removed both error printouts (including the one about zlib
misbehaving, equally unlikely).
diff mbox series

Patch

diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c
index f67f90cd14..1571673f7c 100644
--- a/libavcodec/pngenc.c
+++ b/libavcodec/pngenc.c
@@ -236,7 +236,8 @@  static void png_write_chunk(uint8_t **f, uint32_t tag,
     bytestream_put_be32(f, av_bswap32(tag));
     if (length > 0) {
         crc = av_crc(crc_table, crc, buf, length);
-        memcpy(*f, buf, length);
+        if (*f != buf)
+            memcpy(*f, buf, length);
         *f += length;
     }
     bytestream_put_be32(f, ~crc);
@@ -345,10 +346,54 @@  static int png_get_gama(enum AVColorTransferCharacteristic trc, uint8_t *buf)
     return 1;
 }
 
+static int png_write_iccp(AVCodecContext *avctx, const AVFrameSideData *sd)
+{
+    PNGEncContext *s = avctx->priv_data;
+    z_stream *const zstream = &s->zstream.zstream;
+    const AVDictionaryEntry *entry;
+    const char *name;
+    uint8_t *start, *buf;
+    int ret;
+
+    if (!sd || !sd->size)
+        return 0;
+    zstream->next_in  = sd->data;
+    zstream->avail_in = sd->size;
+
+    /* write the chunk contents first */
+    start = s->bytestream + 8; /* make room for iCCP tag + length */
+    buf = start;
+
+    /* profile description */
+    entry = av_dict_get(sd->metadata, "name", NULL, 0);
+    name = (entry && entry->value[0]) ? entry->value : "icc";
+    for (int i = 0;; i++) {
+        char c = (i == 79) ? 0 : name[i];
+        bytestream_put_byte(&buf, c);
+        if (!c)
+            break;
+    }
+
+    /* compression method and profile data */
+    bytestream_put_byte(&buf, 0);
+    zstream->next_out  = buf;
+    zstream->avail_out = s->bytestream_end - buf;
+    ret = deflate(zstream, Z_FINISH);
+    deflateReset(zstream);
+    if (ret != Z_STREAM_END)
+        return AVERROR_EXTERNAL;
+
+    /* rewind to the start and write the chunk header/crc */
+    png_write_chunk(&s->bytestream, MKTAG('i', 'C', 'C', 'P'), start,
+                    zstream->next_out - start);
+    return 0;
+}
+
 static int encode_headers(AVCodecContext *avctx, const AVFrame *pict)
 {
     AVFrameSideData *side_data;
     PNGEncContext *s = avctx->priv_data;
+    int ret;
 
     /* write png header */
     AV_WB32(s->buf, avctx->width);
@@ -401,7 +446,13 @@  static int encode_headers(AVCodecContext *avctx, const AVFrame *pict)
     if (png_get_gama(pict->color_trc, s->buf))
         png_write_chunk(&s->bytestream, MKTAG('g', 'A', 'M', 'A'), s->buf, 4);
 
-    /* put the palette if needed */
+    side_data = av_frame_get_side_data(pict, AV_FRAME_DATA_ICC_PROFILE);
+    if ((ret = png_write_iccp(avctx, side_data))) {
+        av_log(avctx, AV_LOG_WARNING, "Failed writing iCCP chunk\n");
+        return ret;
+    }
+
+    /* put the palette if needed, must be after colorspace information */
     if (s->color_type == PNG_COLOR_TYPE_PALETTE) {
         int has_alpha, alpha, i;
         unsigned int v;
@@ -525,6 +576,38 @@  the_end:
     return ret;
 }
 
+static int add_icc_profile_size(AVCodecContext *avctx, const AVFrame *pict,
+                                size_t *max_packet_size)
+{
+    PNGEncContext *s = avctx->priv_data;
+    const AVFrameSideData *sd;
+    const int hdr_size = 128;
+    size_t new_pkt_size;
+    uLong bound;
+
+    if (!pict)
+        return 0;
+    sd = av_frame_get_side_data(pict, AV_FRAME_DATA_ICC_PROFILE);
+    if (!sd || !sd->size)
+        return 0;
+    if (sd->size > ULONG_MAX)
+        goto overflow;
+
+    bound = deflateBound(&s->zstream.zstream, sd->size);
+    if (bound > INT32_MAX - hdr_size)
+        goto overflow;
+
+    new_pkt_size = *max_packet_size + bound + hdr_size;
+    if (new_pkt_size < *max_packet_size)
+        goto overflow;
+    *max_packet_size = new_pkt_size;
+    return 0;
+
+overflow:
+    av_log(avctx, AV_LOG_WARNING, "ICC profile too large\n");
+    return AVERROR_INVALIDDATA;
+}
+
 static int encode_png(AVCodecContext *avctx, AVPacket *pkt,
                       const AVFrame *pict, int *got_packet)
 {
@@ -541,6 +624,8 @@  static int encode_png(AVCodecContext *avctx, AVPacket *pkt,
             enc_row_size +
             12 * (((int64_t)enc_row_size + IOBUF_SIZE - 1) / IOBUF_SIZE) // IDAT * ceil(enc_row_size / IOBUF_SIZE)
         );
+    if ((ret = add_icc_profile_size(avctx, pict, &max_packet_size)))
+        return ret;
     ret = ff_alloc_packet(avctx, pkt, max_packet_size);
     if (ret < 0)
         return ret;
@@ -870,6 +955,8 @@  static int encode_apng(AVCodecContext *avctx, AVPacket *pkt,
             enc_row_size +
             (4 + 12) * (((int64_t)enc_row_size + IOBUF_SIZE - 1) / IOBUF_SIZE) // fdAT * ceil(enc_row_size / IOBUF_SIZE)
         );
+    if ((ret = add_icc_profile_size(avctx, pict, &max_packet_size)))
+        return ret;
     if (max_packet_size > INT_MAX)
         return AVERROR(ENOMEM);
 
diff --git a/tests/fate/image.mak b/tests/fate/image.mak
index 573d398915..c6374c7d8a 100644
--- a/tests/fate/image.mak
+++ b/tests/fate/image.mak
@@ -385,11 +385,15 @@  FATE_PNG_PROBE += fate-png-side-data
 fate-png-side-data: CMD = run ffprobe$(PROGSSUF)$(EXESUF) -show_frames \
     -i $(TARGET_SAMPLES)/png1/lena-int_rgb24.png
 
+FATE_PNG_TRANSCODE-$(call ENCDEC, PNG, IMAGE2) += fate-png-icc
+fate-png-icc: CMD = transcode png_pipe $(TARGET_SAMPLES)/png1/lena-int_rgb24.png image2 "-c png" "" "" "-show_frames"
+
 FATE_PNG-$(call DEMDEC, IMAGE2, PNG) += $(FATE_PNG)
 FATE_PNG_PROBE-$(call DEMDEC, IMAGE2, PNG) += $(FATE_PNG_PROBE)
 FATE_IMAGE += $(FATE_PNG-yes)
 FATE_IMAGE_PROBE += $(FATE_PNG_PROBE-yes)
-fate-png: $(FATE_PNG-yes) $(FATE_PNG_PROBE-yes)
+FATE_IMAGE_TRANSCODE += $(FATE_PNG_TRANSCODE-yes)
+fate-png: $(FATE_PNG-yes) $(FATE_PNG_PROBE-yes) $(FATE_PNG_TRANSCODE-yes)
 
 FATE_IMAGE-$(call DEMDEC, IMAGE2, PTX) += fate-ptx
 fate-ptx: CMD = framecrc -i $(TARGET_SAMPLES)/ptx/_113kw_pic.ptx -pix_fmt rgb24 -vf scale
@@ -551,8 +555,10 @@  fate-xbm: $(FATE_XBM-yes)
 
 FATE_IMAGE += $(FATE_IMAGE-yes)
 FATE_IMAGE_PROBE += $(FATE_IMAGE_PROBE-yes)
+FATE_IMAGE_TRANSCODE += $(FATE_IMAGE_TRANSCODE-yes)
 
 FATE_SAMPLES_FFMPEG += $(FATE_IMAGE)
 FATE_SAMPLES_FFPROBE += $(FATE_IMAGE_PROBE)
+FATE_SAMPLES_FFMPEG_FFPROBE += $(FATE_IMAGE_TRANSCODE)
 
-fate-image: $(FATE_IMAGE) $(FATE_IMAGE_PROBE)
+fate-image: $(FATE_IMAGE) $(FATE_IMAGE_PROBE) $(FATE_IMAGE_TRANSCODE)
diff --git a/tests/ref/fate/png-icc b/tests/ref/fate/png-icc
new file mode 100644
index 0000000000..542bb76f9a
--- /dev/null
+++ b/tests/ref/fate/png-icc
@@ -0,0 +1,43 @@ 
+a50d37a0e72bddea2fcbba6fb773e2a0 *tests/data/fate/png-icc.image2
+49397 tests/data/fate/png-icc.image2
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 128x128
+#sar 0: 2835/2835
+0,          0,          0,        1,    49152, 0xe0013dee
+[FRAME]
+media_type=video
+stream_index=0
+key_frame=1
+pts=0
+pts_time=0.000000
+pkt_dts=0
+pkt_dts_time=0.000000
+best_effort_timestamp=0
+best_effort_timestamp_time=0.000000
+pkt_duration=1
+pkt_duration_time=0.040000
+pkt_pos=0
+pkt_size=49397
+width=128
+height=128
+pix_fmt=rgb24
+sample_aspect_ratio=1:1
+pict_type=I
+coded_picture_number=0
+display_picture_number=0
+interlaced_frame=0
+top_field_first=0
+repeat_pict=0
+color_range=pc
+color_space=unknown
+color_primaries=unknown
+color_transfer=unknown
+chroma_location=unspecified
+[SIDE_DATA]
+side_data_type=ICC profile
+name=Photoshop ICC profile
+size=3144
+[/SIDE_DATA]
+[/FRAME]