Re: [FFmpeg-devel] [PATCH v2] avcodec/mjpegenc: support writing ICC profiles

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

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.
> 
> 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>
> ---
> Changes in v2:
> - Merge FATE test into this commit
> - Fix possible segfault (when no AVFrame is available)

When could this ever happen? IMO it can't.

> ---
>  libavcodec/ljpegenc.c        |  6 ++++-
>  libavcodec/mjpegenc.c        |  3 ++-
>  libavcodec/mjpegenc_common.c | 43 +++++++++++++++++++++++++++++++++---
>  libavcodec/mjpegenc_common.h |  2 +-
>  libavcodec/mpegvideo_enc.c   |  3 +++
>  tests/fate/image.mak         |  3 +++
>  tests/ref/fate/jpg-icc       | 42 +++++++++++++++++++++++++++++++++++
>  7 files changed, 96 insertions(+), 6 deletions(-)
>  create mode 100644 tests/ref/fate/jpg-icc
> 
> diff --git a/libavcodec/ljpegenc.c b/libavcodec/ljpegenc.c
> index e15f448f90..c54450c338 100644
> --- a/libavcodec/ljpegenc.c
> +++ b/libavcodec/ljpegenc.c
> @@ -216,6 +216,7 @@ static int ljpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
>  {
>      LJpegEncContext *s = avctx->priv_data;
>      PutBitContext pb;
> +    const AVFrameSideData *sd;
>      const int width  = avctx->width;
>      const int height = avctx->height;
>      const int mb_width  = (width  + s->hsample[0] - 1) / s->hsample[0];
> @@ -233,12 +234,15 @@ static int ljpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
>                          * s->hsample[0] * s->vsample[0];
>      }
>  
> +    if ((sd = av_frame_get_side_data(pict, AV_FRAME_DATA_ICC_PROFILE)))
> +        max_pkt_size += sd->size;

This does not account for the chunk overhead.

> +
>      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 08671b0df7..d7c0c763a1 100644
> --- a/libavcodec/mjpegenc.c
> +++ b/libavcodec/mjpegenc.c
> @@ -77,7 +77,8 @@ 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,
> +    const AVFrame *frame = s->picture->f;
> +    ff_mjpeg_encode_picture_header(s->avctx, &s->pb, frame, s->mjpeg_ctx,
>                                     &s->intra_scantable, 0,
>                                     s->intra_matrix, s->chroma_intra_matrix);
>  
> diff --git a/libavcodec/mjpegenc_common.c b/libavcodec/mjpegenc_common.c
> index 995e2b7670..5594a8d239 100644
> --- a/libavcodec/mjpegenc_common.c
> +++ b/libavcodec/mjpegenc_common.c
> @@ -130,8 +130,10 @@ static void jpeg_table_header(AVCodecContext *avctx, PutBitContext *p,
>      AV_WB16(ptr, size);
>  }
>  
> -static void jpeg_put_comments(AVCodecContext *avctx, PutBitContext *p)
> +static void jpeg_put_comments(AVCodecContext *avctx, PutBitContext *p,
> +                              const AVFrame *frame)
>  {
> +    const AVFrameSideData *sd = NULL;
>      int size;
>      uint8_t *ptr;
>  
> @@ -161,6 +163,41 @@ static void jpeg_put_comments(AVCodecContext *avctx, PutBitContext *p)
>          put_bits(p, 8, 0); /* thumbnail height */
>      }
>  
> +    /* ICC profile */
> +    if (frame)
> +        sd = av_frame_get_side_data(frame, AV_FRAME_DATA_ICC_PROFILE);
> +    if (sd && sd->size) {
> +        static const uint8_t hdr_size = strlen("ICC_PROFILE")+5;

This is illegal as initializers for static objects need to be constant
expressions or string literals. Which this thing isn't. You are
basically presuming that strlen be evaluated at compile-time.

> +        static const uint16_t max_chunk_size = UINT16_MAX - hdr_size;
> +        const uint8_t *data = sd->data;
> +        size_t remaining = sd->size;
> +        size_t nb_chunks = (remaining + max_chunk_size - 1) / max_chunk_size;

There is a potential overflow here. You could avoid it by checking
whether remaining is > 255 * max_chunk_size.

> +        if (nb_chunks > 255) {
> +            av_log(avctx, AV_LOG_WARNING,
> +                "Cannot store %zu byte ICC profile: too large for JPEG\n",

We use SIZE_SPECIFIER for compatibility with old versions of MSVC. Note
that I do not know whether this is still needed, as all currently
supported MSVC versions might already support #zu.

> +                sd->size);
> +            nb_chunks = remaining = 0;
> +        }
> +
> +        for (int i = 0; i < nb_chunks; i++) {
> +            size = FFMIN(remaining, max_chunk_size);
> +            av_assert1(size > 0);
> +            put_marker(p, APP2);
> +            put_bits(p, 16, size + hdr_size);
> +            ff_put_string(p, "ICC_PROFILE", 1);
> +            put_bits(p, 8, i+1);
> +            put_bits(p, 8, nb_chunks);
> +            flush_put_bits(p);
> +            ptr = put_bits_ptr(p);
> +            skip_put_bytes(p, size);
> +            memcpy(ptr, data, size);
> +            remaining -= size;

Everything that you are doing here is byte-aligned, so I think it would
be cleaner (and less code) to write everything directly and
skip_put_bytes once afterwards.

> +            data += size;
> +        }
> +
> +        av_assert1(!remaining);
> +    }
> +
>      /* comment */
>      if (!(avctx->flags & AV_CODEC_FLAG_BITEXACT)) {
>          put_marker(p, COM);
> @@ -213,7 +250,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])
> @@ -233,7 +270,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..3c0a7addac 100644
> --- a/libavcodec/mjpegenc_common.h
> +++ b/libavcodec/mjpegenc_common.h
> @@ -30,7 +30,7 @@
>  struct MJpegContext;
>  
>  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 c69114ea15..932dfa34f7 100644
> --- a/libavcodec/mpegvideo_enc.c
> +++ b/libavcodec/mpegvideo_enc.c
> @@ -1686,10 +1686,13 @@ int ff_mpv_encode_picture(AVCodecContext *avctx, AVPacket *pkt,
>  
>      /* output? */
>      if (s->new_picture.f->data[0]) {
> +        const AVFrameSideData *sd;
>          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
>                                                :
>                                                s->mb_width*s->mb_height*(MAX_MB_BYTES+100)+10000;
> +        if ((sd = av_frame_get_side_data(s->new_picture.f, AV_FRAME_DATA_ICC_PROFILE)))
> +            pkt_size += sd->size;

This does not account for the chunk overhead and it is also not targeted
to MJPEG.

>          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 573d398915..8b72890fbe 100644
> --- a/tests/fate/image.mak
> +++ b/tests/fate/image.mak
> @@ -337,6 +337,9 @@ 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 += fate-jpg-icc
> +fate-jpg-icc: CMD = transcode png_pipe $(TARGET_SAMPLES)/png1/lena-int_rgb24.png mjpeg "-vf scale" "" "" "-show_frames"

This needs an ffprobe dependency.

> +
>  FATE_JPG-$(call DEMDEC, IMAGE2, MJPEG) += $(FATE_JPG)
>  FATE_IMAGE += $(FATE_JPG-yes)
>  fate-jpg: $(FATE_JPG-yes)
> 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]

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