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

[Ronald S. Bultje]

Hi,

On Fri, May 14, 2010 at 10:06 PM, Sebastian Vater
<cdgs.basty at googlemail.com> wrote:
> Sebastian Vater a ?crit :
>> The following changes were made with this patch:
>> 1. Fixed and updated the documentation a lot.
>> 2. Merged lots of duplicate code into a single function
>> 3. Rearranged IFF extra context structure of packet
>> 4. Fixed a plane line size calculation in IFF-PBM
>
> Since, I've decided to provide a separate patch for step 2 I just resend
> the HAM support patch without doing step 2.
[..]
> +    uint8_t * ham_buf; // Temporary buffer for planar to chunky conversation
> +    uint32_t *ham_palbuf; // HAM decode table
> +    unsigned  compression; // Delta compression method used
> +    unsigned  bpp; // Bits per plane to decode
> +    unsigned  ham; // 0 if non-HAM or number of hold bits (6 for bpp > 6, 4 otherwise)
> +    unsigned  flags; // 1 for EHB, 0 is no extra half darkening
> +    unsigned  masking; // TODO: Masking method used

Please vertically align, and use doxy-compatible comments here where
appropriate (///< bla, not // bla). See "doxygen" output to get an
idea of what we're looking for in these kind of docs.

> +#define DECODE_HAM_PLANE32(x)       \

You can probably just make this a for(n=0;n<4;n++), gcc should unroll
that automatically. Then you don't need the macro.

> +static void decodehamplane32(uint32_t *dst,
> +                             const uint8_t  *buf,
> +                             const uint32_t *const pal,
> +                             unsigned buf_size)

You can merge the args on 2 lines, no need for 4. Also, please use
underscores, yourfunctionnamesaregettingalittlebittoolong.

>          if (avctx->pix_fmt == PIX_FMT_PAL8 || avctx->pix_fmt == PIX_FMT_GRAY8) {
>              for(y = 0; y < avctx->height; y++ ) {
>                  uint8_t *row = &s->frame.data[0][ y*s->frame.linesize[0] ];
>                  memset(row, 0, avctx->width);
> -                for (plane = 0; plane < avctx->bits_per_coded_sample && buf < buf_end; plane++) {
> +                for (plane = 0; plane < s->bpp && buf < buf_end; plane++) {
>                      decodeplane8(row, buf, FFMIN(s->planesize, buf_end - buf), plane);
>                      buf += s->planesize;
>                  }
>              }

Why?

> +#define GET_EXTRA_DATA(x) ((uint8_t *) (x)->data + AV_RB16((x)->data))
[..]
> +#define GET_EXTRA_SIZE(x) ((x)->size - AV_RB16((x)->data))

This is only used in libavcodec/iff.c and thus does not belong in iff.h.

Ronald