[FFmpeg-devel] BFI video decoder

[Michael Niedermayer]

On Fri, Apr 18, 2008 at 12:23:47AM +0530, Sisir Koppaka wrote:
> Updated decoder attached.
> 
> I haven't used the refactored version of the length bound checking because
> this was a bit clearer. 

You can factorize the length check using:

static const uint8_t lentab[4]={0,2,0,1};

if (dst + (length<<lentab[code]) > frame_end)
    break;



[...]
> 3 3 3 3 1 3 3 3 3 3 3 3 3 3 1 3 3 3 3 3 3 3 3 1 3 3 3 3 3 3 3 3 3 3 3 1 3 3
> 3 3 3 3 3 3 3 3 3 1 3 1 3 3 3 3 1 3 3 3 3 3 3 3 3 3 3 3 3 3 1 3 3 3 3 3 3 3
> 1 3 3 3 3 1 3 3 3 3 3 3 3 1 3 3 3 3 3 3 3 3 3 3 3 1
> [DECODER] Finished decoding the frame.
> ------------------------------
> It seems to be breaking the bounds too frequently...what do you think?

I think there is a bug in your decoder, read the part about
"Video compression" in the spec again! There is something you forgot.


[...]

> 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 i, j;
>     if (bfi->frame.data[0])
>         avctx->release_buffer(avctx, &bfi->frame);
>     if (avctx->get_buffer(avctx, &bfi->frame) < 0) {
>         av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
>         return -1;
>     }
>     dst = bfi->dst;

>     frame_end = bfi->dst + avctx->width * avctx->height;

declaration and initialization can be merged


>     if (!avctx->frame_number) {
>         int shift;
>         bfi->frame.pict_type = FF_I_TYPE;
>         bfi->frame.key_frame = 1;
>         /* Setting the palette */

>         pal = (uint32_t *) bfi->frame.data[1];
>         for (i = 0; i < avctx->extradata_size / 3; i++) {

>             shift = 16;

int shift = 16;
is IMHO clearer


>             *pal = 0;
>             for (j = 0; j < 3; j++, shift -= 8)
>                 *pal +=

>                     ((avctx->extradata[i * 3 + j] << 2) | (avctx->
>                                                            extradata[i *
>                                                                      3 +
>                                                                      j] >>
>                                                            4)) << shift;

following looks nicer:
                      ((avctx->extradata[i * 3 + j] << 2) |
                       (avctx->extradata[i * 3 + j] >> 4)  ) << shift;

Also how large is the pal array? How large is extradata_size? is there a
possibility later might be larger?


>             pal++;
>         }


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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- 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/0dc840a3/attachment.pgp>