[FFmpeg-devel] [PATCH] swscale alpha channel support (was: [RFC] Alpha support)
Michael Niedermayer
michaelni
Sun Feb 8 19:54:14 CET 2009
On Sun, Feb 08, 2009 at 06:15:36PM +0100, C?dric Schieli wrote:
> 2009/2/8 Michael Niedermayer <michaelni at gmx.at>
[...]
> > [...]
> > > @@ -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
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(...)
>
> [...]
> > > @@ -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.
gcc does use pretty much the full set of valid addressing modes
that incudes base register + index register * scale constant + offset constant
>
> 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)
> );
i think this is valid but bloated
>
> 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)
> );
this is a little more risky
is there some special reason why you dont just do
context->u_temp= (whatever cast it needs) abuf0;
context->v_temp= (whatever cast it needs) abuf1;
?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
There will always be a question for which you do not know the correct awnser.
-------------- 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/20090208/fb8f90f9/attachment.pgp>
More information about the ffmpeg-devel
mailing list