[FFmpeg-devel] [PATCH 1/5] avformat/smacker: Read extradata directly into extradata

[Andreas Rheinhardt]

Andreas Rheinhardt:
> The Smacker demuxer reads four consecutive 32bit values from the file
> header into its demux context (as four uint32_t), converting it to
> native endianness in the process and then writing these four values
> later (after extradata has been allocated) to extradata as four 32bit
> values (converting to little endian in the process).
> 
> This commit changes this: The stream and the extradata are allocated
> earlier, so that the data destined for extradata can be read directly
> into extradata.
> 
> Furthermore, given that these values are not needed for demuxing itself
> they are now no longer kept as part of the demuxing context.
> 
> Finally, a check regarding the number of frames has been moved up,
> too, in order to exit early before unnecessarily allocating the
> stream and the extradata.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
>  libavformat/smacker.c | 47 +++++++++++++++++--------------------------
>  1 file changed, 18 insertions(+), 29 deletions(-)
> 
> diff --git a/libavformat/smacker.c b/libavformat/smacker.c
> index 6de0e7a0f1..4db3ec326f 100644
> --- a/libavformat/smacker.c
> +++ b/libavformat/smacker.c
> @@ -25,10 +25,10 @@
>  
>  #include <inttypes.h>
>  
> -#include "libavutil/bswap.h"
>  #include "libavutil/channel_layout.h"
>  #include "libavutil/intreadwrite.h"
>  #include "avformat.h"
> +#include "avio_internal.h"
>  #include "internal.h"
>  
>  #define SMACKER_PAL 0x01
> @@ -51,7 +51,6 @@ typedef struct SmackerContext {
>      uint32_t flags;
>      uint32_t audio[7];
>      uint32_t treesize;
> -    uint32_t mmap_size, mclr_size, full_size, type_size;
>      uint8_t  aflags[7];
>      uint32_t rates[7];
>      uint32_t pad;
> @@ -128,6 +127,10 @@ static int smacker_read_header(AVFormatContext *s)
>      smk->flags = avio_rl32(pb);
>      if(smk->flags & SMACKER_FLAG_RING_FRAME)
>          smk->frames++;
> +    if (smk->frames > 0xFFFFFF) {
> +        av_log(s, AV_LOG_ERROR, "Too many frames: %"PRIu32"\n", smk->frames);
> +        return AVERROR_INVALIDDATA;
> +    }
>      for(i = 0; i < 7; i++)
>          smk->audio[i] = avio_rl32(pb);
>      smk->treesize = avio_rl32(pb);
> @@ -137,21 +140,25 @@ static int smacker_read_header(AVFormatContext *s)
>          return AVERROR_INVALIDDATA;
>      }
>  
> -//FIXME remove extradata "rebuilding"
> -    smk->mmap_size = avio_rl32(pb);
> -    smk->mclr_size = avio_rl32(pb);
> -    smk->full_size = avio_rl32(pb);
> -    smk->type_size = avio_rl32(pb);
> +    st = avformat_new_stream(s, NULL);
> +    if (!st)
> +        return AVERROR(ENOMEM);
> +
> +    if ((ret = ff_alloc_extradata(st->codecpar, smk->treesize + 16)) < 0) {
> +        av_log(s, AV_LOG_ERROR,
> +               "Cannot allocate %"PRIu32" bytes of extradata\n",
> +               smk->treesize + 16);
> +        return ret;
> +    }
> +    if ((ret = ffio_read_size(pb, st->codecpar->extradata, 16)) < 0)
> +        return ret;
> +
>      for(i = 0; i < 7; i++) {
>          smk->rates[i]  = avio_rl24(pb);
>          smk->aflags[i] = avio_r8(pb);
>      }
>      smk->pad = avio_rl32(pb);
>      /* setup data */
> -    if(smk->frames > 0xFFFFFF) {
> -        av_log(s, AV_LOG_ERROR, "Too many frames: %"PRIu32"\n", smk->frames);
> -        return AVERROR_INVALIDDATA;
> -    }
>      smk->frm_size = av_malloc_array(smk->frames, sizeof(*smk->frm_size));
>      smk->frm_flags = av_malloc(smk->frames);
>      if (!smk->frm_size || !smk->frm_flags) {
> @@ -171,12 +178,6 @@ static int smacker_read_header(AVFormatContext *s)
>      }
>  
>      /* init video codec */
> -    st = avformat_new_stream(s, NULL);
> -    if (!st) {
> -        av_freep(&smk->frm_size);
> -        av_freep(&smk->frm_flags);
> -        return AVERROR(ENOMEM);
> -    }
>      smk->videoindex = st->index;
>      st->codecpar->width = smk->width;
>      st->codecpar->height = smk->height;
> @@ -233,24 +234,12 @@ static int smacker_read_header(AVFormatContext *s)
>  
>  
>      /* load trees to extradata, they will be unpacked by decoder */
> -    if ((ret = ff_alloc_extradata(st->codecpar, smk->treesize + 16)) < 0) {
> -        av_log(s, AV_LOG_ERROR,
> -               "Cannot allocate %"PRIu32" bytes of extradata\n",
> -               smk->treesize + 16);
> -        av_freep(&smk->frm_size);
> -        av_freep(&smk->frm_flags);
> -        return ret;
> -    }
>      ret = avio_read(pb, st->codecpar->extradata + 16, st->codecpar->extradata_size - 16);
>      if(ret != st->codecpar->extradata_size - 16){
>          av_freep(&smk->frm_size);
>          av_freep(&smk->frm_flags);
>          return AVERROR(EIO);
>      }
> -    ((int32_t*)st->codecpar->extradata)[0] = av_le2ne32(smk->mmap_size);
> -    ((int32_t*)st->codecpar->extradata)[1] = av_le2ne32(smk->mclr_size);
> -    ((int32_t*)st->codecpar->extradata)[2] = av_le2ne32(smk->full_size);
> -    ((int32_t*)st->codecpar->extradata)[3] = av_le2ne32(smk->type_size);
>  
>      smk->curstream = -1;
>      smk->nextpos = avio_tell(pb);
> 
Any comments on this patchset? If not, I'll apply it tomorrow.

- Andreas