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

[Alex Converse]

On Tue, Mar 10, 2009 at 2:10 PM, Baptiste Coudurier
<baptiste.coudurier at gmail.com> wrote:
> 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 :>
>

Applied the get_bits_long() version