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

[Michael Niedermayer]

Hi

On Tue, Apr 04, 2006 at 08:57:05PM +0200, Baptiste COUDURIER wrote:
> Hi
> 
> Michael Niedermayer wrote:
> > [...]
> > 
> > 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 ...
> > 
> > [...]
> >
> > 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
> > 
> > [...]
> > 
> 
> All right, I tried to implement it the way you suggested, I hope I
> understood it right.

you did, but gcc doesnt like your code, the mpeg4 mb encode code is 7% 
slower now (meassured with START/STOP_TIMER)

the problem with gcc likely is that there are various limits on inlining
and gcc includes the manual inlining from always_inline so that
forcing a function to be inlined will cause another function to no longer
be inlined

either
A. add always_inline everywhere (not realistic)
B. change the limits via command line paramaters to gcc
C. inline fewer functions
D. reduce the size of the inlined stuff
E. use the preprocessor to do the inlineing so that gcc doesnt know (ugly)
F. combination of the above

note, "nm mpegvideo.o" shows which functions are in there (the missing ones
where inlined everywhere), and IIRC there is a option for gcc too which 
prints warnings if something wasnt inlined which had a inline keyword ...


[...]
> @@ -711,6 +713,16 @@ void mpeg1_encode_mb(MpegEncContext *s,
>      }
>  }
>  
> +void mpeg1_encode_mb(MpegEncContext *s, DCTELEM block[12][64], int motion_x, int motion_y)
> +{
> +    if (s->chroma_format == CHROMA_420)
> +        mpeg1_encode_mb_internal(s, block, motion_x, motion_y,  6, 1, 1);
> +    else if(s->chroma_format == CHROMA_422)
> +        mpeg1_encode_mb_internal(s, block, motion_x, motion_y,  8, 1, 0);
> +    else
> +        mpeg1_encode_mb_internal(s, block, motion_x, motion_y, 12, 0, 0);
> +}

IMHO 420 should be the only special case to reduce the size
actually we could put them even under #ifndef CONFIG_SMALL and set
CONFIG_SMALL if --enable-small is passed to configure though these are
just random ideas if you are bored


[...]
>  void MPV_decode_mb(MpegEncContext *s, DCTELEM block[12][64]){
> -    if(s->avctx->lowres) MPV_decode_mb_internal(s, block, 1);
> -    else                  MPV_decode_mb_internal(s, block, 0);
> +    if(s->avctx->lowres){
> +        if(s->chroma_x_shift == 1 && s->chroma_y_shift == 1)
> +            MPV_decode_mb_internal(s, block, 1, 1, 1);
> +        else if(s->chroma_x_shift)
> +            MPV_decode_mb_internal(s, block, 1, 1, 0);
> +        else
> +            MPV_decode_mb_internal(s, block, 1, 0, 0);
> +    }else{
> +        if(s->chroma_x_shift == 1 && s->chroma_y_shift == 1)
> +            MPV_decode_mb_internal(s, block, 0, 1, 1);
> +        else if(s->chroma_x_shift)
> +            MPV_decode_mb_internal(s, block, 0, 1, 0);
> +        else
> +            MPV_decode_mb_internal(s, block, 0, 0, 0);
> +    }

again somwhow i think that normal 420 and _maybe_ lowres 420
should be the only special cases, its trivial to add the others later ...


> +static int sse_mb(MpegEncContext *s)
> +{
> +    if(s->chroma_format == CHROMA_420)
> +        return sse_mb_internal(s,  8,  8, 1, 1);
> +    else if(s->chroma_format == CHROMA_422)
> +        return sse_mb_internal(s,  8, 16, 1, 0);
> +    else
> +        return sse_mb_internal(s, 16, 16, 0, 0);

hmm, maybe sse should be left as it is without this _internal stuff ...

[...]

-- 
Michael

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