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

[Robert Swain]

2008/10/20 Michael Niedermayer <michaelni at gmx.at>:
> On Mon, Oct 20, 2008 at 09:45:25AM -0700, Baptiste Coudurier wrote:
>> Hi Vitor,
>>
>> 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.
>> >
>> > [...]
>>  >
>> > -    }
>> > +        ((uint32_t *) dst)[i] = ((const uint32_t *) palette)[src[i]];
>>
>> I'll nitpick on style for once :>
>> When using casts, I tend to prefer avoiding useless spaces like:
>> ((const uint32_t*)dst)
>>
>> Just my 2 cents, how others prefer it ?
>
> I for sure prefer no space between ) and palette.
> not sure what i prefer between uint32_t and *

I prefer no space between ')' and the var name and a space between
types and pointers, i.e.:

((const uint32_t *)dst)[i]

Regards,
Rob