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

[Michael Niedermayer]

Hi

On Mon, Sep 17, 2007 at 08:19:50AM +0300, Kostya wrote:
> 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.

how does mplayers demuxer do it? it does pass complete frames from what
i remember


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

then use
int bits[size];


[...]
> > [...]
> > > +static void rv40_postprocess(RV40DecContext *r)
> > > +{
> > 
> > unused
> 
> But it should be used and will be used eventually.

no, postprocessing does NOT belong in the decoder, a loop filter does but
the name doesnt sound like that
if you want to support rv30/40 postprocessing, write a filter using
the filter layer in soc, it would be interresting to see what effect it
has on h.264 and svq3 ...


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

mplayer and xine should be changed to pass the offsets or sizes within the
frame that makes remuxing work, fixes a few race conditions and so on
of course its not your job to fix xine and mplayer, but if you support
only the broken API noone will fix them

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- 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/20070917/240022e8/attachment.pgp>