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

[Alex Converse]

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.
>

and the only time you actually use PUT_* is after if(ex)

+    if (!st->codec->extradata) {
+        st->codec->extradata_size = IFF_EXTRA_CONTEXT_SIZE;
+        st->codec->extradata = av_malloc(IFF_EXTRA_CONTEXT_SIZE +
FF_INPUT_BUFFER_PADDING_SIZE);
+        if (!st->codec->extradata)
+            return AVERROR(ENOMEM);
+    }
+    ex = GET_EXTRA_CONTEXT(st->codec);
+    if (ex) {
+        memset(ex, 0, IFF_EXTRA_CONTEXT_SIZE);
+        PUT_EXCTX_INT8 (ex, EXTRA_CONTEXT_COMPRESSION,  compression);
+        PUT_EXCTX_INT8 (ex, EXTRA_CONTEXT_HAM,          (screenmode &
CAMG_HAM) != 0);
+        PUT_EXCTX_INT8 (ex, EXTRA_CONTEXT_EHB,          (screenmode &
CAMG_EHB) != 0);
+        PUT_EXCTX_INT8 (ex, EXTRA_CONTEXT_MASKING,      masking);
+        PUT_EXCTX_INT16(ex, EXTRA_CONTEXT_TRANSPARENCY, transparency);
+    }

Also in that same block you've just malloced your extradata so there
is no need to check it's size, and you return ENOMEM if that malloc
fails so you don't even need that NULL check.

> 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).
>
> PUT will simply do nothing if ex is NULL.

PUT will not be called if ex is null