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

Vitor Sessak vitor1001
Wed Oct 22 20:06:21 CEST 2008


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.

-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sws_fix10l_4.diff
Type: text/x-diff
Size: 6695 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20081022/e94a4731/attachment.diff>



More information about the ffmpeg-cvslog mailing list