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

[Michael Niedermayer]

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);
    if(is_422 || is_444){
    }
}

things should be implemented as:

static always_inline some_func_intenal(csx, csy){
    ...
}

some_func(){
    if(chroma_shift_x==1 && chroma_shift_y==1)
        some_func_intenal(1, 1);
    else
        some_func_intenal(chroma_shift_x, chroma_shift_y);
}

that way we could easily switch between a big & fast or small and 0.4%slower
version by just forcing some_func_intenal to be inlined or not

[...]

-- 
Michael