[Ffmpeg-devel] [PATCH] Chinese AVS video decoder

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> Hi
>
> On Sun, Jul 02, 2006 at 12:36:26AM +0100, M?ns Rullg?rd wrote:
>> Some comments based on discussions on IRC and other observations.
>> 
>> > +typedef struct {
>> > +    int16_t x;
>> > +    int16_t y;
>> > +    int16_t dist;
>> > +    int16_t ref;
>> > +} vector_t;
>> 
>> If this is marked with __attribute__((align(8))) gcc will be able to
>> read/write the entire thing with one instruction on 64-bit machines.
>
> yes but to avoid breaking non gcc __attribute__ cant be added directly
> maybe DECLARE_ALIGNED_8() could be used ...

Hmm, it can with gcc at least, although it looks a bit odd.  You'll
have to choose from

DECLARE_ALIGNED_8(typedef, struct) {
    int16_t x;
    int16_t y;
    int16_t dist;
    int16_t ref;
} vector_t;

or

typedef struct {
    int16_t x;
    int16_t y;
    int16_t dist;
    int16_t ref;
} DECLARE_ALIGNED_8(vector_t,);

of variants thereof.

>> > +static void intra_pred_vert(uint8_t *d,uint8_t *top,uint8_t *left,int stride) {
>> > +    int y;
>> > +    uint64_t a = *((uint64_t *)(&top[1]));
>> > +    for(y=0;y<8;y++) {
>> > +        *((uint64_t *)(d+y*stride)) = a;
>> > +    }
>> > +}
>> > +
>> > +static void intra_pred_horiz(uint8_t *d,uint8_t *top,uint8_t *left,int stride) {
>> > +    int y;
>> > +    uint64_t a;
>> > +    for(y=0;y<8;y++) {
>> > +        a = left[y+1] * 0x0101010101010101ULL;
>> > +        *((uint64_t *)(d+y*stride)) = a;
>> > +    }
>> > +}
>> > +
>> > +static void intra_pred_dc_128(uint8_t *d,uint8_t *top,uint8_t *left,int stride) {
>> > +    int y;
>> > +    uint64_t a = 0x8080808080808080ULL;
>> > +    for(y=0;y<8;y++)
>> > +        *((uint64_t *)(d+y*stride)) = a;
>> > +}
>> 
>> More alignment trouble.
>
> hmm, where is the problem in the 2nd and 3rd function you quote? without
> looking at the original code id say stride and d must be aigned to 8

Alright, let's see.  The function would be called from this loop:

    for(block=0;block<4;block++) {
        d = h->cy + h->luma_scan[block];
        load_intra_pred_luma(h, top, left, block);
        h->intra_pred_l[(int)h->pred_mode_Y[scan3x3[block]]]
            (d, top, left, h->l_stride);
        if(h->cbp & (1<<block))
            decode_residual_block(h,gb,intra_2dvlc,1,h->qp,d,h->l_stride);
    }

or from one of these lines:

    h->intra_pred_c[pred_mode_uv](h->cu, top, left, h->c_stride);
    h->intra_pred_c[pred_mode_uv](h->cv, top, left, h->c_stride);

h->cy, h->cu, h->cv are all set from h->picture.data[x], which I
suppose are aligned, offset by multiples of 8.  h->luma_scan[x] are
also all multiples of 8 so I guess it's safe after all.

I was only doing a quick scan for suspicious-looking things, and those
kinds of casts certainly need some extra care.

I a nasty one on the first pass:

typedef struct {
    MpegEncContext s;
    Picture picture; //currently decoded frame
    Picture DPB[2];  //reference frames
[...]
} AVSContext;

Those should be AVFrame, or else the code fragments below are not good
at all.  In fact, I'm surprised it's not causing any serious trouble.

    if(h->picture.data[0])
        s->avctx->release_buffer(s->avctx, (AVFrame *)&h->picture);

    s->avctx->get_buffer(s->avctx, (AVFrame *)&h->picture);
[...]
    if(h->pic_type != FF_B_TYPE) {
        if(h->DPB[1].data[0])
            s->avctx->release_buffer(s->avctx, (AVFrame *)&h->DPB[1]);
        memcpy(&h->DPB[1], &h->DPB[0], sizeof(Picture));
        memcpy(&h->DPB[0], &h->picture, sizeof(Picture));
        memset(&h->picture,0,sizeof(Picture));
    }
[...]
    if (buf_size == 0) {
        if(!s->low_delay && h->DPB[0].data[0]) {
            *data_size = sizeof(AVPicture);
            *picture = *(AVFrame *) &h->DPB[0];
        }
        return 0;
    }
[...]
            if(!h->got_keyframe) {
                if(h->DPB[0].data[0])
                    avctx->release_buffer(avctx, (AVFrame *)&h->DPB[0]);
                if(h->DPB[1].data[0])
                    avctx->release_buffer(avctx, (AVFrame *)&h->DPB[1]);
                h->got_keyframe = 1;
            }
[...]
            if(h->pic_type != FF_B_TYPE) {
                if(h->DPB[1].data[0]) {
                    *picture = *(AVFrame *) &h->DPB[1];
                } else {
                    *data_size = 0;
                }
            } else
                *picture = *(AVFrame *) &h->picture;

This is a good example of why blindly adding a cast to silence a
compiler warning is a very, very bad idea.

-- 
M?ns Rullg?rd
mru at inprovide.com