diff mbox

[FFmpeg-devel] apngdec: use side data to pass extradata to the decoder

Message ID 1e54ae6d-e660-4d7f-687d-c4d2f72ae1bc@googlemail.com
State Accepted
Headers show

Commit Message

Andreas Cadhalpun Nov. 1, 2016, 4:18 p.m. UTC
Fixes remuxing apng streams coming from the apng demuxer.
This is a regression since 940b8908b94404a65f9f55e33efb4ccc6c81383c.

Found-by: James Almer <jamrial@gmail.com>
Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavcodec/pngdec.c   | 23 ++++++++++++++--
 libavformat/apngdec.c | 73 +++++++++++++++++++++++++++++++++++----------------
 2 files changed, 71 insertions(+), 25 deletions(-)

Comments

James Almer Nov. 1, 2016, 5:26 p.m. UTC | #1
On 11/1/2016 1:18 PM, Andreas Cadhalpun wrote:
> Fixes remuxing apng streams coming from the apng demuxer.
> This is a regression since 940b8908b94404a65f9f55e33efb4ccc6c81383c.
> 
> Found-by: James Almer <jamrial@gmail.com>
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> ---
>  libavcodec/pngdec.c   | 23 ++++++++++++++--
>  libavformat/apngdec.c | 73 +++++++++++++++++++++++++++++++++++----------------
>  2 files changed, 71 insertions(+), 25 deletions(-)
> 
> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> index 36275ae..83eeb8d 100644
> --- a/libavcodec/pngdec.c
> +++ b/libavcodec/pngdec.c
> @@ -45,6 +45,9 @@ typedef struct PNGDecContext {
>      ThreadFrame last_picture;
>      ThreadFrame picture;
>  
> +    uint8_t* extra_data;
> +    int extra_data_size;
> +
>      int state;
>      int width, height;
>      int cur_w, cur_h;
> @@ -1361,14 +1364,28 @@ static int decode_frame_apng(AVCodecContext *avctx,
>      p = s->picture.f;
>  
>      if (!(s->state & PNG_IHDR)) {
> -        if (!avctx->extradata_size)
> +        int side_data_size = 0;
> +        uint8_t *side_data = NULL;
> +        if (avpkt)
> +            side_data = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, &side_data_size);
> +
> +        if (side_data_size) {
> +            av_freep(&s->extra_data);
> +            s->extra_data = av_mallocz(side_data_size + AV_INPUT_BUFFER_PADDING_SIZE);
> +            if (!s->extra_data)
> +                return AVERROR(ENOMEM);
> +            s->extra_data_size = side_data_size;
> +            memcpy(s->extra_data, side_data, s->extra_data_size);
> +        }
> +
> +        if (!s->extra_data_size)
>              return AVERROR_INVALIDDATA;
>  
>          /* only init fields, there is no zlib use in extradata */
>          s->zstream.zalloc = ff_png_zalloc;
>          s->zstream.zfree  = ff_png_zfree;
>  
> -        bytestream2_init(&s->gb, avctx->extradata, avctx->extradata_size);
> +        bytestream2_init(&s->gb, s->extra_data, s->extra_data_size);
>          if ((ret = decode_frame_common(avctx, s, p, avpkt)) < 0)
>              goto end;
>      }
> @@ -1494,6 +1511,8 @@ static av_cold int png_dec_end(AVCodecContext *avctx)
>      s->last_row_size = 0;
>      av_freep(&s->tmp_row);
>      s->tmp_row_size = 0;
> +    av_freep(&s->extra_data);
> +    s->extra_data_size = 0;
>  
>      return 0;
>  }
> diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
> index bb17896..25af3fb 100644
> --- a/libavformat/apngdec.c
> +++ b/libavformat/apngdec.c
> @@ -49,6 +49,10 @@ typedef struct APNGDemuxContext {
>  
>      int is_key_frame;
>  
> +    uint8_t *extra_data;
> +    int extra_data_size;
> +    int extra_data_updated;
> +
>      /*
>       * loop options
>       */
> @@ -122,9 +126,9 @@ end:
>      return AVPROBE_SCORE_MAX;
>  }
>  
> -static int append_extradata(AVCodecParameters *par, AVIOContext *pb, int len)
> +static int append_extradata(APNGDemuxContext *ctx, AVIOContext *pb, int len)
>  {
> -    int previous_size = par->extradata_size;
> +    int previous_size = ctx->extra_data_size;
>      int new_size, ret;
>      uint8_t *new_extradata;
>  
> @@ -132,18 +136,30 @@ static int append_extradata(AVCodecParameters *par, AVIOContext *pb, int len)
>          return AVERROR_INVALIDDATA;
>  
>      new_size = previous_size + len;
> -    new_extradata = av_realloc(par->extradata, new_size + AV_INPUT_BUFFER_PADDING_SIZE);
> +    new_extradata = av_realloc(ctx->extra_data, new_size + AV_INPUT_BUFFER_PADDING_SIZE);
>      if (!new_extradata)
>          return AVERROR(ENOMEM);
> -    par->extradata = new_extradata;
> -    par->extradata_size = new_size;
> +    ctx->extra_data = new_extradata;
> +    ctx->extra_data_size = new_size;
>  
> -    if ((ret = avio_read(pb, par->extradata + previous_size, len)) < 0)
> +    if ((ret = avio_read(pb, ctx->extra_data + previous_size, len)) < 0)
>          return ret;
>  
>      return previous_size;
>  }
>  
> +static int send_extradata(APNGDemuxContext *ctx, AVPacket *pkt)
> +{
> +    if (!ctx->extra_data_updated) {
> +        uint8_t *side_data = av_packet_new_side_data(pkt, AV_PKT_DATA_NEW_EXTRADATA, ctx->extra_data_size);
> +        if (!side_data)
> +            return AVERROR(ENOMEM);
> +        memcpy(side_data, ctx->extra_data, ctx->extra_data_size);
> +        ctx->extra_data_updated = 1;
> +    }
> +    return 0;
> +}
> +
>  static int apng_read_header(AVFormatContext *s)
>  {
>      APNGDemuxContext *ctx = s->priv_data;
> @@ -178,15 +194,15 @@ static int apng_read_header(AVFormatContext *s)
>          return ret;
>  
>      /* extradata will contain every chunk up to the first fcTL (excluded) */
> -    st->codecpar->extradata = av_malloc(len + 12 + AV_INPUT_BUFFER_PADDING_SIZE);
> -    if (!st->codecpar->extradata)
> +    ctx->extra_data = av_malloc(len + 12 + AV_INPUT_BUFFER_PADDING_SIZE);
> +    if (!ctx->extra_data)
>          return AVERROR(ENOMEM);
> -    st->codecpar->extradata_size = len + 12;
> -    AV_WB32(st->codecpar->extradata,    len);
> -    AV_WL32(st->codecpar->extradata+4,  tag);
> -    AV_WB32(st->codecpar->extradata+8,  st->codecpar->width);
> -    AV_WB32(st->codecpar->extradata+12, st->codecpar->height);
> -    if ((ret = avio_read(pb, st->codecpar->extradata+16, 9)) < 0)
> +    ctx->extra_data_size = len + 12;
> +    AV_WB32(ctx->extra_data,    len);
> +    AV_WL32(ctx->extra_data+4,  tag);
> +    AV_WB32(ctx->extra_data+8,  st->codecpar->width);
> +    AV_WB32(ctx->extra_data+12, st->codecpar->height);
> +    if ((ret = avio_read(pb, ctx->extra_data+16, 9)) < 0)
>          goto fail;
>  
>      while (!avio_feof(pb)) {
> @@ -218,11 +234,11 @@ static int apng_read_header(AVFormatContext *s)
>          switch (tag) {
>          case MKTAG('a', 'c', 'T', 'L'):
>              if ((ret = avio_seek(pb, -8, SEEK_CUR)) < 0 ||
> -                (ret = append_extradata(st->codecpar, pb, len + 12)) < 0)
> +                (ret = append_extradata(ctx, pb, len + 12)) < 0)
>                  goto fail;
>              acTL_found = 1;
> -            ctx->num_frames = AV_RB32(st->codecpar->extradata + ret + 8);
> -            ctx->num_play   = AV_RB32(st->codecpar->extradata + ret + 12);
> +            ctx->num_frames = AV_RB32(ctx->extra_data + ret + 8);
> +            ctx->num_play   = AV_RB32(ctx->extra_data + ret + 12);
>              av_log(s, AV_LOG_DEBUG, "num_frames: %"PRIu32", num_play: %"PRIu32"\n",
>                                      ctx->num_frames, ctx->num_play);
>              break;
> @@ -236,15 +252,15 @@ static int apng_read_header(AVFormatContext *s)
>              return 0;
>          default:
>              if ((ret = avio_seek(pb, -8, SEEK_CUR)) < 0 ||
> -                (ret = append_extradata(st->codecpar, pb, len + 12)) < 0)
> +                (ret = append_extradata(ctx, pb, len + 12)) < 0)
>                  goto fail;
>          }
>      }
>  
>  fail:
> -    if (st->codecpar->extradata_size) {
> -        av_freep(&st->codecpar->extradata);
> -        st->codecpar->extradata_size = 0;
> +    if (ctx->extra_data_size) {
> +        av_freep(&ctx->extra_data);
> +        ctx->extra_data_size = 0;
>      }
>      return ret;
>  }
> @@ -393,6 +409,7 @@ static int apng_read_packet(AVFormatContext *s, AVPacket *pkt)
>          pkt->pts = ctx->pkt_pts;
>          pkt->duration = ctx->pkt_duration;
>          ctx->pkt_pts += ctx->pkt_duration;
> +        ret = send_extradata(ctx, pkt);
>          return ret;
>      case MKTAG('I', 'E', 'N', 'D'):
>          ctx->cur_loop++;
> @@ -400,9 +417,10 @@ static int apng_read_packet(AVFormatContext *s, AVPacket *pkt)
>              avio_seek(pb, -8, SEEK_CUR);
>              return AVERROR_EOF;
>          }
> -        if ((ret = avio_seek(pb, s->streams[0]->codecpar->extradata_size + 8, SEEK_SET)) < 0)
> +        if ((ret = avio_seek(pb, ctx->extra_data_size + 8, SEEK_SET)) < 0)
>              return ret;
> -        return 0;
> +        ret = send_extradata(ctx, pkt);
> +        return ret;

