[Ffmpeg-devel] Re: [RFC] mpeg2 422 encoding.

[Ivan Kalvachev]

2006/4/2, Michael Niedermayer <michaelni at gmx.at>:
> Hi
>
> On Sun, Apr 02, 2006 at 06:05:24PM +0200, Baptiste COUDURIER wrote:
> > Hi
> >
> > Michael Niedermayer wrote:
> > > [...]
> > >
> > > these are wrong, the following is correct
> > >
> > > if (!s->chroma_y_shift) {
> > >     put_bits(&s->pb, mbPatTable[cbp>>2][1], mbPatTable[cbp>>2][0]);
> > >     put_bits(&s->pb, 2, cbp & 3);
> > > }else
> > >     put_bits(&s->pb, mbPatTable[cbp][1], mbPatTable[cbp][0]);
> > >
> > >
> > > also note that the whole 422 encoding support must not slow the much more
> > > common 420 down by a "meassureable" amount
> > >
> >
> > Indeed, you always fix things up. It is now working great.
> > Here is the patch, if you see anything I have forgotten (probably many).
> >
> > I did not add 444 support, does anybody need that anyway ? And since
> > speed is really important... If it becomes needed I will implement it.
> >
> > According to specs, I think 422 interlaced dct coding needs to be
> > different from 420. Chroma should not be treated as frame :
> >
> > From ISO 13818-2 2000:
> > "In the case of chrominance blocks the structure depends upon the
> > chrominance format that is being used. In the case
> > of 4:2:2 and 4:4:4 formats (where there are two blocks in the vertical
> > dimension of the macroblock) the chrominance
> > blocks are treated in exactly the same manner as the luminance blocks.
> > However, in the 4:2:0 format the chrominance
> > blocks shall always be organised in frame structure for the purposes of
> > DCT coding. It should, however, be noted
> > that field based predictions may be made for these blocks which will, in
> > the general case, require that predictions for
> > 8 ? 4 regions (after half-sample filtering) must be made."
> >
> > Am I right ?
>
> IMHO yes
>
>
> >
> > codec_score needs to be changed for trellis quant.
> >
> > Is it acceptable to be applied ? I will add regression tests for 422
> > encoding. Regression tests for 420 are sucessful.
>
> this patch causes a 0.4% speed loss for mpeg4 encoding, this is IMHO not
> acceptable, speed losses add up and if we accept 10 such patches ...
>
> there should be no 420/422/444 checks in the speed critical code
>
> instead of
> some_func(){
>     block_num= is_444 ? 12 : (is_422 ? 8 : 6);

This reminds me that  pre-calculation of block_num and storing it in
some of the contexts could also give some speed up.
I don't like redundancy (e.g. chroma_format, pix_fmt, chroma_x/y and
block_num are all holding same information) but the small speed up is
probably of the half percent magnitude - the scale of the slowdown
this patch is causing ;)

One more advice for optimization. Avoid if(). Not very  complicated
formula is usually faster than single if().