[FFmpeg-devel,v3] avcodec/pngenc: support writing iCCP chunks

From: Niklas Haas <git@haasn.dev>

encode_zbuf is mostly a mirror image of decode_zbuf. 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. This implementation does unfortunately require initializing the
deflate instance twice, but deflateBound() is not redundant with
deflate() so we're not throwing away any significant work.

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

Changes in v3:
- rewrite to write the chunk in-place (inside the packet buffer)

Actually, I implemented an AVBPrint-less version that I think I'm
happier with overall. The extent of the "crimes" needed to support
writing chunks in-place was a single `if` in png_write_chunk and
hard-coding an 8 byte start offset.

I like this the most, since it doesn't require dynamic allocation _at
all_. It also ends up producing a 1 byte smaller test file for some
reason (not as a result of any obvious bug, but simply because zlib
compresses the last few bytes of the stream in a slightly different way,
probably as a result of some internal heuristics related to the buffer
size - the decoded ICC profile checksum is the same).

---
 libavcodec/pngenc.c    | 93 +++++++++++++++++++++++++++++++++++++++++-
 tests/fate/image.mak   |  3 ++
 tests/ref/fate/png-icc |  8 ++++
 3 files changed, 102 insertions(+), 2 deletions(-)
 create mode 100644 tests/ref/fate/png-icc

Niklas Haas:
> From: Niklas Haas <git@haasn.dev>
> 
> encode_zbuf is mostly a mirror image of decode_zbuf. 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. This implementation does unfortunately require initializing the
> deflate instance twice, but deflateBound() is not redundant with
> deflate() so we're not throwing away any significant work.
> 
> Also add a FATE transcode test to ensure that the ICC profile gets
> encoded correctly.
> ---
> 
> Changes in v3:
> - rewrite to write the chunk in-place (inside the packet buffer)
> 
> Actually, I implemented an AVBPrint-less version that I think I'm
> happier with overall. The extent of the "crimes" needed to support
> writing chunks in-place was a single `if` in png_write_chunk and
> hard-coding an 8 byte start offset.
> 
> I like this the most, since it doesn't require dynamic allocation _at
> all_. It also ends up producing a 1 byte smaller test file for some
> reason (not as a result of any obvious bug, but simply because zlib
> compresses the last few bytes of the stream in a slightly different way,
> probably as a result of some internal heuristics related to the buffer
> size - the decoded ICC profile checksum is the same).
> 
> ---
>  libavcodec/pngenc.c    | 93 +++++++++++++++++++++++++++++++++++++++++-
>  tests/fate/image.mak   |  3 ++
>  tests/ref/fate/png-icc |  8 ++++
>  3 files changed, 102 insertions(+), 2 deletions(-)
>  create mode 100644 tests/ref/fate/png-icc
> 
> diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c
> index 3ebcc1e571..e9bbe33adf 100644
> --- a/libavcodec/pngenc.c
> +++ b/libavcodec/pngenc.c
> @@ -235,7 +235,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);
> @@ -343,10 +344,88 @@ static int png_get_gama(enum AVColorTransferCharacteristic trc, uint8_t *buf)
>      return 1;
>  }
>  
> +static size_t zbuf_bound(const uint8_t *data, size_t size)
> +{
> +    z_stream zstream;
> +    size_t bound;
> +
> +    zstream.zalloc = ff_png_zalloc,
> +    zstream.zfree  = ff_png_zfree,

Don't surprise people with comma operators.

> +    zstream.opaque = NULL;
> +    if (deflateInit(&zstream, Z_DEFAULT_COMPRESSION) != Z_OK)

Use the z_stream from the context here and in encode_zbuf() (together
with deflateReset). That saves code, memory and runtime and honours the
user's wishes about compression level. (It will save way more than what
any AVBPrint-small-string-optimization will ever do.)

> +        return 0;
> +
> +    zstream.next_in = data;
> +    zstream.avail_in = size;
> +    bound = deflateBound(&zstream, size);

deflateBound uses uLong for the size which is typically a typedef for
unsigned long. In particular, on 64bit Windows size_t is 64bit and uLong
is 32bit. Furthermore, you need to ensure that the chunk length fits
into 32bits (png_write_chunk() even uses an int instead of an uint32_t
for the length). So some length check is necessary here.
(Notice that my zconf.h contains "typedef unsigned long  uLong; /* 32
bits or more */", so you may presume uLong to be more encompassing than
uint32_t.)