return send_extradata(ctx, pkt);

>      default:
>          {
>          char tag_buf[32];
> @@ -417,6 +435,14 @@ static int apng_read_packet(AVFormatContext *s, AVPacket *pkt)
>      return AVERROR_PATCHWELCOME;
>  }
>  
> +static int apng_read_close(AVFormatContext *s)
> +{
> +    APNGDemuxContext *ctx = s->priv_data;
> +    av_freep(&ctx->extra_data);
> +    ctx->extra_data_size = 0;
> +    return 0;
> +}
> +
>  static const AVOption options[] = {
>      { "ignore_loop", "ignore loop setting"                         , offsetof(APNGDemuxContext, ignore_loop),
>        AV_OPT_TYPE_BOOL, { .i64 = 1 }              , 0, 1      , AV_OPT_FLAG_DECODING_PARAM },
> @@ -442,6 +468,7 @@ AVInputFormat ff_apng_demuxer = {
>      .read_probe     = apng_probe,
>      .read_header    = apng_read_header,
>      .read_packet    = apng_read_packet,
> +    .read_close     = apng_read_close,
>      .flags          = AVFMT_GENERIC_INDEX,
>      .priv_class     = &demuxer_class,
>  };
> 

Seems to work, so LGTM.
Andreas Cadhalpun Nov. 1, 2016, 6:05 p.m. UTC | #2
On 01.11.2016 18:26, James Almer wrote:
> On 11/1/2016 1:18 PM, Andreas Cadhalpun wrote:
>> Fixes remuxing apng streams coming from the apng demuxer.
>> This is a regression since 940b8908b94404a65f9f55e33efb4ccc6c81383c.
>>
>> Found-by: James Almer <jamrial@gmail.com>
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> ---
>>  libavcodec/pngdec.c   | 23 ++++++++++++++--
>>  libavformat/apngdec.c | 73 +++++++++++++++++++++++++++++++++++----------------
>>  2 files changed, 71 insertions(+), 25 deletions(-)
>>
>> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
>> index 36275ae..83eeb8d 100644
>> @@ -393,6 +409,7 @@ static int apng_read_packet(AVFormatContext *s, AVPacket *pkt)
>>          pkt->pts = ctx->pkt_pts;
>>          pkt->duration = ctx->pkt_duration;
>>          ctx->pkt_pts += ctx->pkt_duration;
>> +        ret = send_extradata(ctx, pkt);
>>          return ret;
>>      case MKTAG('I', 'E', 'N', 'D'):
>>          ctx->cur_loop++;
>> @@ -400,9 +417,10 @@ static int apng_read_packet(AVFormatContext *s, AVPacket *pkt)
>>              avio_seek(pb, -8, SEEK_CUR);
>>              return AVERROR_EOF;
>>          }
>> -        if ((ret = avio_seek(pb, s->streams[0]->codecpar->extradata_size + 8, SEEK_SET)) < 0)
>> +        if ((ret = avio_seek(pb, ctx->extra_data_size + 8, SEEK_SET)) < 0)
>>              return ret;
>> -        return 0;
>> +        ret = send_extradata(ctx, pkt);
>> +        return ret;
> 
> return send_extradata(ctx, pkt);

