[FFmpeg-devel] [PATCH] Indeo5 decoder
Maxim
max_pole
Mon Mar 30 01:51:10 CEST 2009
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>
More information about the ffmpeg-devel
mailing list