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

Michael Niedermayer michaelni
Wed Oct 22 23:16:23 CEST 2008


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

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- 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/20081022/86a1d69f/attachment.pgp>



More information about the ffmpeg-cvslog mailing list