[FFmpeg-devel] BFI video decoder

[Michael Niedermayer]

On Thu, Apr 17, 2008 at 07:55:00PM +0530, Sisir Koppaka wrote:
> /* Sorry for cross-posting this message on ffmpeg-soc and devel, but I sent
> a mail with a questionably large attachment in the morning to soc and since
> then my mails haven't been appearing in the soc archives and hence, I'm not
> sure if anyone else is getting them. */
> 
> updated decoder attached.
[...]
> >
> > >     if (avctx->reget_buffer(avctx, &bfi->frame)) {
> > >         av_log(avctx, AV_LOG_ERROR, "reget_buffer() failed\n");
> > >         return -1;
> > >     }
> > >     //avcodec_set_dimensions(avctx, avctx->width, avctx->height);
> > //Check this.
> > >     dst = bfi->dst;
> > >     frame_end = bfi->dst + avctx->width * avctx->height;
> > >     if(bfi->toggle) {
> > >         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++)
> > >             avctx->extradata[i] =
> > >                 (avctx->extradata[i] << 2) | (avctx->extradata[i] >> 4);
> >
> > A decoder cannot write in extradata
> >
> Now uses a local array of size 768....if size is a concern, then i'll reduce
> it to an array of size 3, should I?

The size is of no concern, though it would be nice if this could be done simpler
and without an intermediate array.


[...]
> >
> > >             av_log(NULL,AV_LOG_INFO, "\n[DECODER] Back Chain.");
> > >             break;
> > >         case 2:                //Skip Chain
> >
> > >             if(dst+length>frame_end)
> > >                 break;
> >
> > duplicate
> >
> I did some refactoring but it seems  to degenerate the video. I think the
> question is what to do when we hit one of these cases where length is out of
> bounds etc...do we ignore that particular iteration and move on to the next
> or because do we ignore the whole frame? if we ignore only that iteration,
> then we could possibly be displacing a whole set of pixels, which is what I
> think is happening, looking at the video. Some decent cases of this
> displacement are at http://www.flickr.com/gp/40656348 at N00/44Z0J8 but after
> the refactoring it's become worse for some reason.

We should ignore everything of the current frame after the error at least.

[...]
> static int bfi_decode_frame(AVCodecContext * avctx, void *data,
>                             int *data_size, const uint8_t * buf,
>                             int buf_size)
> {
>     BFIContext *bfi = avctx->priv_data;
>     uint8_t *dst;
>     uint8_t *src;
>     uint8_t *dst_offset;
>     uint8_t colour1, colour2;
>     uint32_t *pal;
>     uint8_t *frame_end;
>     unsigned int code, byte, length, offset, height = avctx->height;
>     int remaining = avctx->width, i, j;

>     if (avctx->reget_buffer(avctx, &bfi->frame)) {
>         av_log(avctx, AV_LOG_ERROR, "reget_buffer() failed\n");
>         return -1;
>     }

As nothing is reused from the content of the AVFrame a normal get_buffer()/
release_buffer() could be used.


>     //avcodec_set_dimensions(avctx, avctx->width, avctx->height); //Check this.
>     dst = bfi->dst;
>     frame_end = bfi->dst + avctx->width * avctx->height;

>     if (bfi->toggle) {

You could use  avctx->frame_number == 0 here and remove the toggle variable


>         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 ?


[...]
>     while (dst != frame_end) {
>         byte = *buf++;
>         code = byte >> 6;
>         length = byte & ~0xC0;
>         if (length == 0) {
>             if (code == 1) {
>                 length = bytestream_get_byte(&buf);
>                 offset = bytestream_get_le16(&buf);
>             } else {

>                 length = bytestream_get_le16(&buf);
>                 if (code == 2)
>                     goto finish;

this does not look correct


>             }
>         } else {
>             if (code == 1)
>                 offset = bytestream_get_byte(&buf);
>         }
> 
>         if (dst + length > frame_end)
>             continue;

>         if (code == 1 && dst_offset < bfi->dst)
>             continue;

this also does not look correct


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

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- 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/20080417/424d91dc/attachment.pgp>