[FFmpeg-devel] [PATCH] swscale alpha channel support

Fri Feb 27 23:30:25 CET 2009

2009/2/27 Michael Niedermayer <michaelni at gmx.at>:
> On Fri, Feb 27, 2009 at 06:28:38PM +0100, C?dric Schieli wrote:
>> Hi all,
>>
[...]
>>
>> #3 : sws_treat_yuva420p_as_yuv420p.patch
>> Use (srcFormat==yuv420p) codepath for the (srcFormat=yuva420p) case in
>> various places where it is appropriate
>
> ok

Applied

>
>
>>
>> #4 : sws_default_alpha_value.patch
>> When converting from a non alpha format to an alpha format, defaults
>> to all ones rather than all zeroes
>> This patch introduces some (small) differences in swscale-exemple
>> output for RGB32_1 and BGR32_1 (see swscale-example.log.diff) that I
>> can't reproduce manually
>
> these look rather harmless, maybe some uninitialized var or missing emms,
> i doubt there is a bug in your code
> anyway comments below
>

It can't be missing emms as the same differences occurs when built
with --disable-mmx

[...]
>>
>> #8 : sws_output_yuva420p.patch
>> Now that yuva420p can be scaled, it can be added to output supported format
>
> ok

This one makes swscale-example crash if #9 is not applied, so it will wait

[...]
>>
>>
>> One remaining issue is a strange difference on x86_64 for some cases
>> (see swscale-example.x86_64.log.diff). After some debugging, it seems
>> it comes from #9. I'll invetigate more on this one.
>
> until that is solved #9 (or whatever causes it) is not acceptable
> what has to be noted is that all of the differences are for flags=1
> and involve either yuv410p or yuv411p
>

Yes, that's what I noticed too.
I've adapted it to work without the others applied, and the
differences remain, so the bug is either in the patch itself, or
already in libswscale

> [...]
>
>
>
> #2
>> Index: ffmpeg/libavcodec/imgconvert.c
>> ===================================================================
>> --- ffmpeg.orig/libavcodec/imgconvert.c ? ? ? 2009-02-27 11:35:30.086183618 +0100
>> +++ ffmpeg/libavcodec/imgconvert.c ? ?2009-02-27 11:35:49.874181190 +0100
>> @@ -721,7 +721,7 @@ int avpicture_layout(const AVPicture* sr
>> ? ? ? ? ? ? ? ? ? ? ? unsigned char *dest, int dest_size)
>> ?{
>> ? ? ?const PixFmtInfo* pf = &pix_fmt_info[pix_fmt];
>> - ? ?int i, j, w, h, data_planes;
>> + ? ?int i, j, w, ow, h, oh, data_planes;
>> ? ? ?const unsigned char* s;
>> ? ? ?int size = avpicture_get_size(pix_fmt, width, height);
>>
>
>> @@ -753,8 +753,13 @@ int avpicture_layout(const AVPicture* sr
>>
>> ? ? ?for (i=0; i<data_planes; i++) {
>> ? ? ? ? ? if (i == 1) {
>> + ? ? ? ? ? ? ow = w;
>> + ? ? ? ? ? ? oh = h;
>
> id do this before the loop
>

That doesn't change much, but why not.
Patch updated.

> [...]
>
>
> #4
> [...]
>> @@ -1661,11 +1661,17 @@ static inline void RENAME(name ## _half)
>> ? ? ?int i;\
>> ? ? ?for (i=0; i<width; i++)\
>> ? ? ?{\
>> - ? ? ? ?int pix0= ((type*)src)[2*i+0];\
>> - ? ? ? ?int pix1= ((type*)src)[2*i+1];\
>> - ? ? ? ?int g= (pix0&maskg)+(pix1&maskg);\
>> - ? ? ? ?int b= ((pix0+pix1-g)&(maskb|(2*maskb)))>>shb;\
>> - ? ? ? ?int r= ((pix0+pix1-g)&(maskr|(2*maskr)))>>shr;\
>> + ? ? ? ?int pix0, pix1, g, b, r;\
>> + ? ? ? ?if (alpha){\
>> + ? ? ? ? ? ?pix0= ((type*)src)[2*i+0]&(maskr|maskg|maskb);\
>> + ? ? ? ? ? ?pix1= ((type*)src)[2*i+1]&(maskr|maskg|maskb);\
>> + ? ? ? ?}else{\
>> + ? ? ? ? ? ?pix0= ((type*)src)[2*i+0];\
>> + ? ? ? ? ? ?pix1= ((type*)src)[2*i+1];\
>> + ? ? ? ?}\
>> + ? ? ? ?g= (pix0&maskg)+(pix1&maskg);\
>> + ? ? ? ?b= ((pix0+pix1-g)&(maskb|(2*maskb)))>>shb;\
>> + ? ? ? ?r= ((pix0+pix1-g)&(maskr|(2*maskr)))>>shr;\
>
> int pix0= ((type*)src)[2*i+0];\
> int pix1= ((type*)src)[2*i+1];\
> int g= (pix0&(maskg|maska))+(pix1&(maskg|maska));\
> int b= ((pix0+pix1-g)&(maskb|(2*maskb)))>>shb;\
> int r= ((pix0+pix1-g)&(maskr|(2*maskr)))>>shr;\
> g &= maskg|(2*maskg)
>
> 1 & less :)

Nice one.
Patch updated.

[...]
>> Index: ffmpeg/libswscale/swscale_internal.h
>> ===================================================================
>> --- ffmpeg.orig/libswscale/swscale_internal.h 2009-02-27 10:06:03.841935207 +0100
>> +++ ffmpeg/libswscale/swscale_internal.h ? ? ?2009-02-27 11:36:09.826184312 +0100
>> @@ -273,6 +273,13 @@ const char *sws_format_name(int format);
>> ? ? ? ? ?|| (x)==PIX_FMT_MONOBLACK ? \
>> ? ? ? ? ?|| (x)==PIX_FMT_MONOWHITE ? \
>> ? ? ?)
>> +#define isALPHA(x) ? ? ?( ? ? ? ? ? \
>> + ? ? ? ? ? (x)==PIX_FMT_BGR32 ? ? ? \
>> + ? ? ? ?|| (x)==PIX_FMT_BGR32_1 ? ? \
>> + ? ? ? ?|| (x)==PIX_FMT_RGB32 ? ? ? \
>> + ? ? ? ?|| (x)==PIX_FMT_RGB32_1 ? ? \
>> + ? ? ? ?|| (x)==PIX_FMT_YUVA420P ? ?\
>> + ? ?)
>>
>> ?static inline int fmt_depth(int fmt)
>> ?{
>
> ok

Applied

>
>
> #6
> [...]
>> +#define YUV2RGBFUNC(func_name, dst_type, alpha) \
>> ?static int func_name(SwsContext *c, uint8_t* src[], int srcStride[], int srcSliceY, \
>> ? ? ? ? ? ? ? ? ? ? ? int srcSliceH, uint8_t* dst[], int dstStride[]){\
>> ? ? ?int y;\
>> ?\
>
>> - ? ?if (c->srcFormat == PIX_FMT_YUV422P) {\
>> + ? ?if (!alpha && c->srcFormat == PIX_FMT_YUV422P) {\
>> ? ? ? ? ?srcStride[1] *= 2;\
>> ? ? ? ? ?srcStride[2] *= 2;\
>> ? ? ?}\
>
> why?

To optimize out the check in the alpha==1 case

>
> [...]
>> Index: ffmpeg/libswscale/yuv2rgb_template.c
>> ===================================================================
>> --- ffmpeg.orig/libswscale/yuv2rgb_template.c 2009-02-27 11:36:06.914181648 +0100
>> +++ ffmpeg/libswscale/yuv2rgb_template.c ? ? ?2009-02-27 15:40:20.681930923 +0100
>> @@ -451,3 +451,23 @@ static inline int RENAME(yuv420_rgb32)(S
>>
>> ? ? ?YUV2RGB_ENDLOOP(4)
>> ?}
>> +
>> +#if CONFIG_SWSCALE_ALPHA
>> +static inline int RENAME(yuva420_rgb32)(SwsContext *c, uint8_t* src[], int srcStride[], int srcSliceY,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int srcSliceH, uint8_t* dst[], int dstStride[]){
>> + ? ?int y, h_size;
>> +
>> + ? ?YUV2RGB_LOOP(4)
>> +
>> + ? ? ? ?*(uint8_t **)(&c->u_temp) = src[3] + y*srcStride[3] - 2*index; /* pa-2index */
>> + ? ? ? ?YUV2RGB_INIT
>> + ? ? ? ?YUV2RGB
>> + ? ? ? ?"mov ? ? ? ? ? ? ? %5, "ESP_OFFSET"(%4);" /* Backup py-2index */
>> + ? ? ? ?"mov ? ? "U_TEMP"(%4), %5;" ? ? ? ? ? ? ? /* Fetch pa-2index */
>> + ? ? ? ?"movq ? ? (%5, %0, 2), %%mm3;" ? ? ? ? ? ?/* Load 8 A A7 A6 A5 A4 A3 A2 A1 A0 */
>> + ? ? ? ?"mov "ESP_OFFSET"(%4), %5;" ? ? ? ? ? ? ? /* Restore py-2index */
>> + ? ? ? ?RGB_PLANAR2PACKED32
>> +
>> + ? ?YUV2RGB_ENDLOOP(4)
>> +}
>> +#endif
>
> there are 7 registers the asm uses just 6 so this save&restore should be
> avoidable even if we then have to fallback to C when HAVE_7REGS is not
> set

Patch updated

Regards,
C?dric Schieli
-------------- next part --------------
A non-text attachment was scrubbed...
Name: avpicture_layout_yuva420p.patch
Type: text/x-patch
Size: 934 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090227/e2579efb/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sws_default_alpha_value.patch
Type: text/x-patch
Size: 14032 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090227/e2579efb/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sws_configure_alpha.patch
Type: text/x-patch
Size: 850 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090227/e2579efb/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sws_yuva2rgb.patch
Type: text/x-patch
Size: 11547 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090227/e2579efb/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sws_scale_alpha.patch
Type: text/x-patch
Size: 51133 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090227/e2579efb/attachment-0004.bin>