[FFmpeg-devel] [PATCH] MOV: support stz2 "Compact Sample Size Box"

[Baptiste Coudurier]

On 3/9/2009 4:38 PM, Michael Niedermayer wrote:
> On Mon, Mar 09, 2009 at 07:31:00PM -0400, Alex Converse wrote:
>> On Fri, Mar 6, 2009 at 4:42 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>>> On Fri, Mar 06, 2009 at 03:07:03PM -0500, Alex Converse wrote:
>>>> 2009/3/3 Michael Niedermayer <michaelni at gmx.at>:
>>>>> On Tue, Mar 03, 2009 at 02:45:15AM -0500, Alex Converse wrote:
>>>>> [...]
>>>>>> @@ -1143,8 +1150,33 @@ static int mov_read_stsz(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
>>>>>>      if (!sc->sample_sizes)
>>>>>>          return AVERROR(ENOMEM);
>>>>>>
>>>>>> +    switch(field_size) {
>>>>>> +    case 4:
>>>>>> +        for(i=0; i<entries-1; i+=2) {
>>>>>> +            int field = get_byte(pb);
>>>>>> +            sc->sample_sizes[i  ] = field >> 4;
>>>>>> +            sc->sample_sizes[i+1] = field & 0xF;
>>>>>> +        }
>>>>>> +        if(entries&1) {
>>>>>> +            sc->sample_sizes[entries-1] = get_byte(pb) >> 4;
>>>>>> +        }
>>>>>> +        break;
>>>>>> +    case 8:
>>>>>> +        for(i=0; i<entries; i++)
>>>>>> +            sc->sample_sizes[i] = get_byte(pb);
>>>>>> +        break;
>>>>>> +    case 16:
>>>>>> +        for(i=0; i<entries; i++)
>>>>>> +            sc->sample_sizes[i] = get_be16(pb);
>>>>>> +        break;
>>>>>> +    case 32:
>>>>>>      for(i=0; i<entries; i++)
>>>>>>          sc->sample_sizes[i] = get_be32(pb);
>>>>>> +        break;
>>>>>> +    default:
>>>>>> +        av_log(c->fc, AV_LOG_ERROR, "Invalid sample field size %d\n", field_size);
>>>>>> +        return -1;
>>>>>> +    }
>>>>> I think that should be using GetBitContext
>>>>>
>>>> Are you sure that's the best approach?
>>> no, but iam fairly sure above is worse ;)
>>> this code isnt speed critical, theres no sense in bloating the
>>> loops up like that ...
>>>
>> Is this better?
> [...]
>> @@ -1046,14 +1054,42 @@ static int mov_read_stsz(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
>>      if (sample_size)
>>          return 0;
>>  
>> +    switch(field_size) {
>> +    case 4:
>> +        break;
>> +    case 8:
>> +        get_size = &get_byte;
>> +        break;
>> +    case 16:
>> +        get_size = &get_be16;
>> +        break;
>> +    case 32:
>> +        get_size = &get_be32;
>> +        break;
>> +    default:
>> +        av_log(c->fc, AV_LOG_ERROR, "Invalid sample field size %d\n", field_size);
>> +        return -1;
>> +    }
>> +
> 
> normally one should not place more than 1 statement on a line but here
> 
> switch(field_size) {
> case 4:                       break;
> case 8: get_size = &get_byte; break;
> case 16:get_size = &get_be16; break;
> case 32:get_size = &get_be32; break;
> 
> looks better to me
> 
> anyway, remaining review is for baptistes, iam happy with above
> 

Well, maybe your first idea of using GetBitContext will be cleaner, it
would mean:
sc->sample_sizes[i] = get_bits_long(&gb, field_size);

You just have to malloc(), get_buffer/ free, unless we can have a way to
ask ByteIOContext to fill_buffer at least to what we need, and then
access ByteIOContext buffer. Do we have a way to do this ? That would be
nice.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no
FFmpeg maintainer                                  http://www.ffmpeg.org