[FFmpeg-devel] BFI video decoder

[Sisir Koppaka]

On Thu, Apr 17, 2008 at 10:56 PM, Michael Niedermayer <michaelni at gmx.at>
wrote:

> On Thu, Apr 17, 2008 at 10:45:18PM +0530, Sisir Koppaka wrote:
> > On Thu, Apr 17, 2008 at 10:32 PM, Michael Niedermayer <michaelni at gmx.at>
> > wrote:
> >
> > > On Thu, Apr 17, 2008 at 10:05:05PM +0530, Sisir Koppaka wrote:
> > > > On Thu, Apr 17, 2008 at 8:24 PM, Michael Niedermayer <
> michaelni at gmx.at>
> > > > wrote:
> > > [...]
> > > > >
> > > > >
> > > > > >         char palette[768];
> > > > > >         bfi->frame.pict_type = FF_I_TYPE;
> > > > > >         bfi->frame.key_frame = 1;
> > > > > >         /* Converts the given 6-bit palette values(0-63) to
> 8-bit
> > > > > values(0-255) */
> > > > > >         for (i = 0; i < avctx->extradata_size; i++)
> > > > > >             palette[i] =
> > > > > >                 (avctx->extradata[i] << 2) |
> (avctx->extradata[i] >>
> > > 4);
> > > > >
> > > > > What could happen if extradata_size where larger than 768 ?
> > > > >
> > > > But we set it to 768 in the demuxer right?
> > >
> > > yes, but think about what could happen if someone came up with the
> idea of
> > > storing bfi in a different container, for example avi. And then that
> > > person
> > > would add support for bfi in avi to ffmpeg (this likely just needs a
> > > single
> > > line to be added to riff.c). suddenly extradata_size would be read
> from
> > > the
> > > avi file and could be larger than 768.
> > >
> > Ok...I hadn't thought about that. I think declaring char
> > palette[avctx->extradata] would support that...
>
> This is true in principle but palette[extradata_size] is dangerous
> as well. The reason is that extradata_size can become up to 4gb but the
> stack
> is not 4gb large.
>
Well, that's a good reason not to use the array, I got the palette code
working so it's fine now...

>
>
> > but this is the version of
> > the palette code without an array:
> >
> >         pal = (uint32_t *) bfi->frame.data[1];
> >         for (i = 0; i < 256; i++) {
> >             int shift = 16;
> >             for (j=0; j<3; j++, shift -= 8)
> >                 *pal += ((avctx->extradata[i*3+j] << 2) |
> > (avctx->extradata[i*3+j] >> 4)) << shift;
> >             pal++;
> >         }
>
> You are adding to *pal, and apparently assume it is 0 at the begin, i
> think
> it is not 0 at the begin.
>
Yes, doing a *pal = 0 before the for works and it's back to normal. I also
changed for (i = 0; i < 256; i++) to for (i = 0; i <
avctx->extradata_size/3; i++) which should probably work for the AVI
transmuxing(i don't know if this is the term :) ) case you described
earlier.

-----------------
Sisir Koppaka