[FFmpeg-devel] [PATCH] Indeo5 decoder

[Diego Biurrun]

On Mon, Mar 30, 2009 at 01:51:10AM +0200, Maxim wrote:
> Diego Biurrun schrieb:
> > 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.
> 
> Added. Sorry for such a greenhorn mistake!

See our new format and patch submission checklists:

http://www.ffmpeg.org/general.html#SEC29

> An updated patch is attached. It contains a replacement for the "switch"
> statement that sets macroblock and block sizes as well (pointed by
> Reimer)...
> 
> --- libavcodec/Makefile	(Revision 18229)
> +++ libavcodec/Makefile	(Arbeitskopie)
> @@ -109,6 +109,7 @@
>  OBJS-$(CONFIG_INDEO3_DECODER)          += indeo3.o
> +OBJS-$(CONFIG_INDEO5_DECODER)          += indeo5.o ivi_common.o

Have you checked this works standalone, i.e. there are no other
dependencies?

Try disabling all decoders and encoders and just enabling indeo5 please.

> --- libavcodec/indeo5.c	(Revision 0)
> +++ libavcodec/indeo5.c	(Revision 0)
> @@ -0,0 +1,836 @@
> +    ctx->prev_frame_type = ctx->frame_type;
> +    ctx->frame_type = get_bits(&ctx->gb, 3);

This could be aligned.

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

weird indentation

> +static av_cold int ivi5_decode_init(AVCodecContext *avctx)

Do you really need the ivi5 prefix for static functions?

> +    ctx->pic_conf.chroma_width  = (avctx->width  + 3)/4;
> +    ctx->pic_conf.chroma_height = (avctx->height + 3)/4;

extra good karma for spaces around the /

> +static int ivi5_decode_frame(AVCodecContext *avctx, void *data, int *data_size,

see above

> +static av_cold int ivi5_decode_close(AVCodecContext *avctx)

and again

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

common

> +static uint16_t inv_bits(const uint16_t val, const int nbits)
> +{
> +    uint16_t    res;

weird spaces

> +        t_width  = !p ? tile_width  : (tile_width  + 3)/4;
> +        t_height = !p ? tile_height : (tile_height + 3)/4;

good karma available here as well ;)

> +    switch(mc_type) {

K&R places a space before the ( after for/if/switch, same in other
places..

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

common

> +/**
> + *  this structure describes a macroblock

Capitalize and add period.

What sort of macroblock?  This is not very descriptive..

> +/**
> + *  this structure describes a tile

see above

> +    int32_t     is_empty; ///< = 1 if this tile doesn't contain any data
> +    uint32_t    data_size;///< size of the data in bytes
> +    uint32_t    num_MBs;  ///< number of macroblocks in this tile
> +    IVIMbInfo   *mbs;     ///< array of macroblock descriptors
> +    IVIMbInfo   *ref_mbs; ///< ptr to the macroblock descriptors of the reference tile

This would be more readable if you pushed the comments one column to the
right.

> +/**
> + *  this structure describes a wavelet band

and again

> +typedef struct {
> +    uint8_t         plane;    ///< plane number this band belongs to
> +    uint8_t         band_num; ///< band number
> +    uint32_t        width;
> +    uint32_t        height;
> +    int16_t         *buf;          ///< output buffer for this band
> +    int16_t         *ref_buf;      ///< reference frame buffer for motion compensation
> +    uint32_t        pitch;         ///< pitch associated with the buffer above
> +    uint8_t         is_empty;      ///< = 1 if this band doesn't contain any data
> +    uint8_t         mb_size;       ///< macroblock size
> +    uint8_t         blk_size;      ///< block size
> +    uint8_t         mc_resolution; ///< resolution of the motion compensation: 0 - fullpel, 1 - halfpel
> +    int8_t          inherit_mv;
> +    int8_t          inherit_qdelta;
> +    int8_t          qdelta_present; ///< tells if Qdelta signal is present in the bitstream (indeo5 only)
> +    uint8_t         quant_mat;      ///< dequant matrix
> +    uint8_t         glob_quant;     ///< quant base for this band
> +    const uint8_t   *scan;          ///< ptr to the scan pattern
> +
> +    uint8_t         huff_sel;     ///< huffman table for this band
> +    IVIHuffDesc     huff_desc;    ///< table descriptor associated with the selector above
> +    VLC             *blk_vlc;     ///< ptr to the vlc table for decoding block data
> +    VLC             blk_vlc_cust; ///< custom block vlc table
> +
> +    uint16_t        *dequant_intra; ///< ptr to dequant tables for intra blocks
> +    uint16_t        *dequant_inter; ///< ptr dequant tables for inter blocks
> +    uint8_t         num_corr;       ///< number of correction entries
> +    uint8_t         corr[61*2];     ///< rvmap correction pairs
> +    uint8_t         rvmap_sel;      ///< rvmap table selector
> +    RVMapDesc       *rv_map;        ///< ptr to the RLE table for this band
> +    uint16_t        num_tiles;      ///< number of tiles in this band
> +    IVITile         *tiles;         ///< array of tile descriptors

The comments could be aligned.

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

see above

> +int  ff_ivi_create_huff_from_desc (const IVIHuffDesc *cb, VLC *pOut, int flag);
> +int  ff_ivi_dec_huff_desc         (GetBitContext *gb, IVIHuffDesc *desc);
> +int  ff_ivi_huff_desc_cmp         (const IVIHuffDesc *desc1, const IVIHuffDesc *desc2);
> +void ff_ivi_huff_desc_copy        (IVIHuffDesc *dst, const IVIHuffDesc *src);
> +int  ff_ivi_init_planes           (IVIPlaneDesc *planes, const IVIPicConfig *cfg);
> +void ff_ivi_free_buffers          (IVIPlaneDesc *planes);
> +int  ff_ivi_init_tiles            (IVIPlaneDesc *planes, const int tile_width, const int tile_height);
> +int  ff_ivi_dec_tile_data_size    (GetBitContext *gb);
> +void ff_ivi_inverse_slant_8x8     (int32_t *in, int16_t *out, uint32_t pitch);
> +void ff_ivi_inverse_slant_4x4     (int32_t *in, int16_t *out, uint32_t pitch);
> +void ff_ivi_dc_slant_2d           (int32_t *in, int16_t *out, uint32_t pitch, int blk_size);
> +int  ff_ivi_decode_blocks         (GetBitContext *gb, IVIBandDesc *band, IVITile *tile);
> +void ff_ivi_process_empty_tile    (IVIBandDesc *band, IVITile *tile, int32_t mv_scale);
> +void ff_ivi_output_plane          (IVIPlaneDesc *plane, uint8_t *dst, int dst_pitch);

This looks somewhat weird aligned, it is not necessarily a bad idea, but
uncommon.

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

That's little detail, standard for what?

> +/**
> + *  indeo5 dequantization matrices consist of two tables: base table and scale table.

nit: matrixes, same in other places

I'm aware that both spellings are correct, but we settled on 'matrixes'
some time ago.

Diego