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

[jianwen chen]

On Sun, Jul 18, 2010 at 4:04 PM, Diego Biurrun <diego at biurrun.de> wrote:

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

Ok. I change to this order,
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <math.h>
+#include <stdint.h>
+#include <xavs.h>
+#include "avcodec.h"


>
> > 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 */
>
  done;


> > +    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.
>
>  Yes. I modified.


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


> > +    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?
>
>
Remove this commented sentence. I list this sentence here to make a mark for
future improvement.
 We can remove it now.


> > +    /*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?
>
>

  OK, I delete the AMANDA from the file. AMANDA is a pretty girl who has
worked on this code before.


> > +    /*AMANDA TAG: Amanda thinks this is only used for counting the fps*/
>
> Here and in other places: space after /* and before */ would aid
> readability.
>
> Good point, I have checked all the /* */ with your comments.


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

Ok, I change this to switch ..case...

The updated patch is attached for checking.  Thanks for your help.


>
> 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-update4.patch
Type: application/octet-stream
Size: 16448 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100718/e579a2a8/attachment.obj>