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

[Stefan Gehrer]

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. Welcome to your comments.
> A known bug is that now this patch not support the mmx optimization. Only C
> version is ok now. I can't this bug now.
> Perhaps Stefan Gehrer may help me.

Can you say more about what the problem is with the MMX stuff? I am
currently concerned that the inter prediction may overflow the 16bit
width in intermediate values, but I have to recheck.
You also mentioned that something has changed in the DCT, but in your
patch only the DCT is added but there are no changes in the IDCT.
Is there something that still needs to be done there (which would
also require a change in the MMX code)?

Some notes about what I saw in the patch:

 > Index: libavcodec/mpegvideo_enc.c
 > ===================================================================
 > --- libavcodec/mpegvideo_enc.c	(revision 20541)
 > +++ libavcodec/mpegvideo_enc.c	(working copy)
 > @@ -366,7 +366,7 @@
 >          return -1;
 >      }
 >
 > -    if(s->quarter_sample && s->codec_id != CODEC_ID_MPEG4){
 > +    if(s->quarter_sample && s->codec_id != CODEC_ID_MPEG4 &&
 > s->codec_id != CODEC_ID_CAVS){
 >          av_log(avctx, AV_LOG_ERROR, "qpel not supported by codec\n");
 >          return -1;
 >      }

I don't think this is necessary. Turning qpel off does not really make
sense. We should ignore this option just like for H.264 encoding


 > @@ -376,7 +376,7 @@
 >          return -1;
 >      }
 >
 > -    if(s->max_b_frames && s->codec_id != CODEC_ID_MPEG4 &&
 > s->codec_id != CODEC_ID_MPEG1VIDEO && s->codec_id !=
 > CODEC_ID_MPEG2VIDEO){
 > +    if(s->max_b_frames && s->codec_id != CODEC_ID_MPEG4 &&
 > s->codec_id != CODEC_ID_MPEG1VIDEO && s->codec_id !=
 > CODEC_ID_MPEG2VIDEO && s->codec_id != CODEC_ID_CAVS){
 >          av_log(avctx, AV_LOG_ERROR, "b frames not supported by
 > codec\n");
 >          return -1;
 >      }

This does make sense, the line just gets awfully long :)

 > @@ -503,6 +503,7 @@
 >
 >      switch(avctx->codec->id) {
 >      case CODEC_ID_MPEG1VIDEO:
 > +    case CODEC_ID_CAVS:
 >          s->out_format = FMT_MPEG1;
 >          s->low_delay= !!(s->flags & CODEC_FLAG_LOW_DELAY);
 >          avctx->delay= s->low_delay ? 0 : (s->max_b_frames + 1);
 > @@ -771,7 +772,7 @@
 >  }
 >
 >
 > -static int load_input_picture(MpegEncContext *s, AVFrame *pic_arg){
 > +int load_input_picture(MpegEncContext *s, AVFrame *pic_arg){
 >      AVFrame *pic=NULL;
 >      int64_t pts;
 >      int i;
 > @@ -1019,7 +1020,7 @@
 >      return best_b_count;
 >  }
 >
 > -static void select_input_picture(MpegEncContext *s){
 > +void select_input_picture(MpegEncContext *s){
 >      int i;
 >
 >      for(i=1; i<MAX_PICTURE_COUNT; i++)

If you want to reuse a function in another file, you should give
it a ff_ prefix (e.g. ff_load_input_picture) and put its
prototype into an appropriate header (in this case probably
mpegvideo.h).

Best regards
Stefan