[FFmpeg-devel] [PATCH 01/N] RV30/40 Decoder - Core

[Kostya]

On Thu, Dec 06, 2007 at 02:23:09AM +0100, Michael Niedermayer wrote:
> On Sat, Dec 01, 2007 at 08:03:23PM +0200, Kostya wrote:
> [...]
> 
> > typedef struct RV34DecContext{
> >     MpegEncContext s;
> 
> >     int mb_bits;             ///< bits needed to read MB offet in slice header
> 
> still unused
> 
> 
> [...]
> >     int vlc_set;             ///< index of currently selected VLC set
> 
> still unused
> 
> 
> [...]
> >     SliceInfo prev_si;       ///< info for the saved slice
> 
> still unused

dropped them 
 
> [...]
> 
> checks for width/height are missing, the only (insufficient) check
> has been commented out:
> 
> >         if(s->width != r->si.width || s->height != r->si.height /*&& avcodec_check_dimensions(s->avctx, r->si.width, r->si.height) >= 0 */){
> 
> 
> i suggest that you add the check in the rv40 slice header parsing
> function where it is needed

done 
 
> [...]
> > /** translation of RV30/40 macroblock types to lavc ones */
> > static const int rv34_mb_type_to_lavc[12] = {
> >     MB_TYPE_INTRA, MB_TYPE_INTRA16x16, MB_TYPE_16x16,   MB_TYPE_8x8,
> >     MB_TYPE_16x16, MB_TYPE_16x16,      MB_TYPE_SKIP,    MB_TYPE_DIRECT2,
> >     MB_TYPE_16x8,  MB_TYPE_8x16,       MB_TYPE_DIRECT2, MB_TYPE_16x16
> > };
> 
> these types are still invalid, they are missing reference information
> one valid type would be for example
> MB_TYPE_SKIP | MB_TYPE_L0 | MB_TYPE_16x16

I've tried to do this, correct if I'm wrong 
 
> [...]
> > /**
> >  * Decode Levenstein (also known as Elias Gamma) code.
> >  */
> > int ff_rv34_get_gamma(GetBitContext *gb)
> > {
> >     int code = 1;
> > 
> >     while(!get_bits1(gb)){
> >         code = (code << 1) | get_bits1(gb);
> >     }
> >     return code;
> > }
> > 
> > /**
> >  * Decode Levenstein (also known as Elias Gamma) code as signed integer.
> >  */
> > int ff_rv34_get_gamma_signed(GetBitContext *gb)
> > {
> 
> >     int code;
> > 
> >     code = ff_rv34_get_gamma(gb);
> 
> can be merged
> 
> 
> >     if(code & 1)
> >         return -(code >> 1);
> >     else
> >         return code >> 1;
> > }
> 
> please check if the following are identical (+-1 doesnt count as
> different)
> 
> svq3_get_ue_golomb() and 
> svq3_get_se_golomb()
> 
> and if they are identical post START/STOP_TIMER scores
> also id like to see START/STOP_TIMER scores in that case for the
> original vlc based code
> 
> the fastest should be placed in golomb.c/h
> and used

I don't know when I can do benchmarks but switched to svq3_get_[su]e_golomb()
anyway.
 
> > 
> > /**
> >  * Decode quantizer difference and return modified quantizer.
> >  */
> > static inline int rv34_decode_dquant(GetBitContext *gb, int quant)
> > {
> >     if(get_bits1(gb))
> >         return quant + rv34_dquant_tab[quant * 2 + get_bits1(gb)];
> >     else
> >         return get_bits(gb, 5);
> > }
> 
> rv34_dquant_tab should be changed to be identical to modified_quant_tab
> it safes that "quant + "

done 
 
> [...]
> >     // calculate which neighbours are available
> >     memset(r->avail, 0, sizeof(r->avail));
> >     if(s->mb_x && !(s->first_slice_line && s->mb_x == s->resync_mb_x))
> >         r->avail[0] = 1;
> >     if(!s->first_slice_line)
> >         r->avail[1] = 1;
> >     if((s->mb_x+1) < s->mb_width && (!s->first_slice_line || (s->first_slice_line && (s->mb_x+1) == s->resync_mb_x)))
> >         r->avail[2] = 1;
> >     if(s->mb_x && !s->first_slice_line && !((s->mb_y-1)==s->resync_mb_y && s->mb_x == s->resync_mb_x))
> >         r->avail[3] = 1;
> 
> r->avail[0] = s->mb_x && !(s->first_slice_line && s->mb_x == s->resync_mb_x)
> r->avail[1] = !s->first_slice_line;  here one wonders why first_slice_line is not used used directly
> r->avail[2] = (s->mb_x+1) < s->mb_width && (!s->first_slice_line || (s->first_slice_line && (s->mb_x+1) == s->resync_mb_x))
> r->avail[3] = ...

done 
 
> [...]
> >             memmove(r->intra_types_hist, r->intra_types, s->b4_stride * 4 * sizeof(int));
> >             memset(r->intra_types, -1, s->b4_stride * 4 * sizeof(int));
> 
> sizeof(*r->intra_types_hist), sizeof(*r->intra_types)
> and why are they int anyway wont int8_t be enough?

replaced 
 
> [...]
> > /**
> >  * Initialize decoder.
> >  */
> > int ff_rv34_decode_init(AVCodecContext *avctx)
> > {
> >     RV34DecContext *r = avctx->priv_data;
> >     MpegEncContext *s = &r->s;
> > 
> 
> >     static int tables_done = 0;
> 
> you can check one entry from one of the tables to avoid this variable

done 
 
> [...]
> 
> >     /* no supplementary picture */
> >     if (buf_size == 0) {
> >         /* special case for last picture */
> >         if (s->low_delay==0 && s->next_picture_ptr) {
> >             *pict= *(AVFrame*)s->next_picture_ptr;
> >             s->next_picture_ptr= NULL;
> > 
> >             *data_size = sizeof(AVFrame);
> >         }
> >     }
> 
> you are missing a return here
> also you are missing CODEC_CAP_DR1 | CODEC_CAP_DELAY in the 2 AVCodec in rv30/40.c

added 
 
> > 
> >     if(!avctx->slice_count){
> >         slice_count = (*buf++) + 1;
> >         slices_hdr = buf + 4;
> >         buf += 8 * slice_count;
> >     }else
> >         slice_count = avctx->slice_count;
> 
> do we need avctx->slice_count support? i think that should be droped!
> it cant break any compatibility as this is a new decoder ...
 
I've left it for now to be the same as rv10/20 decoder.
I'd like to have them dropped simultaneously. 
 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rv34core.tar.bz2
Type: application/x-bzip2
Size: 11107 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071207/015f6941/attachment.bin>