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

[Sebastian Vater]

Michael Niedermayer a ?crit :
> On Wed, May 26, 2010 at 08:11:54PM +0200, Sebastian Vater wrote:
>   
>> 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?
>>     
>
> i dont like any macrs for this no matter what the are called or what
> they do ;)
>   

Would you mind me changing GET_PACKET to GET_IMAGE and otherwise keep it
as is?

The reason is I have to evaluate these multiple times anyway, I call
them from decode_init and decode_frame, while decode_frame exists two
times and with IFF-ANIM going in, even three times.

Using a macro here is probably more readable and useful than writing
that always out.

Also these macros help me here to document it at the very beginning at
the source code, when I discard them, I have to add a documentation in
some other way (where I'm not sure yet how to do this in a proper way,
any idea?)

So just tell me, do you can live with keeping them or do you want really
me removing them?

>>>> +/**
>>>> + * 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.
>>     
>
> ive no strong oppinon on this but id say the variant with fewer lines of
> code is better
>   

Ok, then I'll change that to the avctx, data, size, min_size, ok?

-- 

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