[FFmpeg-devel] [PATCH] RV30/40 decoder

[Kostya]

On Sun, Sep 16, 2007 at 11:10:43PM +0200, Michael Niedermayer wrote:
> Hi
> 
> On Sun, Sep 16, 2007 at 07:58:51PM +0300, Kostya wrote:
> > Here is my RV30/40 decoder developed during this GSoC.
> > 
> > It still outputs slightly incorrect picture and needs fixing of
> > B-frames motion vector reconstruction(prediction) but it should
> > be stable now and picture is quite recognizable. 
> > 
> > VLC tables are omitted from this patch, they can be found
> > at svn://svn.mplayerhq.hu/soc/rv40 files rv40vlc.h and rv40vlc2.h
> 
> 
[...]
> > +/** Slice information saved for truncated slices */
> > +typedef struct SavedSliceInfo{
> > +    uint8_t *data;         ///< bitstream data
> > +    int data_size;         ///< data size
> > +    int bits_used;         ///< bits used up to last decoded block
> > +    int mb_x, mb_y;        ///< coordinates of the last decoded block
> > +}SavedSliceInfo;
> 
> truncated slices are an error in the parser or demuxer and MUST be corrected
> there
> (note this is the most important complaint in this review)

They are stored in container that way and I have not found a way to determine
whether current slice is really previous slice tail or not while demuxing.
 
> > +
> > +/** Decoder context */
> > +typedef struct RV40DecContext{
> > +    MpegEncContext s;
> > +    int mb_bits;             ///< bits needed to read MB offet in slice header
> > +    int *intra_types_hist;   ///< old block types, used for prediction
> > +    int *intra_types;        ///< block types
> > +    int intra_types_stride;  ///< stride for block types data
> 
> why dont you use MpegEncContext.b4_stride ?

Will reuse MpegEncContext variables in this and other cases

[...] 
> > +    cw = av_mallocz(size * 2);
> > +    syms = av_malloc(size * 2);
> > +    bits = av_malloc(size);
> 
> these could as well use use arrays on the stack, simplifying the source

Table sizes vary from 16 to 1296 elements. I suspect that it's easier to allocate
memory instead of remembering where 1296 came from.

[...] 
> [...]
> > +/**
> > + * Real Video 4.0 inverse transform
> > + * Code is almost the same as in SVQ3, only scaling is different
> > + */
> > +static void rv40_intra_inv_transform(DCTELEM *block, const int offset){
> > +    int temp[16];
> > +    unsigned int i;
> 
> this still is duplicate of the svq3 code

I will think how to reuse SVQ3 transform for this case.

[...]
> [...]
> > +static int rv30_parse_slice_header(RV40DecContext *r, GetBitContext *gb, SliceInfo *si)
> > +{
> 
> please split rv30 and rv40 into seperate files (rv30/rv40/common code) and
> merge rv30/rv40 functions if they are nearly idenitcal

There are only two functions specific for RV30 - slice and macroblock type parsing.
 
[...]
> functions which are used in bith rv30 and rv40 should not be named rv40_...

It was sole RV40 decoder before I found out that it's only two functions that is
needed to add RV30 decoding capabilities. Will use prefix rv34_ for these.
 
> [...]
> > +static void rv40_postprocess(RV40DecContext *r)
> > +{
> 
> unused

But it should be used and will be used eventually.
 
> [...]
> > +    if(avctx->slice_count){
> > +        slice_count = avctx->slice_count;
> > +        slice_offset = avctx->slice_offset;
> 
> AVCodecContext.slice_count and slice_offset is deprecated, they break
> remuxing, cause thread issues with the demuxer writing these into the
> decoder context, ...

At least MPlayer and Xine pass slices gathered into one frame and my decoder
decodes both single slice and multiple slice data.
 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB