[FFmpeg-devel] [PATCH v7 1/2] avcodec/pngenc: support writing iCCP chunks
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Fri Apr 1 16:31:16 EEST 2022
Niklas Haas:
> From: Niklas Haas <git at 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 at 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]
More information about the ffmpeg-devel
mailing list