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

[Vitor Sessak]

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.

-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sws_fix10l_3.diff
Type: text/x-diff
Size: 6082 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20081021/968b266a/attachment.diff>