[FFmpeg-devel] [PATCH] IFF: Add the HAM stuff

[Sebastian Vater]

Michael Niedermayer a ?crit :
> On Wed, May 26, 2010 at 03:40:22PM +0200, Sebastian Vater wrote:
>   
>> +#define GET_PALETTE_DATA(x) ((uint8_t *) (x)->extradata + AV_RB16((x)->extradata))
>> +
>> +/**
>> + * Gets the size of CMAP palette data beyond the IFF extra context.
>> + * Please note that any value < 2 of IFF extra context or
>> + * raw extradata < 0 is considered as illegal extradata.
>> + */
>> +#define GET_PALETTE_SIZE(x) ((x)->extradata_size - AV_RB16((x)->extradata))
>> +
>> +/**
>> + * Gets the actual packet data after video properties which contains
>> + * the raw data beyond the IFF extra context.
>> + */
>> +#define GET_PACKET_DATA(x) ((uint8_t *) (x)->data + AV_RB16((x)->data))
>> +
>> +/**
>> + * Gets the size of raw data beyond the IFF extra context. Please note
>> + * that any value < 2 of either IFF extra context or raw packet data
>> + * is considered as an illegal packet.
>> + */
>> +#define GET_PACKET_SIZE(x) ((x)->size - AV_RB16((x)->data))
>>     
>
> i think these macros are ugly
> it seems to me that fields in IffContext or local variables where
> possible would be nicer
>   

Yes, that's what I do, I transfer the data from these to the IffContext.
Just thought that the macros increase readibility in actual code.

But maybe you meant more likely it will increase readibility to change
names of GET_PACKET_DATA/SIZE
to: GET_IMAGE_DATA/SIZE?
So it's consistent with GET_PALETTE_DATA/SIZE?

>> +/**
>> + * Extracts the IFF extra context and updates internal
>> + * decoder structures.
>> + *
>> + * @param avctx the AVCodecContext where to extract extra context to
>> + * @param avpkt the AVPacket to extract extra context from or NULL to use avctx
>> + * @return 0 in case of success, a negative error code otherwise
>> + */
>> +static int extract_header(AVCodecContext *const avctx,
>> +                          const AVPacket *const avpkt) {
>>     
>
> isnt const char *data, int size simpler than this and the if() below?
>   

Won't really simplify because avctx is needed for a) av_log and b) to
get the private IffContext.
Also avctx->extradata can have 0 size of palette data (grayscale) while
avpkt->data always
has to have image data, just compare:
+    if (avpkt) {
+    [...]
+        if (buf_size <= 1 || GET_PACKET_SIZE(avpkt) <= 1) {
+    [...]
+    } else {
+    [...]
+        if (buf_size <= 1 || GET_PALETTE_SIZE(avctx) < 0) {
+    [...]
+    }

So you see, there's a small difference between these two. But maybe that
can be solved by
passing a third parameter min_size? What do you think?

So it becomes instead of these two above:
        if (buf_size <= 1 || GET_PALETTE_SIZE(avctx) < min_size) {

However, this makes the function call longer (4 parameters instead of
just two):
avctx, data, size, min_size.

The question therefore is, what's the better trade off here...I'll leave
that to you.

-- 

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