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

[Michael Niedermayer]

On Sun, Feb 08, 2009 at 10:21:59PM +0100, C?dric Schieli wrote:
> 2009/2/8 Michael Niedermayer <michaelni at gmx.at>
> 
> > On Sun, Feb 08, 2009 at 06:15:36PM +0100, C?dric Schieli wrote:
> >
> [...]
> 
> > > Yes, it's clearer that way. I can also enclose most of the new code in
> > > CONFIG_ALPHA ifs/ifdefs
> >
> > i prefer the least amount of #if* thus please only do that where it is
> > really
> > helping speed or object size
> > if(CONFIG_ALPHA && ...) is also much prettier than
> > #if CONFIG_ALPHA
> >    if(...)
> >
> 
> Sadly, adding a --enable-alpha option badly defines ARCH_ALPHA besides
> defining CONFIG_ALPHA.
> Is CONFIG_ALPHA_CHANNEL ok ? or maybe CONFIG_SWSCALE_ALPHA ?

CONFIG_SWSCALE_ALPHA is ok

[...]
> @@ -693,24 +728,28 @@
>          int Y1= buf0[i2  ]<<1;\
>          int Y2= buf0[i2+1]<<1;\
>  
> -#define YSCALE_YUV_2_RGB1_C(type) \
> -    YSCALE_YUV_2_PACKED1_C\
> -    type *r, *b, *g;\
> +#define YSCALE_YUV_2_RGB1_C(type,alpha) \
> +    YSCALE_YUV_2_PACKED1_C(type,alpha)\
>      r = (type *)c->table_rV[V];\
>      g = (type *)(c->table_gU[U] + c->table_gV[V]);\
>      b = (type *)c->table_bU[U];\
>  
> -#define YSCALE_YUV_2_PACKED1B_C \
> +#define YSCALE_YUV_2_PACKED1B_C(type,alpha) \
>      for (i=0; i<(dstW>>1); i++){\
>          const int i2= 2*i;\
>          int Y1= buf0[i2  ]>>7;\
>          int Y2= buf0[i2+1]>>7;\
>          int U= (uvbuf0[i     ] + uvbuf1[i     ])>>8;\
>          int V= (uvbuf0[i+VOFW] + uvbuf1[i+VOFW])>>8;\
> +        type av_unused *r, *b, *g;\
> +        int av_unused A1, A2;\
> +        if (alpha){\
> +            A1= abuf0[i2  ]>>7;\
> +            A2= abuf0[i2+1]>>7;\
> +        }\
>  
> -#define YSCALE_YUV_2_RGB1B_C(type) \
> -    YSCALE_YUV_2_PACKED1B_C\
> -    type *r, *b, *g;\
> +#define YSCALE_YUV_2_RGB1B_C(type,alpha) \
> +    YSCALE_YUV_2_PACKED1B_C(type,alpha)\
>      r = (type *)c->table_rV[V];\
>      g = (type *)(c->table_gU[U] + c->table_gV[V]);\
>      b = (type *)c->table_bU[U];\
> @@ -768,17 +807,52 @@
>  #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){\

> +            int needAlpha = CONFIG_ALPHA_CHANNEL ? (int)c->alpPixBuf : 0;\

(int)c->alpPixBuf could be 0 with 32bit int and 64bit pointers

int needAlpha = CONFIG_ALPHA_CHANNEL && c->alpPixBuf;\
seems safer


[...]
> @@ -3131,6 +3254,13 @@
>          av_freep(&c->chrPixBuf);
>      }
>  
> +    if (CONFIG_ALPHA_CHANNEL && c->alpPixBuf)
> +    {

new code should have the { placed on the same line as the statement, like in
K&R
if(CONFIG_ALPHA_CHANNEL && c->alpPixBuf){


[...]
> @@ -1012,11 +1039,30 @@
>          if (c->flags & SWS_ACCURATE_RND){
>              switch(c->dstFormat){
>              case PIX_FMT_RGB32:

> +                if(CONFIG_ALPHA_CHANNEL && c->alpPixBuf)
> +                {

{ placement


> +                    YSCALEYUV2PACKEDX_ACCURATE
> +                    YSCALEYUV2RGBX
> +                    "movq                      %%mm2, "U_TEMP"(%0)  \n\t"
> +                    "movq                      %%mm4, "V_TEMP"(%0)  \n\t"
> +                    "movq                      %%mm5, "Y_TEMP"(%0)  \n\t"
> +                    YSCALEYUV2PACKEDX_ACCURATE_YA(ALP_MMX_FILTER_OFFSET)

> +                    "movq                      %%mm3, %%mm2         \n\t"
> +                    "movq               "Y_TEMP"(%0), %%mm5         \n\t"
> +                    "psraw                        $3, %%mm1         \n\t"
> +                    "psraw                        $3, %%mm7         \n\t"
> +                    "packuswb                  %%mm7, %%mm1         \n\t"
> +                    "movq                      %%mm1, %%mm7         \n\t"
> +                    WRITEBGR32(%4, %5, %%REGa)

the 2 register-register moves look avoidable if WRITEBGR32 wouldnt use
fixed registers but rather have the used args passed as arguments.

also note, a patch changing WRITEBGR32 to take its registers as arguments
should be seperate. Also some of the changes you did like spliting macros
could be seperate (its trivial and could be commited already)

also iam assuming all the code has been tested?
and when already speaking about testing things should be tested on
x86_32 and 64


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090210/800ed31a/attachment.pgp>