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

[Sebastian Vater]

Michael Niedermayer a ?crit :
> On Wed, May 26, 2010 at 08:28:53PM +0200, Sebastian Vater wrote:
>   
>> 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.
>>     
>
> but why?
> why arent they in a single function that puts the values in the context?
>   

They actually are...extract_header. ;-)

But see the body in decode_frame*. I use them in initializers, too.
I could change that, but this would add more lines:
    const uint8_t *buf = avpkt->size >= 2 ? GET_PACKET_DATA(avpkt) : NULL;
    const int buf_size = avpkt->size >= 2 ? GET_PACKET_SIZE(avpkt) : 0;

would have to be changed to:
    uint8_t *buf;
    int buf_size;

And later after extract_header call:
    buf = GET_IMAGE_DATA(avpkt);
    buf_size = GET_IMAGE_SIZE(avpkt);

That would be possible, too. But twice (later three) times this overhead.
I think that makes code more lines than before (but on the other hand,
smaller in code size, I guess).

So again here, keep it as it is or change it to this way? ;-)

I guess you prefer changing to this way, right? ;-)

-- 

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