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

[Michael Niedermayer]

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?


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

ok

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA
-------------- 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/d9f46689/attachment.pgp>