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

[Michael Niedermayer]

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 ;)


> 
> >> +/**
> >> + * 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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- 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/6dacf475/attachment.pgp>