Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
* [FFmpeg-devel] [PATCH v7 1/2] avcodec/pngenc: support writing iCCP chunks
@ 2022-03-28 15:26 Niklas Haas
  2022-03-28 15:26 ` [FFmpeg-devel] [PATCH v7 2/2] avcodec/mjpegenc: support writing ICC profiles Niklas Haas
  2022-04-01 13:31 ` [FFmpeg-devel] [PATCH v7 1/2] avcodec/pngenc: support writing iCCP chunks Andreas Rheinhardt
  0 siblings, 2 replies; 4+ messages in thread
From: Niklas Haas @ 2022-03-28 15:26 UTC (permalink / raw)
  To: ffmpeg-devel; +Cc: 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)
+{
+    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]
-- 
2.35.1

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 4+ messages in thread
* [FFmpeg-devel] [PATCH v7 2/2] avcodec/mjpegenc: support writing ICC profiles
  2022-03-28 15:26 [FFmpeg-devel] [PATCH v7 1/2] avcodec/pngenc: support writing iCCP chunks Niklas Haas
@ 2022-03-28 15:26 ` Niklas Haas
  2022-04-01 13:31 ` [FFmpeg-devel] [PATCH v7 1/2] avcodec/pngenc: support writing iCCP chunks Andreas Rheinhardt
  1 sibling, 0 replies; 4+ messages in thread
From: Niklas Haas @ 2022-03-28 15:26 UTC (permalink / raw)
  To: ffmpeg-devel; +Cc: Niklas Haas

From: Niklas Haas <git@haasn.dev>

This is mostly straightforward. The major complication is that, as a
result of the 16-bit chunk size limitation, ICC profiles may need to be
split up into multiple chunks.

We also need to make sure to allocate enough extra space in the packet
to fit the ICC profile, so modify both mpegvideo_enc.c and ljpegenc.c to
take into account this extra overhead, failing cleanly if necessary.

Also add a FATE transcode test to ensure that the ICC profile gets
written (and read) correctly. Note that this ICC profile is smaller than
64 kB, so this doesn't test the APP2 chunk re-arranging code at all.

Signed-off-by: Niklas Haas <git@haasn.dev>
---
 libavcodec/ljpegenc.c        |  6 ++--
 libavcodec/mjpegenc.c        |  3 +-
 libavcodec/mjpegenc_common.c | 68 ++++++++++++++++++++++++++++++++++--
 libavcodec/mjpegenc_common.h |  4 ++-
 libavcodec/mpegvideo_enc.c   |  4 ++-
 tests/fate/image.mak         |  6 +++-
 tests/ref/fate/jpg-icc       | 42 ++++++++++++++++++++++
 7 files changed, 124 insertions(+), 9 deletions(-)
 create mode 100644 tests/ref/fate/jpg-icc

diff --git a/libavcodec/ljpegenc.c b/libavcodec/ljpegenc.c
index fad19cbb76..382d291621 100644
--- a/libavcodec/ljpegenc.c
+++ b/libavcodec/ljpegenc.c
@@ -220,7 +220,7 @@ static int ljpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
     const int height = avctx->height;
     const int mb_width  = (width  + s->hsample[0] - 1) / s->hsample[0];
     const int mb_height = (height + s->vsample[0] - 1) / s->vsample[0];
-    int max_pkt_size = AV_INPUT_BUFFER_MIN_SIZE;
+    size_t max_pkt_size = AV_INPUT_BUFFER_MIN_SIZE;
     int ret, header_bits;
 
     if(    avctx->pix_fmt == AV_PIX_FMT_BGR0
@@ -233,12 +233,14 @@ static int ljpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
                         * s->hsample[0] * s->vsample[0];
     }
 
+    if ((ret = ff_mjpeg_add_icc_profile_size(avctx, pict, &max_pkt_size)) < 0)
+        return ret;
     if ((ret = ff_alloc_packet(avctx, pkt, max_pkt_size)) < 0)
         return ret;
 
     init_put_bits(&pb, pkt->data, pkt->size);
 
