[FFmpeg-devel] [PATCH] adding xavs encoding support

[Diego Biurrun]

Please don't top-post.

On Sat, Jul 17, 2010 at 01:11:46PM +0800, jianwen chen wrote:
> OK.  I have modified the code according to the comments.
> 
> Please check the attached patch .

This is missing a changelog and documentation update.
You also have tabs and trailing whitespace in there.

Please refer to our patch submission checklist.

> --- libavcodec/libxavs.c	(revision 0)
> +++ libavcodec/libxavs.c	(revision 0)
> @@ -0,0 +1,335 @@
> +/*
> + * AVS encoding using the xavs library
> + * Copyright (C) 2010 Amanda, Y.N. Wu <amanda11192003 at gmail.com>
> + *
> + * This file will be part of FFmpeg.

This is not our standard header.  Legal stuff is not the place to be
funny I'm afraid.

> +#include "avcodec.h"
> +#include <xavs.h>
> +#include <math.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#define END_OF_STREAM 0x001

Place system headers before local headers.

> +#define XAVS_PART_I8X8 0x002  /* Analyze i8x8 (requires 8x8 transform) */
> +#define XAVS_PART_P8X8 0x010  /* Analyze p16x8, p8x16 and p8x8 */
> +#define XAVS_PART_B8X8 0x100  /* Analyze b16x8, b*/
> +typedef struct XavsContext {
> +} XavsContext;

An empty line could help readability.

> +static int encode_nals(AVCodecContext *ctx, uint8_t *buf, int size, xavs_nal_t *nals, int nnal, int skip_sei)

Break this long line.

> +{
> +    XavsContext *x4 = ctx->priv_data;
> +    uint8_t *p = buf;
> +    int i, s;
> +//    int curr_frame = ctx->i_frames;

cruft

> +            if(xavs_nal_encode(x4->sei, &x4->sei_size, 1, nals + i) < 0)

if (

same below

> +    if (frame) {
> +        for (i = 0; i < 3; i++) {
> +            x4->pic.img.plane[i] = frame->data[i];
> +            x4->pic.img.i_stride[i] = frame->linesize[i];

Align the = for extra readability.

> +    if (xavs_encoder_encode(x4->enc, &nal, &nnal, frame? &x4->pic: NULL, &pic_out) < 0)

long line

> +    if(bufsize==0&&frame==NULL&&x4->end_of_stream==0)
> +    {

inconsistent { placement
put spaces around those operators

> +        buf[bufsize] = 0x0;
> +        buf[bufsize+1] = 0x0;
> +        buf[bufsize+2] = 0x01;
> +        buf[bufsize+3] = 0xb1;

Align the = for extra readability.

> +        bufsize+=4;
> +        x4->end_of_stream=END_OF_STREAM;

put spaces around those operators

> +static av_cold int Xavs_close(AVCodecContext *avctx)

Capitalized function names are ugly and not commonly used in FFmpeg
- get rid of them.

> +    x4->params.pf_log               = XAVS_log;
> +    x4->params.p_log_private        = avctx;
> +//    x4->params.i_frame_total        = max_frames[CODEC_TYPE_VIDEO];

cruft

> +   // x4->params.b_deblocking_filter         = avctx->flags & CODEC_FLAG_LOOP_FILTER;

more

> +    x4->params.i_width              = avctx->width;
> +    x4->params.i_height             = avctx->height;
> +    x4->params.vui.i_sar_width      = avctx->sample_aspect_ratio.num;
> +    x4->params.vui.i_sar_height     = avctx->sample_aspect_ratio.den;
> +    /*AMANDA TAG: Amanda thinks this is only used for counting the fps*/
> +    x4->params.i_fps_num  = avctx->time_base.den;
> +    x4->params.i_fps_den  = avctx->time_base.num;
> +    x4->params.analyse.inter    = XAVS_ANALYSE_I8x8 |XAVS_ANALYSE_PSUB16x16| XAVS_ANALYSE_BSUB16x16;

Align the = for better readability.

Diego