[FFmpeg-devel] [PATCH] Indeo5 decoder

[Diego Biurrun]

On Fri, Mar 27, 2009 at 04:41:18PM +0100, Maxim wrote:
> 
> Any further reviews plz!

Your patch is missing a documentation and changelog update.

> --- libavcodec/indeo5.c	(Revision 0)
> +++ libavcodec/indeo5.c	(Revision 0)
> @@ -0,0 +1,846 @@
> +
> +/**
> + * @file indeo5.c

Add the subdirectory prefix, i.e. libavcodec/.

> + * This is an open-source decoder for Indeo Video Interactive v5

I think stating that it is open source is redundant within FFmpeg
doxygen documentation.

> +    RVMapDesc       rvmap_tabs[9]; ///< local changeable copy of the static rvmap tables
> +    IVIPlaneDesc    planes[3]; ///< color planes
> +    uint32_t        frame_type;
> +    uint32_t        prev_frame_type; ///< frame type of the previous frame
> +    uint32_t        frame_num;
> +    uint32_t        pic_hdr_size; ///< picture header size in bytes
> +    uint8_t         frame_flags;
> +    uint16_t        check_sum; ///< frame checksum
> +
> +    int16_t         mb_huff_sel; ///< MB huffman table selector
> +    IVIHuffDesc     mb_huff_desc; ///< MB table descriptor associated with the selector above
> +    VLC             *mb_vlc; ///< ptr to the vlc table for decoding macroblock data
> +    VLC             mb_vlc_cust; ///< custom macroblock vlc table

The comments could be vertically aligned.

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

ditto

> +static av_cold void ivi5_build_dequant_tables ()

Leave out the space before the ( to conform with K&R, same below.

Functions without parameters should have 'void' as parameter list.

> +/**
> + *  decode indeo5 GOP (Group of pictures) header
> + *  this header is present in key frames only
> + *  it defines parameters for all frames in a GOP

Please capitalize sentences and end them in periods.  This reads awkward
because it is not clear where sentences begin and end.  same below.

> +                av_log(avctx,AV_LOG_ERROR,"Extended transform info encountered!\n");
> +            if (get_bits(&ctx->gb,2)) {
> +                av_log(avctx,AV_LOG_ERROR,"End marker missing!\n");

I would place spaces after the commas.  It makes the code easier to read
and conforms to K&R.  Also, you inconsistently do it in some places, but
not in others.  Please do it everywhere.

> +/**
> + *  decode info (block type, cbp, quant delta, motion vector) for all macroblocks in the current tile

needlessly long line

> +static int ivi5_decode_mb_info (IVI5DecContext *ctx, IVIBandDesc *band, IVITile *tile, AVCodecContext *avctx)

ditto and same in other places

> +    mb = tile->mbs;
> +    ref_mb = tile->ref_mbs;
> +    offs = tile->ypos * band->pitch + tile->xpos;
> +    mv_x = mv_y = 0;
> +    mv_scale = (ctx->planes[0].bands[0].mb_size >> 3) - (band->mb_size >> 3); /* factor for motion vector scaling */

Some of this could be aligned.

> +static av_cold int ivi5_decode_init(AVCodecContext *avctx)

BTW, is the ivi5 prefix necessary on all those functions?

> +static int ivi5_decode_frame(AVCodecContext *avctx,
> +                               void *data, int *data_size,
> +                               const uint8_t *buf, int buf_size)

weird indentation


> +AVCodec indeo5_decoder = {
> +    .name = "indeo5",
> +    .type = CODEC_TYPE_VIDEO,
> +    .id = CODEC_ID_INDEO5,
> +    .priv_data_size = sizeof(IVI5DecContext),
> +    .init = ivi5_decode_init,
> +    .close = ivi5_decode_close,
> +    .decode = ivi5_decode_frame,
> +    .long_name = NULL_IF_CONFIG_SMALL("Intel Indeo Video Interactive 5"),

This could be aligned.

> --- libavcodec/ivi_common.c	(Revision 0)
> +++ libavcodec/ivi_common.c	(Revision 0)
> @@ -0,0 +1,1453 @@
> +/*
> + * Common functions for Indeo Video Interactive codecs (indeo4.c and indeo5.c)

Leave out the filenames, they have a tendency to be overlooked when
files get moved around.

> +/**
> + * @file ivi_common.c

directory prefix

> +            mb->xpos = x;
> +            mb->ypos = y;
> +            mb->buf_offs = mb_offset;
> +
> +            mb->type = 1; /* set the macroblocks type = INTER */
> +            mb->cbp = 0; /* all blocks are empty */
> +
> +            if (!band->qdelta_present && band->plane == 0 && band->band_num == 0) {
> +                mb->q_delta = band->glob_quant;
> +                mb->mv_x = 0;
> +                mb->mv_y = 0;

align

> +const IVIHuffDesc blk_huff_desc[8] = {
[...]

All these tables could profit from being aligned.

> --- libavcodec/ivi_common.h	(Revision 0)
> +++ libavcodec/ivi_common.h	(Revision 0)
> @@ -0,0 +1,195 @@
> +/*
> + * Common functions for Indeo Video Interactive codecs (indeo4.c and indeo5.c)

see above about file names

> +/**
> + * @file ivi_common.h

directory prefix

> + * This file contains structures and macros shared by both Indeo4 and Indeo5 decoders.
> + */
> +
> +#ifndef AVCODEC_IVI_COMMON_H
> +#define AVCODEC_IVI_COMMON_H
> +
> +#include <stdint.h>
> +
> +typedef struct {
> +    int16_t     xpos;
> +    int16_t     ypos;
> +    uint32_t    buf_offs; ///< address in the output buffer for this mb
> +    uint8_t     type; ///< macroblock type
> +    uint8_t     cbp; ///< coded block pattern
> +    uint8_t     q_delta; ///< quant delta
> +    int8_t      mv_x; ///< motion vector (x component)
> +    int8_t      mv_y; ///< motion vector (y component)

The comments could be aligned, same below.

> +/**
> + *  this structure a color plane (luma or chroma)

this sentence no verb

> +static inline int ivi_pic_config_cmp(IVIPicConfig *str1, IVIPicConfig *str2)
> +{
> +    return (str1->pic_width    != str2->pic_width    || str1->pic_height    != str2->pic_height ||
> +            str1->chroma_width != str2->chroma_width || str1->chroma_height != str2->chroma_height ||
> +            str1->tile_width   != str2->tile_width   || str1->tile_height   != str2->tile_height ||
> +            str1->luma_bands   != str2->luma_bands   || str1->chroma_bands  != str2->chroma_bands);

The || could be aligned.

> --- libavcodec/indeo5data.h	(Revision 0)
> +++ libavcodec/indeo5data.h	(Revision 0)
> @@ -0,0 +1,211 @@
> +
> +/**
> + * @file indeo5data.h

directory prefix

> +} ivi5_common_pic_sizes[15] = {{640, 480}, {320, 240}, {160, 120}, {704, 480}, {352, 240}, {352, 288}, {176, 144},
> +                          {240, 180}, {640, 240}, {704, 240}, {80, 60}, {88, 72}, {0, 0}, {0, 0}, {0, 0}};

This could be aligned.

Diego