Changed.

> Seems to work, so LGTM.

Pushed.

Best regards,
Andreas
diff mbox

Patch

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 36275ae..83eeb8d 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -45,6 +45,9 @@  typedef struct PNGDecContext {
     ThreadFrame last_picture;
     ThreadFrame picture;
 
+    uint8_t* extra_data;
+    int extra_data_size;
+
     int state;
     int width, height;
     int cur_w, cur_h;
@@ -1361,14 +1364,28 @@  static int decode_frame_apng(AVCodecContext *avctx,
     p = s->picture.f;
 
     if (!(s->state & PNG_IHDR)) {
-        if (!avctx->extradata_size)
+        int side_data_size = 0;
+        uint8_t *side_data = NULL;
+        if (avpkt)
+            side_data = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, &side_data_size);
+
+        if (side_data_size) {
+            av_freep(&s->extra_data);
+            s->extra_data = av_mallocz(side_data_size + AV_INPUT_BUFFER_PADDING_SIZE);
+            if (!s->extra_data)
+                return AVERROR(ENOMEM);
+            s->extra_data_size = side_data_size;
+            memcpy(s->extra_data, side_data, s->extra_data_size);
+        }
+
+        if (!s->extra_data_size)
             return AVERROR_INVALIDDATA;
 
         /* only init fields, there is no zlib use in extradata */
         s->zstream.zalloc = ff_png_zalloc;
         s->zstream.zfree  = ff_png_zfree;
 
-        bytestream2_init(&s->gb, avctx->extradata, avctx->extradata_size);
+        bytestream2_init(&s->gb, s->extra_data, s->extra_data_size);
         if ((ret = decode_frame_common(avctx, s, p, avpkt)) < 0)
             goto end;
     }
@@ -1494,6 +1511,8 @@  static av_cold int png_dec_end(AVCodecContext *avctx)
     s->last_row_size = 0;
     av_freep(&s->tmp_row);
     s->tmp_row_size = 0;
+    av_freep(&s->extra_data);
+    s->extra_data_size = 0;
 
     return 0;
 }
diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
index bb17896..25af3fb 100644
--- a/libavformat/apngdec.c
+++ b/libavformat/apngdec.c
@@ -49,6 +49,10 @@  typedef struct APNGDemuxContext {
 
     int is_key_frame;
 
+    uint8_t *extra_data;
+    int extra_data_size;
+    int extra_data_updated;
+
     /*
      * loop options
      */
@@ -122,9 +126,9 @@  end:
     return AVPROBE_SCORE_MAX;
 }
 
-static int append_extradata(AVCodecParameters *par, AVIOContext *pb, int len)
+static int append_extradata(APNGDemuxContext *ctx, AVIOContext *pb, int len)
 {
-    int previous_size = par->extradata_size;
+    int previous_size = ctx->extra_data_size;
     int new_size, ret;
     uint8_t *new_extradata;
 
@@ -132,18 +136,30 @@  static int append_extradata(AVCodecParameters *par, AVIOContext *pb, int len)
         return AVERROR_INVALIDDATA;
 
     new_size = previous_size + len;
-    new_extradata = av_realloc(par->extradata, new_size + AV_INPUT_BUFFER_PADDING_SIZE);
+    new_extradata = av_realloc(ctx->extra_data, new_size + AV_INPUT_BUFFER_PADDING_SIZE);
     if (!new_extradata)
         return AVERROR(ENOMEM);
-    par->extradata = new_extradata;
-    par->extradata_size = new_size;
+    ctx->extra_data = new_extradata;
+    ctx->extra_data_size = new_size;
 
-    if ((ret = avio_read(pb, par->extradata + previous_size, len)) < 0)
+    if ((ret = avio_read(pb, ctx->extra_data + previous_size, len)) < 0)
         return ret;
 
     return previous_size;
 }
 
+static int send_extradata(APNGDemuxContext *ctx, AVPacket *pkt)
+{
+    if (!ctx->extra_data_updated) {
+        uint8_t *side_data = av_packet_new_side_data(pkt, AV_PKT_DATA_NEW_EXTRADATA, ctx->extra_data_size);
+        if (!side_data)
+            return AVERROR(ENOMEM);
+        memcpy(side_data, ctx->extra_data, ctx->extra_data_size);
+        ctx->extra_data_updated = 1;
+    }
+    return 0;
+}
+
 static int apng_read_header(AVFormatContext *s)
 {
     APNGDemuxContext *ctx = s->priv_data;
@@ -178,15 +194,15 @@  static int apng_read_header(AVFormatContext *s)
         return ret;
 
     /* extradata will contain every chunk up to the first fcTL (excluded) */
-    st->codecpar->extradata = av_malloc(len + 12 + AV_INPUT_BUFFER_PADDING_SIZE);
-    if (!st->codecpar->extradata)
+    ctx->extra_data = av_malloc(len + 12 + AV_INPUT_BUFFER_PADDING_SIZE);
+    if (!ctx->extra_data)
         return AVERROR(ENOMEM);
-    st->codecpar->extradata_size = len + 12;
-    AV_WB32(st->codecpar->extradata,    len);
-    AV_WL32(st->codecpar->extradata+4,  tag);
-    AV_WB32(st->codecpar->extradata+8,  st->codecpar->width);
-    AV_WB32(st->codecpar->extradata+12, st->codecpar->height);
-    if ((ret = avio_read(pb, st->codecpar->extradata+16, 9)) < 0)
+    ctx->extra_data_size = len + 12;
+    AV_WB32(ctx->extra_data,    len);
+    AV_WL32(ctx->extra_data+4,  tag);
+    AV_WB32(ctx->extra_data+8,  st->codecpar->width);
+    AV_WB32(ctx->extra_data+12, st->codecpar->height);
+    if ((ret = avio_read(pb, ctx->extra_data+16, 9)) < 0)
         goto fail;
 
     while (!avio_feof(pb)) {
@@ -218,11 +234,11 @@  static int apng_read_header(AVFormatContext *s)
         switch (tag) {
         case MKTAG('a', 'c', 'T', 'L'):
             if ((ret = avio_seek(pb, -8, SEEK_CUR)) < 0 ||
-                (ret = append_extradata(st->codecpar, pb, len + 12)) < 0)
+                (ret = append_extradata(ctx, pb, len + 12)) < 0)
                 goto fail;
             acTL_found = 1;
-            ctx->num_frames = AV_RB32(st->codecpar->extradata + ret + 8);
-            ctx->num_play   = AV_RB32(st->codecpar->extradata + ret + 12);
+            ctx->num_frames = AV_RB32(ctx->extra_data + ret + 8);
+            ctx->num_play   = AV_RB32(ctx->extra_data + ret + 12);
             av_log(s, AV_LOG_DEBUG, "num_frames: %"PRIu32", num_play: %"PRIu32"\n",
                                     ctx->num_frames, ctx->num_play);
             break;
@@ -236,15 +252,15 @@  static int apng_read_header(AVFormatContext *s)
             return 0;
         default:
             if ((ret = avio_seek(pb, -8, SEEK_CUR)) < 0 ||
-                (ret = append_extradata(st->codecpar, pb, len + 12)) < 0)
+                (ret = append_extradata(ctx, pb, len + 12)) < 0)
                 goto fail;
         }
     }
 
 fail:
-    if (st->codecpar->extradata_size) {
-        av_freep(&st->codecpar->extradata);
-        st->codecpar->extradata_size = 0;
+    if (ctx->extra_data_size) {
+        av_freep(&ctx->extra_data);
+        ctx->extra_data_size = 0;
     }
     return ret;
 }
@@ -393,6 +409,7 @@  static int apng_read_packet(AVFormatContext *s, AVPacket *pkt)
         pkt->pts = ctx->pkt_pts;
         pkt->duration = ctx->pkt_duration;
         ctx->pkt_pts += ctx->pkt_duration;
+        ret = send_extradata(ctx, pkt);
         return ret;
     case MKTAG('I', 'E', 'N', 'D'):
         ctx->cur_loop++;
@@ -400,9 +417,10 @@  static int apng_read_packet(AVFormatContext *s, AVPacket *pkt)
             avio_seek(pb, -8, SEEK_CUR);
             return AVERROR_EOF;
         }
-        if ((ret = avio_seek(pb, s->streams[0]->codecpar->extradata_size + 8, SEEK_SET)) < 0)
+        if ((ret = avio_seek(pb, ctx->extra_data_size + 8, SEEK_SET)) < 0)
             return ret;
-        return 0;
+        ret = send_extradata(ctx, pkt);
+        return ret;
     default:
         {
         char tag_buf[32];
@@ -417,6 +435,14 @@  static int apng_read_packet(AVFormatContext *s, AVPacket *pkt)
     return AVERROR_PATCHWELCOME;
 }
 
+static int apng_read_close(AVFormatContext *s)
+{
+    APNGDemuxContext *ctx = s->priv_data;
+    av_freep(&ctx->extra_data);
+    ctx->extra_data_size = 0;
+    return 0;
+}
+
 static const AVOption options[] = {
     { "ignore_loop", "ignore loop setting"                         , offsetof(APNGDemuxContext, ignore_loop),
       AV_OPT_TYPE_BOOL, { .i64 = 1 }              , 0, 1      , AV_OPT_FLAG_DECODING_PARAM },
@@ -442,6 +468,7 @@  AVInputFormat ff_apng_demuxer = {
     .read_probe     = apng_probe,
     .read_header    = apng_read_header,
     .read_packet    = apng_read_packet,
+    .read_close     = apng_read_close,
     .flags          = AVFMT_GENERIC_INDEX,
     .priv_class     = &demuxer_class,
 };