[FFmpeg-devel] [PATCH] HAM6/HAM8 support for IFF demuxer/decoder

[Måns Rullgård]

Sebastian Vater <cdgs.basty at googlemail.com> writes:

> Ronald S. Bultje a ?crit :
>> Hi,
>>
>> On Thu, May 6, 2010 at 12:51 PM, Sebastian Vater
>> <cdgs.basty at googlemail.com> wrote:
>>   
>>> They should be!
>>>
>>> x is NULL if the extradata context isn't available, i.e. if an old IFF
>>> demuxer is used in combination with the current new IFF decoder.
>>>
>>> In this case the old IFF demuxer just allocates (1 << number of
>>> bitplanes) * 3 bytes. Since maximum number of planes is 8, that will be
>>> 256*3 = 768 bytes.
>>>
>>> The GET_EXTRA_CONTEXT(x) checks if the extradata is at least 1 KB (to
>>> make sure there's not only the palette data like it happened in old
>>> versions) and returns NULL if there's no other extradata.
>>> Since I'm using uint8_t *ex = GET_EXTRA_CONTEXT(avctx) for example, that
>>> will return NULL to ex.
>>>
>>> The PUT/GET macros just check whether if the context is available (by
>>> comparing ex to non-NULL) and do the appreciate action if that's the
>>> case, otherwise GET returns always 0).
>>>     
>>
>> I think this is overengineering. I'd prefer if you'd just use 2-3
>> lines of doxy in iff.c or iff.h to explain the extradata layout, and
>> then use the bytestream.h API to fill it. There's no need for this
>> elaborate framework to get a few bytes from extradata during codec
>> initialization. bytestream.h is simpler, faster and less code.
>> And the NULL checks are unnecessary, simply if (!extradata) return
>> -EINVAL; and you're done (all imho).
>>   
>
> No, it should not do this!
>
> An extradata pointing to NULL is perfectly valid for the old decoder if
> either having number of bitplanes > 8 or with image having no CMAP chunk
> (grayscale image).
>
> In that case, the new decoder needs to handle all extra fields the old
> way (which is assuming them all zero) and can work without problems with
> the new demuxer in this patch.

So do it that way if extradata is null and the new way if it's non-null.

> The question remains though, if it still makes sense to use bytestream.h
> API when the NULL ptr checks are always required...

You still need to check it for null only once.

-- 
M?ns Rullg?rd
mans at mansr.com