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

[Michael Niedermayer]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090310/4448b3be/attachment.pgp>