> +    deflateEnd(&zstream);
> +    return bound;
> +}
> +
> +static int encode_zbuf(uint8_t **buf, const uint8_t *buf_end,
> +                       const uint8_t *data, size_t size)
> +{
> +    z_stream zstream;
> +    int ret;
> +
> +    zstream.zalloc = ff_png_zalloc,
> +    zstream.zfree  = ff_png_zfree,
> +    zstream.opaque = NULL;
> +    if (deflateInit(&zstream, Z_DEFAULT_COMPRESSION) != Z_OK)
> +        return AVERROR_EXTERNAL;
> +    zstream.next_in  = data;
> +    zstream.avail_in = size;
> +    zstream.next_out  = *buf;
> +    zstream.avail_out = buf_end - *buf;
> +    ret = deflate(&zstream, Z_FINISH);
> +    deflateEnd(&zstream);
> +    if (ret != Z_STREAM_END)
> +        return AVERROR_EXTERNAL;
> +
> +    *buf = zstream.next_out;
> +    return 0;
> +}
> +
> +static int png_write_iccp(uint8_t **bytestream, const uint8_t *end,
> +                          const AVFrameSideData *side_data)
> +{
> +    const AVDictionaryEntry *entry;
> +    const char *name;
> +    uint8_t *start, *buf;
> +    int ret;
> +
> +    if (!side_data || !side_data->size)
> +        return 0;
> +
> +    /* write the chunk contents first */
> +    start = *bytestream + 8; /* make room for iCCP tag + length */
> +    buf = start;
> +
> +    /* profile description */
> +    entry = av_dict_get(side_data->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);
> +    if ((ret = encode_zbuf(&buf, end, side_data->data, side_data->size)))
> +        return ret;
> +
> +    /* rewind to the start and write the chunk header/crc */
> +    png_write_chunk(bytestream, MKTAG('i', 'C', 'C', 'P'), start, buf - 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);
> @@ -399,7 +478,14 @@ 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(&s->bytestream, s->bytestream_end, side_data))) {
> +        av_log(avctx, AV_LOG_WARNING, "Failed writing iCCP chunk: %s\n",
> +               av_err2str(ret));

The user already gets the error code (which is always AVERROR_EXTERNAL,
so not really useful); no need to repeat it.

> +        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;
> @@ -526,6 +612,7 @@ static int encode_png(AVCodecContext *avctx, AVPacket *pkt,
>                        const AVFrame *pict, int *got_packet)
>  {
>      PNGEncContext *s = avctx->priv_data;
> +    const AVFrameSideData *sd;
>      int ret;
>      int enc_row_size;
>      size_t max_packet_size;
> @@ -537,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 ((sd = av_frame_get_side_data(pict, AV_FRAME_DATA_ICC_PROFILE)))
> +        max_packet_size += zbuf_bound(sd->data, sd->size);

You only account for this when encoding png, yet not for apng;
encode_headers() is also called for them and AV_INPUT_BUFFER_MIN_SIZE
might not suffice; anyway, we should try to avoid using that define --
it is a remnant of an era when users could (had to?) provide buffers in
the hope that they are big enough.

>      if (max_packet_size > INT_MAX)
>          return AVERROR(ENOMEM);
>      ret = ff_alloc_packet(avctx, pkt, max_packet_size);
> diff --git a/tests/fate/image.mak b/tests/fate/image.mak
> index 573d398915..da4f3709e9 100644
> --- a/tests/fate/image.mak
> +++ b/tests/fate/image.mak
> @@ -385,6 +385,9 @@ 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_PROBE += fate-png-icc
> +fate-png-icc: CMD = transcode png_pipe $(TARGET_SAMPLES)/png1/lena-int_rgb24.png image2 "-c png"
> +

FATE_PNG_PROBE exists for those tests which only need ffprobe, not
ffmpeg. A transcode test always need ffmpeg.
And as I already said: You should use ffprobe to read the side data of
the frames that emanate from decoding the encoded file.

>  FATE_PNG-$(call DEMDEC, IMAGE2, PNG) += $(FATE_PNG)
>  FATE_PNG_PROBE-$(call DEMDEC, IMAGE2, PNG) += $(FATE_PNG_PROBE)
>  FATE_IMAGE += $(FATE_PNG-yes)
> diff --git a/tests/ref/fate/png-icc b/tests/ref/fate/png-icc
> new file mode 100644
> index 0000000000..fd92a9f949
> --- /dev/null
> +++ b/tests/ref/fate/png-icc
> @@ -0,0 +1,8 @@
> +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