[Ffmpeg-devel] [PATCH] Chinese AVS video decoder

[Michael Niedermayer]

Hi

On Sun, Jul 02, 2006 at 01:26:07PM +0200, Stefan Gehrer wrote:
> Hi,
> 
> new patch versus current SVN attached, hopefully addressing
> all outstanding points. 
> 
> Von: Michael Niedermayer
> > the intra prediction code, or more precissely the border loading stuff
> > is a little messy, first things are stored in left/top_border* then they
> > are copied to top/left and then they are finaly used, can that 2 level
> > copy be avoided and pointers to left/top_border* be passed into the
> > intra prediction functions?
> 
> avoiding the first copy could be done by deblocking the whole frame after
> decoding, but I think the H.264 code was changed the other way round
> for good reason.

its possible to just deblock one block above and to the left of what is
decoded to avoid all copies but that wasnt what i had in mind


> 
> For the second copying: The advantage is that the mess is concentrated
> in load_intra*, while all the intra prediction functions are only a few
> lines and very readable. I did actually try to make these decisions about
> neighbouring pixels in the prediction functions themselves at one point, 
> but after a few functions I didn't like the look of it. Sure, there is
> a price on performance, but I think that stuff is not the most critical.

i didnt suggest to change the prediction functions, my suggestion was just
to not copy the contents of left/top_border* into yet another buffer but
instead pass a pointer to them ...


> 
> > 
> > ive added the new files to svn, ill commit the rest of your patch as soon
> > as you add a ff_ prefix (or av*_ for anything which should be vissible to 
> > the user application) to the new non static functions
> > 
> Done, with one change: Making those put/avg_pixels* functions in
> dsputil.c non-static and prefixed with ff_ seemed to turn into quite an
> intrusive patch, instead I moved four stub functions (the *_mc00 stuff)
> from cavsdsp.c to dsputil.c

thats ok, but making all functions in cavsdsp non static and initalizing
DSPContext with them in dsputil.c is messy, they should stay static and
the init should be done in a function in cavsdsp.c

furthermore your patch mixes cosmetical changes (variable renamings, comment
changes) and functional changes, such patches are not acceptable
ive commited the changes to mpeg12* and avcodec.h as they are independant
of the rest and they didnt mix cosmetic & functional changes


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is