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

[Diego Biurrun]

On Sun, Jul 18, 2010 at 02:47:46PM +0800, jianwen chen wrote:
> On Sat, Jul 17, 2010 at 6:55 PM, Diego Biurrun <diego at biurrun.de> wrote:
> 
> > > --- libavcodec/libxavs.c      (revision 0)
> > > +++ libavcodec/libxavs.c      (revision 0)
> > +#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.
> 
>     OK,  however, I also refer to the libx264.c with the same order for the
> including headers.

That file does it wrong then.

> I can change the order as following
> 
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <math.h>
> 
> > +#include "avcodec.h"
> > +#include <xavs.h>

Better, but only slightly, you still place xavs.h after avcodec.h.

> --- libavcodec/libxavs.c	(revision 0)
> +++ libavcodec/libxavs.c	(revision 0)
> @@ -0,0 +1,355 @@
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <math.h>
> +
> +#include "avcodec.h"
> +#include <xavs.h>

Place all system headers before local headers.

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

more readable:

  #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 */

> +    for (i = 0; i < nnal; i++) {
> +        /* Don't put the SEI in extradata. */
> +        if (skip_sei && nals[i].i_type == NAL_SEI) {
> +            x4->sei = av_malloc( 5 + nals[i].i_payload * 4 / 3 );
> +            if(xavs_nal_encode(x4->sei, &x4->sei_size, 1, nals + i) < 0)

if (

> +    if( !bufsize && !frame && !(x4->end_of_stream) ){

Inconsistent formatting; you again miss the space after the if and put
spaces inside the parentheses.

> +    /*Amanda thinks there is no IDR frame in AVS 1.0.Sequence header is used as a flag*/

long line

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

Why is this commented out but still present?

> +    /*AMANDA TAG: AVS doesn't allow B picture as reference and the max allowed number of B is 2*/

see above

And what is AMANDA?

> +    /*AMANDA TAG: Amanda thinks this is only used for counting the fps*/

Here and in other places: space after /* and before */ would aid
readability.

> +    if (avctx->me_method == ME_EPZS){
> +        x4->params.analyse.i_me_method = XAVS_ME_DIA;
> +    }
> +    else if (avctx->me_method == ME_HEX){
> +        x4->params.analyse.i_me_method = XAVS_ME_HEX;
> +    }
> +    else if (avctx->me_method == ME_UMH){
> +        x4->params.analyse.i_me_method = XAVS_ME_UMH;
> +    }
> +    else if (avctx->me_method == ME_FULL){
> +        x4->params.analyse.i_me_method = XAVS_ME_ESA;
> +    }
> +    else if (avctx->me_method == ME_TESA){
> +        x4->params.analyse.i_me_method = XAVS_ME_TESA;
> +    }
> +    else{
> +        x4->params.analyse.i_me_method = XAVS_ME_HEX;
> +    }

This could be a switch IMO.

Diego