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

[Sebastian Vater]

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.

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

-- 

Best regards,
                   :-) Basty/CDGS (-: