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

[Ronald S. Bultje]

Hi,

On Sun, Apr 25, 2010 at 3:38 PM, Sebastian Vater
<cdgs.basty at googlemail.com> wrote:
> 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!

Applied.

Ronald