[FFmpeg-devel] ffmpeg: copy the extradata from encoder to muxer

On 27.10.2016 21:37, James Almer wrote:
> If apng encoder needs to add new extradata in a packet, it should do it
> with av_packet_new_side_data() instead.

Thanks for the hint. Attached is a patch fixing it that way.

Best regards,
Andreas

On 10/27/2016 5:38 PM, Andreas Cadhalpun wrote:
> On 27.10.2016 21:37, James Almer wrote:
>> > If apng encoder needs to add new extradata in a packet, it should do it
>> > with av_packet_new_side_data() instead.
> Thanks for the hint. Attached is a patch fixing it that way.
> 
> Best regards,
> Andreas
> 
> 
> 0001-apng-use-side-data-to-pass-extradata-to-muxer.patch
> 
> 
> From 4325ccdaf3bb7c74312cbaeb49d76fe535f18956 Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> Date: Thu, 27 Oct 2016 22:34:48 +0200
> Subject: [PATCH] apng: use side data to pass extradata to muxer
> 
> This fixes creating apng files, which is broken since commit
> 5ef19590802f000299e418143fc2301e3f43affe.
> 
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> ---
>  libavcodec/pngenc.c   |  4 ++++
>  libavformat/apngenc.c | 27 ++++++++++++++++++++++++---
>  2 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c
> index 00c830e..1a308f2 100644
> --- a/libavcodec/pngenc.c
> +++ b/libavcodec/pngenc.c
> @@ -917,6 +917,10 @@ static int encode_apng(AVCodecContext *avctx, AVPacket *pkt,
>      if (s->last_frame) {
>          uint8_t* last_fctl_chunk_start = pkt->data;
>          uint8_t buf[26];
> +        uint8_t *side_data = av_packet_new_side_data(pkt, AV_PKT_DATA_NEW_EXTRADATA, avctx->extradata_size);

You could add a variable called extradata_updated or so to PNGEncContext and set it to
1 here so the encoder doesn't add side data to every packet when it's only needed for
the first.

> +        if (!side_data)
> +            return AVERROR(ENOMEM);
> +        memcpy(side_data, avctx->extradata, avctx->extradata_size);
>  
>          AV_WB32(buf + 0, s->last_frame_fctl.sequence_number);
>          AV_WB32(buf + 4, s->last_frame_fctl.width);
> diff --git a/libavformat/apngenc.c b/libavformat/apngenc.c
> index e25df2e..f702667 100644
> --- a/libavformat/apngenc.c
> +++ b/libavformat/apngenc.c
> @@ -101,15 +101,29 @@ static int apng_write_header(AVFormatContext *format_context)
>      return 0;
>  }
>  
> -static void flush_packet(AVFormatContext *format_context, AVPacket *packet)
> +static int flush_packet(AVFormatContext *format_context, AVPacket *packet)
>  {
>      APNGMuxContext *apng = format_context->priv_data;
>      AVIOContext *io_context = format_context->pb;
>      AVStream *codec_stream = format_context->streams[0];
>      AVCodecParameters *codec_par = codec_stream->codecpar;
> +    uint8_t *side_data = NULL;
> +    int side_data_size = 0;
>  
>      av_assert0(apng->prev_packet);
>  
> +    if (packet)
> +        side_data = av_packet_get_side_data(packet, AV_PKT_DATA_NEW_EXTRADATA, &side_data_size);

If the muxer gets only one frame, the code below (standard png mode fallback) will not
work. You can test this with "./ffmpeg -f lavfi -i testsrc=s=32x32 -vframes 1 apng.apng".

Don't check for packet and use apng->prev_packet unconditionally instead. That should
do it.

> +
> +    if (side_data_size) {
> +        av_freep(&codec_par->extradata);
> +        codec_par->extradata = av_mallocz(side_data_size + AV_INPUT_BUFFER_PADDING_SIZE);
> +        if (!codec_par->extradata)
> +            return AVERROR(ENOMEM);
> +        codec_par->extradata_size = side_data_size;
> +        memcpy(codec_par->extradata, side_data, codec_par->extradata_size);
> +    }
> +
>      if (apng->frame_number == 0 && !packet) {
>          uint8_t *existing_acTL_chunk;
>          uint8_t *existing_fcTL_chunk;
> @@ -195,11 +209,13 @@ static void flush_packet(AVFormatContext *format_context, AVPacket *packet)
>      av_packet_unref(apng->prev_packet);
>      if (packet)
>          av_copy_packet(apng->prev_packet, packet);
> +    return 0;
>  }
>  
>  static int apng_write_packet(AVFormatContext *format_context, AVPacket *packet)
>  {
>      APNGMuxContext *apng = format_context->priv_data;
> +    int ret;
>  
>      if (!apng->prev_packet) {
>          apng->prev_packet = av_malloc(sizeof(*apng->prev_packet));
> @@ -208,7 +224,9 @@ static int apng_write_packet(AVFormatContext *format_context, AVPacket *packet)
>  
>          av_copy_packet(apng->prev_packet, packet);
>      } else {
> -        flush_packet(format_context, packet);
> +        ret = flush_packet(format_context, packet);
> +        if (ret < 0)
> +            return ret;
>      }
>  
>      return 0;
> @@ -219,10 +237,13 @@ static int apng_write_trailer(AVFormatContext *format_context)
>      APNGMuxContext *apng = format_context->priv_data;
>      AVIOContext *io_context = format_context->pb;
>      uint8_t buf[8];
> +    int ret;
>  
>      if (apng->prev_packet) {
> -        flush_packet(format_context, NULL);
> +        ret = flush_packet(format_context, NULL);
>          av_freep(&apng->prev_packet);
> +        if (ret < 0)
> +            return ret;
>      }
>  
>      apng_write_chunk(io_context, MKBETAG('I', 'E', 'N', 'D'), NULL, 0);
> -- 2.9.3

Ideally, you'd not use avctx->extradata* in the apng encoder or codecpar->extradata*
in the muxer. The former should only be written by the init() function, and the latter
should afaik not be modified by the muxer at all.
If you can store the pointers and sizes for the extradata in the encoder/muxer contexts
and use them instead of avctx and codecpar extradata then that'd be great.