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

[Diego Biurrun]

On Mon, Nov 16, 2009 at 11:19:35PM +0800, zhihang wang wrote:
> Hi, ffmpeg friends. The attachment is the cavs encoder patch I modified
> according to the comments given before.
> The patch file is splitted into 6 logical parts so that you can review it
> easily.

Thanks.

> Welcome to your comments.

Ideally you would send each patch in a different mail.  This makes
reviewing easier.

> --- libavcodec/cavsenc.c	(revision 0)
> +++ libavcodec/cavsenc.c	(revision 0)
> @@ -0,0 +1,2351 @@
> +
> +#define REC_FILE "rec.yuv"
> +//#define OUTPUT_REC
> +#ifdef OUTPUT_REC
> +static void cavs_frame_dump(AVSContext *s);
> +#endif

Please avoid this ugly forward declaration.  Also, if this is just
debugging cruft, you should maybe just remove it.

> +#define ENCODE_FLAG 0
> +#define LAMBDA_ACCURACY_BITS    16

Place the 0 in the same column as the 16 for greater readability.

> +#define LAMBDA_FACTOR(lambda)   ((int32_t)((float)(1 << LAMBDA_ACCURACY_BITS)*(lambda) + 0.5f))
> +#define MV_COST(factor, bits)   (((factor)*(bits)) >> LAMBDA_ACCURACY_BITS)
> +
> +#define SAVE_MB_BEST min_mb_cost = cost;                              \
> +    best_blk_sz = blk_sz;                                             \
> +    memcpy(mb_best_ref, best_ref    ,  4*sizeof(int));                \
> +    memcpy(mb_best_mv , h->mv  , (2*4*3)*sizeof(cavs_vector));        \
> +    memcpy(mb_best_pmv, tmp_best_pmv,  4*sizeof(cavs_vector));
> +
> +#define SSE_FUN_HEIGHT(blk_sz) int sse_fun = ff_cavs_sse_fun[blk_sz]; \
> +    int height  = ff_cavs_block_height[blk_sz];
> +
> +#define OFF_LUMA_SCAN(x,blk) x + h->luma_scan[blk]
> +
> +static int (*me)(AVSContext *h, enum cavs_block blk_sz, int blk_idx,
> +                 int ref, cavs_vector* pmv, cavs_vector* mv);
> +static int (*bi_me)(AVSContext *h, enum cavs_block blk_sz, int blk_idx,
> +                 int ref, cavs_vector* pmv, cavs_vector* mv);

Again, please avoid forward declarations.

> +    if (s->pict_type == FF_I_TYPE ) {
> +        put_header(s, CAVS_EDIT_CODE);
> +        put_header(s, PIC_I_START_CODE);
> +    }
> +    else
> +        put_header(s, PIC_PB_START_CODE);
> +
> +    put_bits(pb, 16, 0xdeaf); //TODO correct bbv_dwlay
> +    if (s->pict_type == FF_I_TYPE) {
> +        put_bits(pb, 1, 0); //time_code not present
> +        put_bits(pb, 1, 1); //marker bit
> +    }
> +    else
> +        put_bits(pb, 2, s->pict_type-1);

Put the else on the same line as the {, same in all other places

> +        if ((level > CAVS_MAX_LEVEL)||(run > CAVS_MAX_RUN)
> +                ||((level_code = enc->rlcode[run][level]) < 0)) {

alignment

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

indentation is off by one space

> +static void partition_copy_c(uint8_t *src, int src_stride, uint8_t *dst, int dst_stride, int w, int h)

Please break lines over 80 characters where easily possible.

> +static void ff_cavs_qpel_pred(AVSContext *h, uint8_t* dst, int dst_stride,
> +           enum cavs_block blk_sz, int blk_idx, int y, int x, int pos, int ref)

The indentation is off, the second line should be aligned on the same
column as the first function argument.  Please fix this everywhere else
as well.

> +    // predict mode
> +    if ((i_neighbour & (A_AVAIL|B_AVAIL)) == (A_AVAIL|B_AVAIL)) {

Note that American English is preferred, i.e. neighbOr instead of
neighbOUr.

> +        if (h->rdo[s->pict_type])
> +            diff = rdcost_intra_block(h, src, dst, s->linesize, block,
> +                             s->qscale, *coded_mode, *code_len, 1, mode);

Indentation, same as above; you sometimes misindent function calls the
same way you misindent function declarations.

> +        else
> +            diff = l_bitcost[i] * h->lambda_mode
> +                  + s->dsp.sse[1](NULL, src, dst, s->linesize, 8);

Similar, please fix the alignment, 'l' and '+' should be on the same
column.

> +#ifdef OUTPUT_REC
> +   cavs_frame_dump(h);
> +#endif
> +
> +#ifdef OUTPUT_REC
> +/**
> + * dump the reconstruction frame
> + * it is used only for debug
> + */
> +static void cavs_frame_dump(AVSContext *h)
> +{
> +}
> +#endif

I think if you put the #ifdef around the function body, you can get rid
of all the #ifdefs when calling the function.

> +#endif
> +    h->loop_filter_disable = 1;
> +    h->thread_context[0] = h;
> +    for(i=1; i<s->avctx->thread_count; i++){

for (, more below

Please put spaces around operators.

> +static void free_picture(MpegEncContext *s, Picture *pic) {
> +    int i;
> +
> +    if (pic->data[0] && pic->type != FF_BUFFER_TYPE_SHARED) {
> +        free_frame_buffer(s, pic);
> +    }

pointless {}

> +    for(i = 0; i < MAX_PICTURE_COUNT; i++) {
> +        free_picture(s, &s->picture[i]);
> +    }

again

> +AVCodec cavs_encoder =
> +{

Put the { on the same line as the declaration.

Diego