[FFmpeg-cvslog] r25066 - trunk/libavcodec/imgconvert.c

[Michael Niedermayer]

On Fri, Sep 10, 2010 at 11:23:03AM -0700, Baptiste Coudurier wrote:
> On 09/10/2010 11:06 AM, Reimar D?ffinger wrote:
>> On Fri, Sep 10, 2010 at 06:32:35PM +0100, M?ns Rullg?rd wrote:
>>> Reimar D?ffinger<Reimar.Doeffinger at gmx.de>  writes:
>>>
>>>> On Fri, Sep 10, 2010 at 05:28:27PM +0100, M?ns Rullg?rd wrote:
>>>>> Reimar D?ffinger<Reimar.Doeffinger at gmx.de>  writes:
>>>>>
>>>>>> On Fri, Sep 10, 2010 at 04:36:19PM +0100, M?ns Rullg?rd wrote:
>>>>>>> Why the FUCK do all 8-bit pixel formats claim they have a palette?
>>>>>>
>>>>>> Because anyone actually using (e.g. for displaying) the data
>>>>>> then doesn't need to add a special case for every single of them.
>>>>>
>>>>> But there is no palette.  How does pretending there is one when it
>>>>> isn't even allocated (hence the valgrind error) simplify anything?
>>>>
>>>> It's a bug if there is none, I know for sure that at least almost
>>>> all 8-bit formats did indeed have a palette in data[1] some time
>>>> ago.
>>>
>>> And just what is in that palette?
>>>
>>> In the case at hand, there is obviously _something_ there, but there's
>>> not enough of it.  Since you seem to think this makes sense, I invite
>>> you to fix the bug.  I'm not touching it further.
>>
>> I don't think there's anything at all there, it's just that crappy
>> hack of rawvideo decoder messing up (ok, ok, maybe I am being a bit
>> too extreme here).
>> av_image_fill_pointers just sets the palette pointer to right after
>> the image data. Which makes little sense and should probably at most
>> be done for PAL8 - otherwise it is inconsistent with avpicture_get_size.
>> Anyway that means in this case it points right into nothing.
>> rawdec.c needs a special case for all paletted formats and needs to set
>> data[1] to data initialized by ff_set_systematic_pal (normally
>> get_buffer would handle that, but that is not used for raw video).
>> A proper solution should of course clean the code up instead of making
>> it even more of a mess, but something like below code should be what
>> is needed:
> >
> > [...]
> >
>
> Well I never agreed to setting a palette for all 8 bits formats in the  
> first place.
> IMHO If code using the pixel formats wants a palette it can generate it,  
> instead of generating it systematically.

It simplifies code if all 8bit formats always have a palette.
otherwise (for example) every avfilter which wants to support 4 and 8bit
rgb/bgr formats would need code specific to each format or code setting
the palette, well a call to shared code at least ...
It is simpler to simply set that palette during picture allocation
we could even have static const pals that are used ...

either way, if you have a better suggestion (and patch that results in
simpler code) this is very welcome. But whichever system is used the
palette must be consistently handled, one decoder cannot just choose not
to set the palette while all others do.


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

It is not what we do, but why we do it that matters.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20100913/61c1b607/attachment.pgp>