[FFmpeg-devel] [PATCH 02/N] RV30/40 Decoder - RV30 decoder

[Kostya]

On Sat, Dec 01, 2007 at 10:13:59PM +0100, Michael Niedermayer wrote:
> On Sat, Dec 01, 2007 at 08:07:51PM +0200, Kostya wrote:
> > Here it is. I decided to drop loop filter because
> > it's not complete yet and I don't know what parameters
> > should be fed to it.
> 
> [...]
> 
> > static int rv30_parse_slice_header(RV34DecContext *r, GetBitContext *gb, SliceInfo *si)
> > {
> >     int t, mb_bits;
> >     int w = r->s.width, h = r->s.height;
> >     int mb_size;
> > 
> >     memset(si, 0, sizeof(SliceInfo));
> >     skip_bits(gb, 3);
> >     si->type = get_bits(gb, 2);
> >     if(si->type == 1) si->type = 0;
> >     if(get_bits1(gb))
> >         return -1;
> >     si->quant = get_bits(gb, 5);
> >     skip_bits1(gb);
> 
> >     t = get_bits(gb, 13);
> 
> t is unused

dropped 
 
> >     skip_bits(gb, r->rpr);
> 
> >     si->vlc_set = 0;
> 
> uneeded due to the memset(), either one of the 2 should be removed

dropped as well 
 
> [...]
> > /**
> >  * Decode 4x4 intra types array
> >  */
> > static int rv30_decode_intra_types(RV34DecContext *r, GetBitContext *gb, int *dst)
> > {
> >     int i, j, k;
> >     int A, B;
> >     int *ptr;
> >     int code;
> > 
> >     for(i = 0; i < 4; i++, dst += r->s.b4_stride){
> >         ptr = dst;
> 
> dst += r->s.b4_stride - 4 would avoid the ptr variable

dropped ptr too 
 
> >         for(j = 0; j < 4; j+= 2){
> >             code = (ff_rv34_get_gamma(gb) - 1) << 1;
> >             if(code >= 81*2){
> >                 av_log(r->s.avctx, AV_LOG_ERROR, "Incorrect intra prediction code\n");
> >                 return -1;
> >             }
> >             for(k = 0; k < 2; k++){
> 
> >                 A = ptr[-r->s.b4_stride] + 1;
> >                 B = ptr[-1] + 1;
> 
> declaration and statement can be merged, this is also true for code

done 
 
> [...]
> >     int code;
> > 
> >     code = ff_rv34_get_gamma(gb) - 1;
> 
> can be merged

merged
I just can't get accustomed enough with function calls on variable declarations.

> [...]
> > /**
> >  * Initialize decoder
> >  */
> > static int rv30_decode_init(AVCodecContext *avctx)
> > {
> >     RV34DecContext *r = avctx->priv_data;
> >     MpegEncContext *s = &r->s;
> > 
> >     r->rv30 = 1;
> >     ff_rv34_decode_init(avctx);
> >     if(avctx->extradata_size < 2){
> >         av_log(avctx, AV_LOG_ERROR, "Extradata is too small\n");
> >         return -1;
> >     }
> >     r->rpr = (avctx->extradata[1] & 7) >> 1;
> >     r->rpr = FFMIN(r->rpr + 1, 3);
> >     r->parse_slice_header = rv30_parse_slice_header;
> 
> >     r->decode_intra_types = rv30_decode_intra_types;
> >     r->decode_mb_info     = rv30_decode_mb_info;
> 
> these are used just once, so a
> if(rv30) A()
> else     B()
> would be more obvious, also it would allow gcc to inline the code
 
I will leave it as is because somebody will want to disable one of the decoders
and this will save a bit of code. Motion compensation will use if(r->rv30)
though.
 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB