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

Baptiste Coudurier baptiste.coudurier
Tue Mar 10 19:10:02 CET 2009


Hi Alex,

On 3/10/2009 11:01 AM, Alex Converse wrote:
> On Mon, Mar 9, 2009 at 9:57 PM, Baptiste Coudurier
> <baptiste.coudurier at gmail.com> wrote:
>> 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.
>>
> 
> Here is a version that uses get_bits_long().
> 
> In my opinion it's much messier. If only I could get away with using
> pb->buf_ptr to init the get_bits that would be much cleaner.
> 

Well, ok. Thanks for your effort, you I'm ok with the two patches you
can commit whichever you prefer :>

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




More information about the ffmpeg-devel mailing list