[FFmpeg-devel] [PATCH] swscale alpha channel support (was: [RFC] Alpha support)

[Cédric Schieli]

2009/2/8 Michael Niedermayer <michaelni at gmx.at>

> On Sun, Feb 08, 2009 at 10:33:56AM +0100, C?dric Schieli wrote:
> > Hello,
> >
> >
> > This is a new (and hopefully final) version of my patch to bring alpha
> > channel support into swscale.
>
> patience, it looks already quite good, but patches of such size need a few
> revissions.
>

No problem, I'm not in a hurry.


> [...]
> > @@ -768,17 +807,41 @@
> >  #define YSCALE_YUV_2_ANYRGB_C(func, func2, func_g16, func_monoblack)\
> >      switch(c->dstFormat)\
> >      {\
> > -    case PIX_FMT_RGB32:\
> > -    case PIX_FMT_BGR32:\
> > -    case PIX_FMT_RGB32_1:\
> > -    case PIX_FMT_BGR32_1:\
> > -        func(uint32_t)\
> > -            ((uint32_t*)dest)[i2+0]= r[Y1] + g[Y1] + b[Y1];\
> > -            ((uint32_t*)dest)[i2+1]= r[Y2] + g[Y2] + b[Y2];\
> > -        }                \
> > +    case PIX_FMT_RGBA:\
> > +    case PIX_FMT_BGRA:\
>
> > +        if (!CONFIG_SMALL && c->alpPixBuf)\
> > +        {\
> > +            func(uint32_t,1)\
> > +                ((uint32_t*)dest)[i2+0]= r[Y1] + g[Y1] + b[Y1] +
> (A1<<24);\
> > +                ((uint32_t*)dest)[i2+1]= r[Y2] + g[Y2] + b[Y2] +
> (A2<<24);\
> > +            }\
> > +        }else{\
> > +            int needAlpha = (int)c->alpPixBuf;\
> > +            func(uint32_t,CONFIG_SMALL ? needAlpha : 0)\
> > +                ((uint32_t*)dest)[i2+0]= r[Y1] + g[Y1] + b[Y1] +
> ((CONFIG_SMALL && needAlpha) ? (A1<<24) : 0);\
> > +                ((uint32_t*)dest)[i2+1]= r[Y2] + g[Y2] + b[Y2] +
> ((CONFIG_SMALL && needAlpha) ? (A2<<24) : 0);\
> > +            }\
> > +        }\
> > +        break;\
>
> i think
> if(CONFIG_SMALL){
>    int needAlpha = CONFIG_ALPHA ? (int)c->alpPixBuf : 0;\
>    func(uint32_t, needAlpha)\
>        ((uint32_t*)dest)[i2+0]= r[Y1] + g[Y1] + b[Y1] + (needAlpha ?
> (A1<<24) : 0);\
>        ((uint32_t*)dest)[i2+1]= r[Y2] + g[Y2] + b[Y2] + (needAlpha ?
> (A2<<24) : 0);\
>    }\
> }else{
>    if(CONFIG_ALPHA && c->alpPixBuf){
>        func(uint32_t, 1)\
>            ((uint32_t*)dest)[i2+0]= r[Y1] + g[Y1] + b[Y1] + (A1<<24) : 0);\
>            ((uint32_t*)dest)[i2+1]= r[Y2] + g[Y2] + b[Y2] + (A2<<24) : 0);\
>        }\
>    }else{
>        func(uint32_t, 0)\
>            ((uint32_t*)dest)[i2+0]= r[Y1] + g[Y1] + b[Y1]);\
>            ((uint32_t*)dest)[i2+1]= r[Y2] + g[Y2] + b[Y2]);\
>        }\
>    }
> }
>
> would be clearer, more flexible and generate the same code, its a few lines
> more source sadly but i think the clarity outweights that.
>

Yes, it's clearer that way. I can also enclose most of the new code in
CONFIG_ALPHA ifs/ifdefs

[...]
> > @@ -1165,6 +1231,36 @@
> >          {
> >              //Note 8280 == DSTW_OFFSET but the preprocessor can't handle
> that there :(
> >              case PIX_FMT_RGB32:
> > +                if(c->alpPixBuf)
> > +                {
> > +                    __asm__ volatile(
> > +                    "mov %%"REG_b", "ESP_OFFSET"(%5)        \n\t"
> > +                    "mov        %6, %%"REG_b"               \n\t"
>
> you write into ebx here
>
> > +                    "mov %%"REG_b", "U_TEMP"(%5)            \n\t"
> > +                    "mov        %7, %%"REG_b"               \n\t"
>
> %7 is "m" and could use ebx to address
> you can prevent this by putting REG_b on the clobber list though this
> will likely not compile with -fPIC
>
> basically once you changed a register that is not on the output and not
> clobber list you no longer can access any "m" nor "r"
>

Ok. That's my first experiment with inline assembly (and x86 assembly), so I
have a lot to learn. I thought only "pointer" registers (esp, ebp) could be
used for "m" referencing.

also keep in mind push/pop change esp, and esp can be used in "m" as well
> so again no more "m" after push
>

I've already figured it out myself, painfully...

What about the following ?

                    __asm__ volatile(
                    "mov %%"REG_b", "ESP_OFFSET"(%5)        \n\t"
                    "mov        %6, %%"REG_b"               \n\t"
                    "mov %%"REG_b", "U_TEMP"(%5)            \n\t"
                    "mov "ESP_OFFSET"(%5), %%"REG_b"        \n\t"
                    "mov        %7, %%"REG_b"               \n\t"
                    "mov %%"REG_b", "V_TEMP"(%5)            \n\t"
                    "mov "ESP_OFFSET"(%5), %%"REG_b"        \n\t"
                    "mov        %4, %%"REG_b"               \n\t"
                    "push %%"REG_BP"                        \n\t"
                    [...]
                    :: "c" (buf0), "d" (buf1), "S" (uvbuf0), "D" (uvbuf1),
"m" (dest),
                    "a" (&c->redDither), "m" (abuf0), "m" (abuf1)
                    );

Or this one ?

                    __asm__ volatile(
                    "mov %%"REG_c", "ESP_OFFSET"(%5)        \n\t"
                    "mov        %6, %%"REG_c"               \n\t"
                    "mov %%"REG_c", "U_TEMP"(%5)            \n\t"
                    "mov        %7, %%"REG_c"               \n\t"
                    "mov %%"REG_a", "V_TEMP"(%5)            \n\t"
                    "mov "ESP_OFFSET"(%5), %%"REG_c"        \n\t"
                    "mov %%"REG_b", "ESP_OFFSET"(%5)        \n\t"
                    "mov        %4, %%"REG_b"               \n\t"
                    "push %%"REG_BP"                        \n\t"
                    [...]
                    :: "c" (buf0), "d" (buf1), "S" (uvbuf0), "D" (uvbuf1),
"m" (dest),
                    "a" (&c->redDither), "m" (abuf0), "m" (abuf1)
                    );


Regards,
C?dric Schieli
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel at mplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel