[FFmpeg-cvslog] r27730 - in trunk/libswscale: swscale.c swscale_internal.h

[Michael Niedermayer]

On Sun, Oct 19, 2008 at 08:25:41PM +0200, Vitor Sessak wrote:
> Michael Niedermayer wrote:
>> On Sat, Oct 18, 2008 at 11:55:58PM +0200, Vitor Sessak wrote:
>>> vitor wrote:
>>>> Author: vitor
>>>> Date: Wed Oct  8 19:46:22 2008
>>>> New Revision: 27730
>>>> Log:
>>>> Add a new unscaled PAL8 -> RGB converter.
>>> 1000l for me for this patch. I tested it, but without noticing that 
>>> typing "make" do _not_ rebuild swscale-example. Actually it made a small 
>>> regression in {PAL,RGB8,BGR8,RGB4,BGR4}->{RGB32, BGR32}.
>>>
>>> Three possibilities:
>>>
>>> 1- Revert it
>>> 2- sws_fix10l.diff: fix the actual bug (in rgb2rgb.c)
>>> 3- sws_hack.diff: just fix my breakage
>> i do not think 2 or 3 fix the code
>> the palette is in RGB32 format IIRC so one side of the #ifdef has to be
>> wrong. Also the rgb24 would need ifdefs which it does not have, or the
>> palette would have to be converted into a differnt byte order or the
>> switch case would need to be changed a little
>
> AFAIK, before the palette was either in BGR32 or RGB32 depending on 
> alignment. Now I've made everything alignment independent. Also, if I 
> understand correctly the code, I think that swscaler uses a slightly 
> unusual definition of RGB/BGR.

the swscaler uses the same definition as lav* uses, and its very similar
to mplayers except some rgb <-> bgr flips
These definitions have also been choosen that way for efficiency and
simplicity.

anyway, this patch doesnt look correct either
i suggest you read the definition of the formats in avutils.h
also dont forget the big/little endian differences

review below

PS: just to make sure its clear, these *palette functions that where unused
by swscale before where buggy dont try to base your understanding of the
rgb formats on them.

[...]
> Index: libswscale/swscale.c
> ===================================================================
> --- libswscale/swscale.c	(revision 27788)
> +++ libswscale/swscale.c	(working copy)
> @@ -2739,7 +2739,10 @@
>              u= av_clip_uint8((RU*r + GU*g + BU*b + (257<<(RGB2YUV_SHIFT-1)))>>RGB2YUV_SHIFT);
>              v= av_clip_uint8((RV*r + GV*g + BV*b + (257<<(RGB2YUV_SHIFT-1)))>>RGB2YUV_SHIFT);
>              c->pal_yuv[i]= y + (u<<8) + (v<<16);
> -            c->pal_rgb[i]= b + (g<<8) + (r<<16);
> +            c->pal_rgb[(i<<2)    ]= b;
> +            c->pal_rgb[(i<<2) + 1]= g;
> +            c->pal_rgb[(i<<2) + 2]= r;
> +            c->pal_rgb[(i<<2) + 3]= 0;
>          }
>      }
>  
> Index: libswscale/swscale_internal.h
> ===================================================================
> --- libswscale/swscale_internal.h	(revision 27788)
> +++ libswscale/swscale_internal.h	(working copy)
> @@ -81,7 +81,7 @@
>      double param[2];
>  
>      uint32_t pal_yuv[256];
> -    uint32_t pal_rgb[256];
> +    uint8_t pal_rgb[4*256];
>  
>      int16_t **lumPixBuf;
>      int16_t **chrPixBuf;

not ok



> Index: libswscale/rgb2rgb.c
> ===================================================================
> --- libswscale/rgb2rgb.c	(revision 27788)
> +++ libswscale/rgb2rgb.c	(working copy)
> @@ -219,26 +219,8 @@
>  {
>      long i;
>  
> -/*
>      for (i=0; i<num_pixels; i++)
> -        ((unsigned *)dst)[i] = ((unsigned *)palette)[src[i]];
> -*/
> -
> -    for (i=0; i<num_pixels; i++)
> -    {
> -        #ifdef WORDS_BIGENDIAN
> -            dst[3]= palette[src[i]*4+2];
> -            dst[2]= palette[src[i]*4+1];
> -            dst[1]= palette[src[i]*4+0];
> -        #else
> -        //FIXME slow?
> -            dst[0]= palette[src[i]*4+2];
> -            dst[1]= palette[src[i]*4+1];
> -            dst[2]= palette[src[i]*4+0];
> -            //dst[3]= 0; /* do we need this cleansing? */
> -        #endif
> -        dst+= 4;
> -    }
> +        ((uint32_t *) dst)[i] = ((const uint32_t *) palette)[src[i]];
>  }
>  
>  void palette8tobgr32(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette)

ok


> @@ -246,18 +228,11 @@
>      long i;
>      for (i=0; i<num_pixels; i++)
>      {
> -        #ifdef WORDS_BIGENDIAN
> -            dst[3]= palette[src[i]*4+0];
> -            dst[2]= palette[src[i]*4+1];
> -            dst[1]= palette[src[i]*4+2];
> -        #else
>              //FIXME slow?
> -            dst[0]= palette[src[i]*4+0];
> +            dst[3]= 0;
> +            dst[2]= palette[src[i]*4+0];
>              dst[1]= palette[src[i]*4+1];
> -            dst[2]= palette[src[i]*4+2];
> -            //dst[3]= 0; /* do we need this cleansing? */
> -        #endif
> -
> +            dst[0]= palette[src[i]*4+2];
>          dst+= 4;
>      }
>  }

not ok, 32bit writes are faster than 3-4 8bit writes, its also wrong
on at least one endianness

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

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- 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-cvslog/attachments/20081019/519e150a/attachment.pgp>