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

[Ronald S. Bultje]

Hi,

On Thu, May 6, 2010 at 12:51 PM, Sebastian Vater
<cdgs.basty at googlemail.com> wrote:
> Alex Converse a ?crit :
>> On Thu, May 6, 2010 at 11:52 AM, Sebastian Vater
>> <cdgs.basty at googlemail.com> wrote:
>>
>> +/**
>> + * Puts an uint8_t/uint16_t/uint32_t/uint64_t value using big endian mode
>> + * if, and only if the extradata context is allocated
>> + */
>> +#define PUT_EXCTX_INT8(x,offset,v) ?if ((x) != NULL)
>> ? ? ? ? ? ? ?\
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (((uint8_t *) (x))[(offset)] = (v))
>> +#define PUT_EXCTX_INT16(x,offset,v) if ((x) != NULL)
>> ? ? ? ? ? ? ?\
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? AV_WB16(((uint8_t *) (x)) +
>> (offset), (v))
>> +#define PUT_EXCTX_INT32(x,offset,v) if ((x) != NULL)
>> ? ? ? ? ? ? ?\
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? AV_WB32(((uint8_t *) (x)) +
>> (offset), (v))
>> +#define PUT_EXCTX_INT64(x,offset,v) if ((x) != NULL)
>> ? ? ? ? ? ? ?\
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? AV_WB64(((uint8_t *) (x)) +
>> (offset), (v))
>> +
>>
>> What do these macros actually accomplish? They are only used where x
>> is already uint8_t and not null.
>>
>
> 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).

Ronald