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

[Michael Niedermayer]

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


[...]

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


[...]
> /** 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


[...]
> /**
>  * 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


> 
> /**
>  * 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 + "


[...]
>     // 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] = ...


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


[...]
> /**
>  * 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


[...]

>     /* 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


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


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

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- 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/20071206/8c265d74/attachment.pgp>