[FFmpeg-devel] [PATCH] Updated original IFF demuxer code to be 100% standard IFF-compliance

[Stefano Sabatini]

On date Tuesday 2010-04-20 23:13:17 +0200, Sebastian Vater encoded:
> So, I finished the partial stuff, too...
> 
> Sebastian Vater a ?crit :
> > Hi Ronald!
> >
> > Ronald S. Bultje a ?crit :
> >   
> >> That would break things badly, no? Don't skip 1 byte BEFORE reading
> >> the data. And you're no longer exiting the loop so we will never
> >> actually decode the file.
> >>   
> >>     
> > Fixed partially.
> >
> >   
> Fixed.
> 
> -- 
> 
> Best regards,
>                    :-) Basty/CDGS (-:
> 

> Index: ffmpeg-svn/libavformat/iff.c
> ===================================================================
> --- ffmpeg-svn/libavformat/iff.c	(r??vision 22924)
> +++ ffmpeg-svn/libavformat/iff.c	(copie de travail)
> @@ -2,6 +2,7 @@
>   * IFF (.iff) file demuxer
>   * Copyright (c) 2008 Jaikrishnan Menon <realityman at gmx.net>
>   * Copyright (c) 2010 Peter Ross <pross at xvid.org>
> + * Copyright (c) 2010 Sebastian Vater <cdgs.basty at googlemail.com>
>   *
>   * This file is part of FFmpeg.
>   *
> @@ -72,6 +73,7 @@
>  } bitmap_compression_type;
>  
>  typedef struct {
> +    uint64_t  body_pos;
>      uint32_t  body_size;
>      uint32_t  sent_bytes;
>      uint32_t  audio_frame_count;
> @@ -106,8 +108,7 @@
>      IffDemuxContext *iff = s->priv_data;
>      ByteIOContext *pb = s->pb;
>      AVStream *st;
> -    uint32_t chunk_id, data_size;
> -    int padding, done = 0;
> +    uint32_t chunk_id, data_size, data_size_skip;
>      int compression = -1;
>      char *buf;
>  
> @@ -120,28 +121,43 @@
>      // codec_tag used by ByteRun1 decoder to distinguish progressive (PBM) and interlaced (ILBM) content
>      st->codec->codec_tag = get_le32(pb);
>  
> -    while(!done && !url_feof(pb)) {
> +    while(!url_feof(pb)) {
>          chunk_id = get_le32(pb);
> -        data_size = get_be32(pb);
> -        padding = data_size & 1;
> +        data_size_skip = data_size = get_be32(pb);
> +        data_size_skip += data_size & 1;
>  
>          switch(chunk_id) {
>          case ID_VHDR:
>              st->codec->codec_type = AVMEDIA_TYPE_AUDIO;
> +
> +            if (data_size < 14)
> +                return AVERROR_INVALIDDATA;
> +
>              url_fskip(pb, 12);
>              st->codec->sample_rate = get_be16(pb);
> -            url_fskip(pb, 1);
> -            compression            = get_byte(pb);
> -            url_fskip(pb, 4);
> +
> +            data_size_skip -= 14;
> +
> +            if (data_size >= 16) {
> +                url_fskip(pb, 1);
> +                compression            = get_byte(pb);
> +
> +                data_size_skip -= 2;
> +            }
>              break;
>  
>          case ID_BODY:
> +            iff->body_pos = url_ftell(pb);
>              iff->body_size = data_size;
> -            done = 1;
>              break;
>  
>          case ID_CHAN:
> +            if (data_size < 4)
> +                return AVERROR_INVALIDDATA;
> +
>              st->codec->channels = (get_be32(pb) < 6) ? 1 : 2;
> +
> +            data_size_skip -= 4;
>              break;
>  
>          case ID_CMAP:
> @@ -151,20 +167,36 @@
>                  return AVERROR(ENOMEM);
>              if (get_buffer(pb, st->codec->extradata, data_size) < 0)
>                  return AVERROR(EIO);
> +
> +            data_size_skip -= data_size;
>              break;
>  
>          case ID_BMHD:
>              st->codec->codec_type            = AVMEDIA_TYPE_VIDEO;
> +
> +            if (data_size <= 8)
> +                return AVERROR_INVALIDDATA;
> +
>              st->codec->width                 = get_be16(pb);
>              st->codec->height                = get_be16(pb);
>              url_fskip(pb, 4); // x, y offset
>              st->codec->bits_per_coded_sample = get_byte(pb);
> -            url_fskip(pb, 1); // masking
> -            compression                      = get_byte(pb);
> -            url_fskip(pb, 3); // paddding, transparent
> -            st->sample_aspect_ratio.num      = get_byte(pb);
> -            st->sample_aspect_ratio.den      = get_byte(pb);
> -            url_fskip(pb, 4); // source page width, height

It's a good norm to keep the old lines in the new patch with no
reindentation, that should help to make the patch smaller and ease
review / future reading of the diff, reindentation will be applied
in a further patch.

[...]

Apart this, patch looks OK to me (I assume it has been tested).

Regards.
-- 
FFmpeg = Fundamentalist Fabulous Minimalistic Pitiful EnGine