[FFmpeg-devel] [FFMPEG] [PATCH] cavs encoder

[Diego Biurrun]

On Sat, Oct 31, 2009 at 12:27:54AM +0800, zhihang wang wrote:
> [...]

Your patch is very big.  Try to split it into multiple parts to ease
review and get it applied more quickly.

Please get rid of the trailing whitespace and format your code in K&R
style.  Your style is quite inconsistent, please fix that, I made some
suggestions below, please implement them throughout your code.

> --- libavcodec/cavsenc.c	(revision 0)
> +++ libavcodec/cavsenc.c	(revision 0)
> @@ -0,0 +1,2461 @@
> +
> +/**
> + * @file cavsenc.c

missing directory prefix

> + * Chinese AVS video (AVS1-P2, JiZhun profile) encoder
> + * @author Stefan Gehrer <stefan.gehrer at gmx.de>, Zhihang Wang <zhihang.wang at gmail.com>, frank dong <sfdong at gmail.com>

Break needlessly long lines like these.

> +#define REC_FILE "rec.yuv"
> +#define OUTPUT_REC

Hmmm

> +#define AVS_DEBUG
> +#ifdef AVS_DEBUG
> +  #undef fprintf
> +  #undef printf
> +  #undef exit
> +#endif

Looks like debug cruft.

> +const enum cavs_mv_loc ff_cavs_mv_loc_c[4][4] =
> +{

Please use K&R style for new files, i.e. keep { on the same line for
struct and enum declarations.

> +static const uint16_t quant_mul[64] =
> +{
> +    32768,29775,27554,25268,23170,21247,19369,17770,
> +    16302,15024,13777,12634,11626,10624, 9742, 8958,
> +    8192, 7512, 6889, 6305, 5793, 5303, 4878, 4467,
> +    4091, 3756, 3444, 3161, 2894, 2654, 2435, 2235,
> +    2048, 1878, 1722, 1579, 1449, 1329, 1218, 1117,
> +    1024,  939,  861,  790,  724,  664,  609,  558,
> +    512,  470,  430,  395,  362,  332,  304,  279,
> +    256,  235,  215,  197,  181,  166,  152,  140

This table would be more readable aligned.

> +    DCTELEM *level_buf = s->levels[block];
> +    uint8_t *run_buf = s->runs[block];

The = could be aligned, same below in many places.

> +    for (coeff_num=0;coeff_num<s->total_coeff[block];coeff_num++)
> +    {

{ on the same line, same below

> +        level = abs(level_buf[coeff_num])-1;
> +        sign = (level_buf[coeff_num]>>31)&1;
> +        run = run_buf[coeff_num] - 1;

This would be more readable with spaces around operators, same below in
many places.

> +    for (block=0;block<4;block++)

Use some spaces please.

> +            if(intra)

if (, same below

> +static void recon_block(MpegEncContext *s, uint8_t *src, uint8_t *dst, int stride, int block, int qp, int intra)

Break long lines where easily possible.

> +    switch (pos)
> +    {
> +        case P_ee: //GG, j'
> +            break;

Indent the case at the same depth as the switch.

> +            for(i=0; i<4; i++)
> +            {
> +                dst[i] = (uint8_t)(((src[i] << 3) + dy * (src2[i] - src[i]) + 4) >> 3);
> +            }

pointless {}, same in many other places

> +                dst[i] = (uint8_t)((i00 * src[i  ] +
> +                            i01 * src[i+1] +
> +                            i10 * src2[i  ] +
> +                            i11 * src2[i+1] + 32) >> 6);

weird indentation

> +static int mbtype_to_code(MpegEncContext* const s, enum cavs_block blk_sz, int best_ref[4])
> +{
> +  if (s->pict_type == FF_P_TYPE)
> +  {
> +    if (blk_sz<= BLK_8X8)
> +    {
> +      return blk_sz;
> +    }
> +    else
> +    {
> +      return (5 + cbp_enc_tab[s->cbp][0]);
> +    }
> +  }

wrong indent level

> +    for(block=0; block<4; block++)

for (, same everywhere else

> +    if (avctx->refs>1)

spaces around operators please, same everywhere else

> +    for (block=0; block<ff_cavs_block_num[blk_sz]; block+=ff_cavs_loop_step[blk_sz])
> +    {
> +      tmp_diff[block] = s->dsp.sse[sse_fun](NULL, OFF_LUMA_SCAN(s->ey,block), OFF_LUMA_SCAN(s->cy,block), s->linesize, height);
> +      diff += tmp_diff[block];
> +    }

indentation

> +static inline void set_pmv(cavs_vector *mv, enum cavs_block size) {

{ on the next line for function declarations

> +    switch(size) {

switch (, same everywhere else

> +static inline int check_mv(MpegEncContext *s, cavs_vector new_mv, cavs_vector *pmv,
> +                     cavs_vector *mvmin, int *min_cost, enum cavs_block blk_sz, int blk_idx)
> +
> +static inline int bi_check_mv(MpegEncContext *s, cavs_vector fw_mv, cavs_vector *pmv,
> +                     cavs_vector *mvmin, cavs_vector *bw_mv, int *min_cost, enum cavs_block blk_sz, int blk_idx)

indentation, same below

> +#ifdef OUTPUT_REC
> +
> +static void cavs_frame_dump( MpegEncContext *s )
> +{
> +}
> +
> +#endif

debug cruft?

> --- libavcodec/Makefile	(revision 20419)
> +++ libavcodec/Makefile	(working copy)
> @@ -67,6 +67,8 @@
>  OBJS-$(CONFIG_CAVS_DECODER)            += cavs.o cavsdec.o cavsdsp.o \
>                                            mpeg12data.o mpegvideo.o
> +OBJS-$(CONFIG_CAVS_ENCODER)            += cavs.o cavsenc.o cavsdsp.o \
> +	                                      mpeg12data.o mpegvideo.o

Lose the tab.

Diego