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

[Ronald S. Bultje]

Hi,

On Sun, Apr 25, 2010 at 2:48 PM, Sebastian Vater
<cdgs.basty at googlemail.com> wrote:
> So, I redid the IFF compliance patch with git...should be ready now for mainline git!
[..]
> +    uint32_t  body_pos;

Should be uint64_t.

> -        padding = data_size & 1;
> +        data_size_skip = data_size = get_be32(pb);
> +        data_size_skip += data_size & 1;

This is wrong if data_size = 0xFFFFFFFF. It's unlikely, but the add
will overflow.

The whole data_skip_size sounds like NIH syndrome to me. Better
approach is to do uint64_t orig_pos = url_ftell() after reading
chunkID/size, and in the end skip data_size - (url_ftell(pb) -
orig_pos) + (data_size & 1). That way you don't have to keep track of
how many bytes you've read and how many you haven't, like you do
everywhere below:

[..]
> -            url_fskip(pb, 1);
> -            compression            = get_byte(pb);
> +            data_size_skip -= 14;
> +
> +            if (data_size >= 16) {
> +                url_fskip(pb, 1);
> +                compression            = get_byte(pb);
> +
> +                data_size_skip -= 2;
> +            }
>              break;

Bleh, also this mixes reindenting and functional changes
(compression=... and the url_fskip(1)).

>          case ID_BODY:
> +            iff->body_pos = url_ftell(pb);
>              iff->body_size = data_size;
> -            done = 1;
> +            data_size_skip -= data_size;
>              break;

This again won't work. The whole point of this change appears to be
potential chunks after the BODY, but this code specifically does NOT
skip the BODY data, thus you will never read any chunk after the BODY.

>          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
> +
> +            data_size_skip -= 9;
> +
> +            if (data_size >= 11) {
> +                url_fskip(pb, 1); // masking
> +                compression                      = get_byte(pb);
> +
> +                data_size_skip -= 2;
> +            }
> +            if (data_size >= 16) {
> +                url_fskip(pb, 3); // paddding, transparent
> +                st->sample_aspect_ratio.num      = get_byte(pb);
> +                st->sample_aspect_ratio.den      = get_byte(pb);
> +
> +                data_size_skip -= 5;
> +            }
>              break;

And again, this part mixes functional/cosmetic changes, the
data_skip_size is not needed as per my advice above.

> -        default:
> -            url_fseek(pb, data_size + padding, SEEK_CUR);
> +            data_size_skip -= data_size;
>              break;
> +
> +        if (data_size_skip > 0)
> +            url_fskip(pb, data_size_skip);

This (the default case) is wrong, it should likely be removed
completely. Now, for unknown chunks, you end up NOT skipping the data.

Ronald