[FFmpeg-devel] [PATCH] Indeo5 decoder

[Maxim]

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!

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

removed...

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

aligned

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

fixed

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

changed here and at some other places...

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

done...

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

broken up...

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

aligned

>   
>> +static av_cold int ivi5_decode_init(AVCodecContext *avctx)
>>     
>
> BTW, is the ivi5 prefix necessary on all those functions?
>   

Ok, it comes from my source that mixes both indeo4 and indeo5 functions.
Therefore I used this prefix for distinguish between them both...
Removed in the patch except the init/decode/close...

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

fixed

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

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

removed

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

added

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

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

fixed

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

added

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

done

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

fixed

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

done

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

added

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

done...

An updated patch is attached. It contains a replacement for the "switch"
statement that sets macroblock and block sizes as well (pointed by
Reimer)...

Regards
Maxim
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: indeo5patch.txt
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090330/a7e9ffdd/attachment.txt>