[Ffmpeg-devel] libswscale coding style

[Ivo]

Hi,

On Friday 27 April 2007 03:23, Marc Hoffman wrote:
> Diego Biurrun writes:
>  > I've just implemented pre-commit hooks for libswscale that check for
>  > tabs and trailing whitespace, same as for FFmpeg.
>  >
>  > I'm in the process of removing trailing whitespace and tabs and
>  > reformatting the files while I'm at it.

Great!

>  > What's the preferred style for inline ASM, things like
>  > rgb2rgb_template.c:100?

I would really like to see the "instr op, op\n\t" be replaced by something 
more readable, like:

    "   movq %%mm0, %%mm1       \n"
    "   movq %%mm1, %%mm2       \n"
    "   punpckldq 9%1, %%mm1    \n"

et cetera. Perhaps even align the operands, although that'll be a lot of 
work.

> I would like to see the reference codes and the MMX stuff completely
> separated out.  Make a C reference model and then allow that behavior
> to be overloaded with function pointers.  At the frame or band
> resolution it doesn't matter if its inlined or a set of 10 function
> calls to get there.
>
> rgb2rgb.c has a series of functions like this:
>
> static void rgb24to32_C (const uint8_t *src,uint8_t *dst,long src_size)
> {
[..snip..]
> }
>
> init_rgbfuncs (SWSOPS *ops) {
>
>    ops->rgb24to32 = rgb24to32_C;
>
>    ...
> ...
> }
>
>
> not sure what to do with the inline stuff it doesn't really help all that
> much.
>
> then have an rgb2rgb_mmx.c that has:
>
> static rgb24to32 (const uint8_t *src,uint8_t *dst,long src_size)
> {
[..snip..]
> }
>
> ff_mmx_init_rgbfuncs (SWSOPS *ops) {
>
>    ops->rgb24to32 = rgb24to32_MMX;
>
>    ...
>    ...
> }
>
> and then an
>
> rgb2rgb_altivec.c
>
> and then an
>
> rgb2rgb_bfin.c
>
> Then wrap the whole thing up in a single init who calls the
> init_rgbfuncs then the architecture specific one.

This will duplicate the C code in every source file (mmx, mmx2, altivec, 
3dnow, bfin, sse2 et cetera). I don't like that. If it has a bug, you are 
bound to miss one when fixing that bug. Perhaps the simd-functions could 
just call the corresponding C functions with adjusted src, dst and src_size 
after they are done? If you have them inlined, I doubt the two or three 
extra instructions outside the loop will cause a measurable speed penalty.

--Ivo