[FFmpeg-devel] [PATCH v3] avcodec/pngenc: support writing iCCP chunks
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Tue Mar 15 08:44:10 EET 2022
Niklas Haas:
> From: Niklas Haas <git at 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
More information about the ffmpeg-devel
mailing list