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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".