[FFmpeg-devel] [PATCH] Indeo5 decoder

[Diego Biurrun]

On Mon, Apr 06, 2009 at 08:41:57PM +0200, Maxim wrote:
> 
> An updated patch attached fixing the most things showed by patcheck...

Documentation, build system and similar parts are now OK.

Note that this patch is huge.  Maybe you can find a way to split it,
that would speed up reviewing.  One way could be submitting the common
code separately first.

> --- libavcodec/indeo5.c	(Revision 0)
> +++ libavcodec/indeo5.c	(Revision 0)
> @@ -0,0 +1,842 @@
> +
> +static uint16_t     deq8x8_intra[5][24][64]; ///< dequant tables for intra blocks 8x8
> +static uint16_t     deq8x8_inter[5][24][64]; ///< dequant tables for inter blocks 8x8
> +static uint16_t     deq4x4_intra[24][16];    ///< dequant tables for intra blocks 4x4
> +static uint16_t     deq4x4_inter[24][16];    ///< dequant tables for inter blocks 4x4

dequant tables for 8x8 intra blocks

> +            if (!p) {
> +                band->quant_mat = (i > 1) ? i+1 : 0;
> +            } else {
> +                band->quant_mat = 5;
> +            }

redundant {}

> +                    if (band->inherit_mv){

missing space before {

> --- libavcodec/ivi_common.c	(Revision 0)
> +++ libavcodec/ivi_common.c	(Revision 0)
> @@ -0,0 +1,1335 @@
> +
> +/**
> + *  Two-dimensional inverse slant 8x8 transform.
> + *
> + *  @param  in      [in]  pointer to the vector of transform coefficients
> + *  @param  out     [out] pointer to the output buffer (frame)
> + *  @param  pitch   [in]  pitch to move to the next y line
> + *  @param  flags   [in]  pointer to the array of column flags:
> + *                        1 - non_empty column, 0 - empty one

non-empty?  same below..

> + *  This is a speed-up version of the inverse 2D slant transforms
> + *  for the case if there is a non-zero DC coeff and all AC coeffs are zero.

Are they charging you by the character? ;)

Just write "coefficients" instead of "coeffs".

> +void ff_ivi_process_empty_tile(AVCodecContext *avctx, IVIBandDesc *band, IVITile *tile, int32_t mv_scale)
> +{
> +    int             x, y, need_mc, mbn, blk, num_blocks, mv_x, mv_y, mc_type;
> +    int             offs, mb_offset, row_offset;

What is offs supposed to stand for?  "offsets"?  Maybe you can use a
slightly more descriptive name.

> +        for (y = 0; y < tile->height; y++) {
> +            memcpy(dst, src, tile->width*sizeof(int16_t));

extra good karma for spaces around the *

> --- libavcodec/indeo5data.h	(Revision 0)
> +++ libavcodec/indeo5data.h	(Revision 0)
> @@ -0,0 +1,191 @@
> +
> +#ifndef AVCODEC_IVI5DATA_H
> +#define AVCODEC_IVI5DATA_H

Use the filename as multiple inclusion guard.

Diego