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

[Michael Niedermayer]

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


[...]
> > +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


[...]
> > +    if(newmode < 0) {
> > +        av_log(NULL, AV_LOG_ERROR, "Illegal intra prediction mode\n");
> > +        *mode = 0;
> > +    } else {
> > +        *mode = newmode;
> > +    }
> > +}
> [...]
> > +static inline void veccpy(vector_t *dst, vector_t *src) {
> > +    *((uint64_t *)dst) = *((uint64_t *)src);
> > +}
> 
> This should be
> 
> static inline void veccpy(vector_t *dst, const vector_t *src) {
>     *dst = *src;
> }

thats something i shouldnt have missing during reviewing :((((
of course this is unaccpetable, veccpy should be replaced by dst=src
entirely

[...]

> 
> > +/* kth-order exponential golomb code */
> > +static inline int get_ue_code(GetBitContext *gb, int order) {
> > +    if(order)
> > +        return (get_ue_golomb(gb) << order) + get_bits(gb,order);
> 
> The order of evaluation of operands is unspecified, so that could read

thats another one i shouldnt have missed :(

[...]

> > +static int decode_residual_block(AVSContext *h, GetBitContext *gb,
> > +                                 const residual_vlc_t *r, int esc_golomb_order,
> > +                                 int qp, uint8_t *dst, int stride) {
> > +    int i,pos = -1;
> > +    int level_code, esc_code, level, run, mask;
> > +    int level_buf[64];
> > +    int run_buf[64];
> > +    int dqm = dequant_mul[qp];
> > +    int dqs = dequant_shift[qp];
> > +    int dqa = 1 << (dqs - 1);
> > +    const uint8_t *scantab = ff_zigzag_direct;
> > +    DCTELEM block[64];
> > +
> > +    memset(block,0,64*sizeof(DCTELEM));
> 
> Is this memset really needed?

yes


[...]

> > +AVCodec cavs_decoder = {
> > +    "cavs",
> > +    CODEC_TYPE_VIDEO,
> > +    CODEC_ID_CAVS,
> > +    sizeof(AVSContext),
> > +    cavs_decode_init,
> > +    NULL,
> > +    cavs_decode_end,
> > +    cavs_decode_frame,
> > +    CODEC_CAP_TRUNCATED | CODEC_CAP_DELAY, //FIXME is this correct ?
> 
> CODEC_CAP_TRUNCATED should go.  It's not supported.

yes and CODEC_CAP_DR1 probably is so it should be added

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

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is