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

[jianwen chen]

On Sat, Jul 17, 2010 at 6:55 PM, Diego Biurrun <diego at biurrun.de> wrote:

> 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.
>
>
    I have used the patchcheck in the tools directory . Thanks.


> > --- 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.
>
>
    I use the same header with the same header format as the files
under ffmpeg such as libavcodec/libx264.c, libfaac.c.

> +#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.
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>


> > +#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.


Yes, you are right.

>
>


> > +static int encode_nals(AVCodecContext *ctx, uint8_t *buf, int size,
> xavs_nal_t *nals, int nnal, int skip_sei)
>
> Break this long line.
>

   Done.


>
> > +{
> > +    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
>

    Done;

>
> > +    if(bufsize==0&&frame==NULL&&x4->end_of_stream==0)
> > +    {
>
> inconsistent { placement
> put spaces around those operators
>

   Done;

>
> > +        buf[bufsize] = 0x0;
> > +        buf[bufsize+1] = 0x0;
> > +        buf[bufsize+2] = 0x01;
> > +        buf[bufsize+3] = 0xb1;
>
> Align the = for extra readability.
>
>      Done.

> > +        bufsize+=4;
> > +        x4->end_of_stream=END_OF_STREAM;
>
> put spaces around those operators
>
>      Done.


> > +static av_cold int Xavs_close(AVCodecContext *avctx)
>
> Capitalized function names are ugly and not commonly used in FFmpeg
> - get rid of them.
>
>
 Ok, we use the  XAVS_close  XAVS_init  and  XAVS_frame.


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

    Done;

>
> > +   // 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.
>

    Done.

   A new patch is attached for checking.

   And I have finished the regression tests for all the patches.

   Thanks.

   Jianwen


> Diego
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch-ffmpeg-svn-r24248-for-libxavs-update3.patch
Type: application/octet-stream
Size: 16534 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100718/7dfc9e5e/attachment.obj>