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

[Michael Niedermayer]

On Wed, May 26, 2010 at 09:15:32PM +0200, Sebastian Vater wrote:
> 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? ;-)

id like to get rid of the macros.
aka no grep #define matches ;)

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100526/67c5c2fe/attachment.pgp>