-    ff_mjpeg_encode_picture_header(avctx, &pb, NULL, &s->scantable,
+    ff_mjpeg_encode_picture_header(avctx, &pb, pict, NULL, &s->scantable,
                                    s->pred, s->matrix, s->matrix);
 
     header_bits = put_bits_count(&pb);
diff --git a/libavcodec/mjpegenc.c b/libavcodec/mjpegenc.c
index a39b990eea..d35195d52e 100644
--- a/libavcodec/mjpegenc.c
+++ b/libavcodec/mjpegenc.c
@@ -80,7 +80,7 @@ static av_cold void init_uni_ac_vlc(const uint8_t huff_size_ac[256],
 
 static void mjpeg_encode_picture_header(MpegEncContext *s)
 {
-    ff_mjpeg_encode_picture_header(s->avctx, &s->pb, s->mjpeg_ctx,
+    ff_mjpeg_encode_picture_header(s->avctx, &s->pb, s->picture->f, s->mjpeg_ctx,
                                    &s->intra_scantable, 0,
                                    s->intra_matrix, s->chroma_intra_matrix);
 
@@ -130,6 +130,7 @@ static void mjpeg_encode_picture_frame(MpegEncContext *s)
     }
 
     bytes_needed = (total_bits + 7) / 8;
+    ff_mjpeg_add_icc_profile_size(s->avctx, s->picture->f, &bytes_needed);
     ff_mpv_reallocate_putbitbuffer(s, bytes_needed, bytes_needed);
 
     for (int i = 0; i < m->huff_ncode; i++) {
diff --git a/libavcodec/mjpegenc_common.c b/libavcodec/mjpegenc_common.c
index 7b82644763..8f246c96d5 100644
--- a/libavcodec/mjpegenc_common.c
+++ b/libavcodec/mjpegenc_common.c
@@ -131,8 +131,41 @@ static void jpeg_table_header(AVCodecContext *avctx, PutBitContext *p,
     AV_WB16(ptr, size);
 }
 
-static void jpeg_put_comments(AVCodecContext *avctx, PutBitContext *p)
+enum {
+    ICC_HDR_SIZE    = 16, /* ICC_PROFILE\0 tag + 4 bytes */
+    ICC_CHUNK_SIZE  = UINT16_MAX - ICC_HDR_SIZE,
+    ICC_MAX_CHUNKS  = UINT8_MAX,
+};
+
+int ff_mjpeg_add_icc_profile_size(AVCodecContext *avctx, const AVFrame *frame,
+                                  size_t *max_pkt_size)
 {
+    const AVFrameSideData *sd;
+    size_t new_pkt_size;
+    int nb_chunks;
+    sd = av_frame_get_side_data(frame, AV_FRAME_DATA_ICC_PROFILE);
+    if (!sd || !sd->size)
+        return 0;
+
+    if (sd->size > ICC_MAX_CHUNKS * ICC_CHUNK_SIZE) {
+        av_log(avctx, AV_LOG_ERROR, "Cannot store %"SIZE_SPECIFIER" byte ICC "
+               "profile: too large for JPEG\n",
+               sd->size);
+        return AVERROR_INVALIDDATA;
+    }
+
+    nb_chunks = (sd->size + ICC_CHUNK_SIZE - 1) / ICC_CHUNK_SIZE;
+    new_pkt_size = *max_pkt_size + nb_chunks * (UINT16_MAX + 2 /* APP2 marker */);
+    if (new_pkt_size < *max_pkt_size) /* overflow */
+        return AVERROR_INVALIDDATA;
+    *max_pkt_size = new_pkt_size;
+    return 0;
+}
+
+static void jpeg_put_comments(AVCodecContext *avctx, PutBitContext *p,
+                              const AVFrame *frame)
+{
+    const AVFrameSideData *sd = NULL;
     int size;
     uint8_t *ptr;
 
@@ -162,6 +195,35 @@ static void jpeg_put_comments(AVCodecContext *avctx, PutBitContext *p)
         put_bits(p, 8, 0); /* thumbnail height */
     }
 
+    /* ICC profile */
+    sd = av_frame_get_side_data(frame, AV_FRAME_DATA_ICC_PROFILE);
+    if (sd && sd->size) {
+        const int nb_chunks = (sd->size + ICC_CHUNK_SIZE - 1) / ICC_CHUNK_SIZE;
+        const uint8_t *data = sd->data;
+        size_t remaining = sd->size;
+        /* must already be checked by the packat allocation code */
+        av_assert0(remaining <= ICC_MAX_CHUNKS * ICC_CHUNK_SIZE);
+        flush_put_bits(p);
+        for (int i = 0; i < nb_chunks; i++) {
+            size = FFMIN(remaining, ICC_CHUNK_SIZE);
+            av_assert1(size > 0);
+            ptr = put_bits_ptr(p);
+            ptr[0] = 0xff; /* chunk marker, not part of ICC_HDR_SIZE */
+            ptr[1] = APP2;
+            AV_WB16(ptr+2, size + ICC_HDR_SIZE);
+            AV_WL32(ptr+4,  MKTAG('I','C','C','_'));
+            AV_WL32(ptr+8,  MKTAG('P','R','O','F'));
+            AV_WL32(ptr+12, MKTAG('I','L','E','\0'));
+            ptr[16] = i+1;
+            ptr[17] = nb_chunks;
+            memcpy(&ptr[18], data, size);
+            skip_put_bytes(p, size + ICC_HDR_SIZE + 2);
+            remaining -= size;
+            data += size;
+        }
+        av_assert1(!remaining);
+    }
+
     /* comment */
     if (!(avctx->flags & AV_CODEC_FLAG_BITEXACT)) {
         put_marker(p, COM);
@@ -214,7 +276,7 @@ void ff_mjpeg_init_hvsample(AVCodecContext *avctx, int hsample[4], int vsample[4
 }
 
 void ff_mjpeg_encode_picture_header(AVCodecContext *avctx, PutBitContext *pb,
-                                    MJpegContext *m,
+                                    const AVFrame *frame, struct MJpegContext *m,
                                     ScanTable *intra_scantable, int pred,
                                     uint16_t luma_intra_matrix[64],
                                     uint16_t chroma_intra_matrix[64])
@@ -234,7 +296,7 @@ void ff_mjpeg_encode_picture_header(AVCodecContext *avctx, PutBitContext *pb,
     if (avctx->codec_id == AV_CODEC_ID_AMV)
         return;
 
-    jpeg_put_comments(avctx, pb);
+    jpeg_put_comments(avctx, pb, frame);
 
     jpeg_table_header(avctx, pb, m, intra_scantable,
                       luma_intra_matrix, chroma_intra_matrix, hsample);
diff --git a/libavcodec/mjpegenc_common.h b/libavcodec/mjpegenc_common.h
index ac753bf153..69e132f223 100644
--- a/libavcodec/mjpegenc_common.h
+++ b/libavcodec/mjpegenc_common.h
@@ -29,8 +29,10 @@
 
 struct MJpegContext;
 
+int ff_mjpeg_add_icc_profile_size(AVCodecContext *avctx, const AVFrame *frame,
+                                  size_t *max_pkt_size);
 void ff_mjpeg_encode_picture_header(AVCodecContext *avctx, PutBitContext *pb,
-                                    struct MJpegContext *m,
+                                    const AVFrame *frame, struct MJpegContext *m,
                                     ScanTable *intra_scantable, int pred,
                                     uint16_t luma_intra_matrix[64],
                                     uint16_t chroma_intra_matrix[64]);
diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c
index 71c999fab0..7fe889dec3 100644
--- a/libavcodec/mpegvideo_enc.c
+++ b/libavcodec/mpegvideo_enc.c
@@ -1689,9 +1689,11 @@ int ff_mpv_encode_picture(AVCodecContext *avctx, AVPacket *pkt,
     /* output? */
     if (s->new_picture.f->data[0]) {
         int growing_buffer = context_count == 1 && !pkt->data && !s->data_partitioning;
-        int pkt_size = growing_buffer ? FFMAX(s->mb_width*s->mb_height*64+10000, avctx->internal->byte_buffer_size) - AV_INPUT_BUFFER_PADDING_SIZE
+        size_t pkt_size = growing_buffer ? FFMAX(s->mb_width*s->mb_height*64+10000, avctx->internal->byte_buffer_size) - AV_INPUT_BUFFER_PADDING_SIZE
                                               :
                                               s->mb_width*s->mb_height*(MAX_MB_BYTES+100)+10000;
+        if ((ret = ff_mjpeg_add_icc_profile_size(avctx, s->new_picture.f, &pkt_size)) < 0)
+            return ret;
         if ((ret = ff_alloc_packet(avctx, pkt, pkt_size)) < 0)
             return ret;
         if (s->mb_info) {
diff --git a/tests/fate/image.mak b/tests/fate/image.mak
index c6374c7d8a..70be281411 100644
--- a/tests/fate/image.mak
+++ b/tests/fate/image.mak
@@ -337,9 +337,13 @@ fate-jpg-12bpp: CMD = framecrc -idct simple -i $(TARGET_SAMPLES)/jpg/12bpp.jpg -
 FATE_JPG += fate-jpg-jfif
 fate-jpg-jfif: CMD = framecrc -idct simple -i $(TARGET_SAMPLES)/jpg/20242.jpg
 
+FATE_JPG_TRANSCODE-$(call ENCDEC, MJPEG, IMAGE2) += fate-jpg-icc
+fate-jpg-icc: CMD = transcode png_pipe $(TARGET_SAMPLES)/png1/lena-int_rgb24.png mjpeg "-vf scale" "" "" "-show_frames"
+
 FATE_JPG-$(call DEMDEC, IMAGE2, MJPEG) += $(FATE_JPG)
 FATE_IMAGE += $(FATE_JPG-yes)
-fate-jpg: $(FATE_JPG-yes)
+FATE_IMAGE_TRANSCODE += $(FATE_JPG_TRANSCODE-yes)
+fate-jpg: $(FATE_JPG-yes) $(FATE_JPG_TRANSCODE-yes)
 
 FATE_JPEGLS += fate-jpegls-2bpc
 fate-jpegls-2bpc: CMD = framecrc -idct simple -i $(TARGET_SAMPLES)/jpegls/4.jls
diff --git a/tests/ref/fate/jpg-icc b/tests/ref/fate/jpg-icc
new file mode 100644
index 0000000000..220146555e
--- /dev/null
+++ b/tests/ref/fate/jpg-icc
@@ -0,0 +1,42 @@
+0a323df5cdfb9574e329b9831be054a6 *tests/data/fate/jpg-icc.mjpeg
+11010 tests/data/fate/jpg-icc.mjpeg
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 128x128
+#sar 0: 1/1
+0,          0,          0,        1,    49152, 0xaac06b42
+[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=11010
+width=128
+height=128
+pix_fmt=yuvj444p
+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=bt470bg
+color_primaries=unknown
+color_transfer=unknown
+chroma_location=center
+[SIDE_DATA]
+side_data_type=ICC profile
+size=3144
+[/SIDE_DATA]
+[/FRAME]
-- 
2.35.1

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [FFmpeg-devel] [PATCH v7 1/2] avcodec/pngenc: support writing iCCP chunks
  2022-03-28 15:26 [FFmpeg-devel] [PATCH v7 1/2] avcodec/pngenc: support writing iCCP chunks Niklas Haas
  2022-03-28 15:26 ` [FFmpeg-devel] [PATCH v7 2/2] avcodec/mjpegenc: support writing ICC profiles Niklas Haas
@ 2022-04-01 13:31 ` Andreas Rheinhardt
  2022-04-05 10:59   ` Niklas Haas
  1 sibling, 1 reply; 4+ messages in thread
From: Andreas Rheinhardt @ 2022-04-01 13:31 UTC (permalink / raw)
  To: ffmpeg-devel

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]

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [FFmpeg-devel] [PATCH v7 1/2] avcodec/pngenc: support writing iCCP chunks
  2022-04-01 13:31 ` [FFmpeg-devel] [PATCH v7 1/2] avcodec/pngenc: support writing iCCP chunks Andreas Rheinhardt
@ 2022-04-05 10:59   ` Niklas Haas
  0 siblings, 0 replies; 4+ messages in thread
From: Niklas Haas @ 2022-04-05 10:59 UTC (permalink / raw)
  To: ffmpeg-devel

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).
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-05 10:59 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-28 15:26 [FFmpeg-devel] [PATCH v7 1/2] avcodec/pngenc: support writing iCCP chunks Niklas Haas
2022-03-28 15:26 ` [FFmpeg-devel] [PATCH v7 2/2] avcodec/mjpegenc: support writing ICC profiles Niklas Haas
2022-04-01 13:31 ` [FFmpeg-devel] [PATCH v7 1/2] avcodec/pngenc: support writing iCCP chunks Andreas Rheinhardt
2022-04-05 10:59   ` Niklas Haas

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git