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

[Vitor Sessak]

Michael Niedermayer wrote:
> On Wed, Oct 22, 2008 at 08:06:21PM +0200, Vitor Sessak wrote:
>> Michael Niedermayer wrote:
>>> On Tue, Oct 21, 2008 at 10:06:58PM +0200, Vitor Sessak wrote:
>>>> Michael Niedermayer wrote:
>>>>> On Mon, Oct 20, 2008 at 08:05:26PM +0200, Vitor Sessak wrote:
>>>>>> Michael Niedermayer wrote:
>>>>>>> On Mon, Oct 20, 2008 at 06:27:36PM +0200, Vitor Sessak wrote:
>>>>>>>> Michael Niedermayer wrote:
>>>>>>>>> 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
>>>>>>>> Documented header files are a bliss =)
>>>>>>>>
>>>>>>>> Hopefully correct version attached.
>>>>>>>>
>>>>>>>> -Vitor
>>>>>>> [...]
>>>>>>>>  void palette8tobgr32(const uint8_t *src, uint8_t *dst, long 
>>>>>>>> num_pixels, const uint8_t *palette)
>>>>>>>>  {
>>>>>>>>      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[1]= palette[src[i]*4+1];
>>>>>>>> -            dst[2]= palette[src[i]*4+2];
>>>>>>>> -            //dst[3]= 0; /* do we need this cleansing? */
>>>>>>>> -        #endif
>>>>>>>>  -        dst+= 4;
>>>>>>>> -    }
>>>>>>>> +    for (i=0; i<num_pixels; i++)
>>>>>>>> +        ((uint32_t *) dst)[i] =
>>>>>>>> +            bswap_32(((const uint32_t *) palette)[src[i]]) >> 8;
>>>>>>>>  }
>>>>>>> i think it would be faster to bswap 256 entries in palette than
>>>>>>> width*height, besides this would allow the same code to be used for
>>>>>>> rgb & bgr
>>>>>> Yes, but it would have to be done in swscale(). Isn't having both 
>>>>>> pal_rgb[] and pal_bgr[] in the context a bit of an overkill? 
>>>>> yep, what about a pal_native ?
>>>> ok.
>>>>
>>>>> there are btw 4 32bit rgb/bgr formats so its not 2 vs 1 but 4 vs 1
>>>>>> Also palette8tobgr32() is a public function, so I think it should 
>>>>>> either be fixed or removed...
>>>>> i dont mind removing broken and redundant functions
>>>> Both done.
>>> [...]
>>>>  /**
>>>> - * Palette is assumed to contain BGR32.
>>>> + * Palette is assumed to contain RGB32 (in CPU endianness).
>>>>   */
>>>>  void palette8torgb24(const uint8_t *src, uint8_t *dst, long num_pixels, 
>>>> const uint8_t *palette)
>>>>  {
>>>>      long i;
>>>> -/*
>>>> -    Writes 1 byte too much and might cause alignment issues on some 
>>>> architectures?
>>>> +
>>>>      for (i=0; i<num_pixels; i++)
>>>> -        ((unsigned *)(&dst[i*3])) = ((unsigned *)palette)[src[i]];
>>>> -*/
>>>> -    for (i=0; i<num_pixels; i++)
>>>>      {
>>>>          //FIXME slow?
>>>> +#ifdef WORDS_BIGENDIAN
>>>> +        dst[0]= palette[src[i]*4+1];
>>>> +        dst[1]= palette[src[i]*4+2];
>>>> +        dst[2]= palette[src[i]*4+3];
>>>> +#else
>>>>          dst[0]= palette[src[i]*4+2];
>>>>          dst[1]= palette[src[i]*4+1];
>>>>          dst[2]= palette[src[i]*4+0];
>>>> +#endif
>>>>          dst+= 3;
>>>>      }
>>>>  }
>>>>  +/**
>>>> + * Palette is assumed to contain RGB32 (in CPU endianness).
>>>> + */
>>>>  void palette8tobgr24(const uint8_t *src, uint8_t *dst, long num_pixels, 
>>>> const uint8_t *palette)
>>>>  {
>>>>      long i;
>>>> -/*
>>>> -    Writes 1 byte too much and might cause alignment issues on some 
>>>> architectures?
>>>> +
>>>>      for (i=0; i<num_pixels; i++)
>>>> -        ((unsigned *)(&dst[i*3])) = ((unsigned *)palette)[src[i]];
>>>> -*/
>>>> -    for (i=0; i<num_pixels; i++)
>>>>      {
>>>>          //FIXME slow?
>>>> +#ifdef WORDS_BIGENDIAN
>>>> +        dst[2]= palette[src[i]*4+1];
>>>> +        dst[1]= palette[src[i]*4+2];
>>>> +        dst[0]= palette[src[i]*4+3];
>>>> +#else
>>>>          dst[0]= palette[src[i]*4+0];
>>>>          dst[1]= palette[src[i]*4+1];
>>>>          dst[2]= palette[src[i]*4+2];
>>>> +#endif
>>>>          dst+= 3;
>>>>      }
>>>>  }
>>> You could merge these by changing the palette as well, that also would
>>> avoid the #ifdef WORDS_BIGENDIAN
>> Ok, if I understood what you meant.
>>
>>>> Index: libswscale/rgb2rgb.h
>>>> ===================================================================
>>>> --- libswscale/rgb2rgb.h	(revision 27788)
>>>> +++ libswscale/rgb2rgb.h	(working copy)
>>>> @@ -61,7 +61,7 @@
>>>>  extern void bgr8torgb8  (const uint8_t *src, uint8_t *dst, long 
>>>> src_size);
>>>>   -extern void palette8torgb32(const uint8_t *src, uint8_t *dst, long 
>>>> num_pixels, const uint8_t *palette);
>>>> +extern void palette8topacked32(const uint8_t *src, uint8_t *dst, long 
>>>> num_pixels, const uint8_t *palette);
>>>>  extern void palette8tobgr32(const uint8_t *src, uint8_t *dst, long 
>>>> num_pixels, const uint8_t *palette);
>>> this one has been removed ...
>>
>> 10l.
>>
>> New patch attached.
> 
> looks ok

Applied. I hope it have finally fixed the palette-based tests in FATE.

-Vitor