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

[Sebastian Vater]

Hi Ronald!

Ronald S. Bultje a ?crit :
>> +    uint32_t  body_pos;
>>     
>
> Should be uint64_t.
>   
Fixed.
> 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:
>   
Fixed in regards your method, it's really a neat idea!
> Bleh, also this mixes reindenting and functional changes
> (compression=... and the url_fskip(1)).
>   
Oops, fixed!
>>          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.
>   
Fixed!
>>          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.
>   
Fixed!
>> -        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.
>   
Fixed!

-- 

Best regards,
                   :-) Basty/CDGS (-:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: iff-compliance.patch
Type: text/x-patch
Size: 3615 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100425/7bdc637c/attachment.bin>