[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