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

[Alex Converse]

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.

--Alex
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mov-stz2-gb.diff
Type: text/x-diff
Size: 2763 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090310/a6808ffe/attachment.diff>