[FFmpeg-devel] [PATCH]NVIDIA VDPAU patch for h264

[Reimar Döffinger]

On Sun, Nov 30, 2008 at 10:03:39PM +0100, Carl Eugen Hoyos wrote:
> +extern int ff_VDPAU_h264_add_data_chunk(H264Context *h, const uint8_t *buf, int buf_size);
> +extern int ff_VDPAU_h264_picture_complete(H264Context *h);
> +

extern is useless should not be used here IMO.

> @@ -101,6 +104,16 @@
>      {0,2,0,2,7,10,7,10}
>  };
>  
> +static const enum PixelFormat pixfmt_vdpau_h264_baseline_420[] = {
> +                                           PIX_FMT_VDPAU_H264_BASELINE,
> +                                           PIX_FMT_NONE};
> +static const enum PixelFormat pixfmt_vdpau_h264_main_420[] = {
> +                                           PIX_FMT_VDPAU_H264_MAIN,
> +                                           PIX_FMT_NONE};
> +static const enum PixelFormat pixfmt_vdpau_h264_high_420[] = {
> +                                           PIX_FMT_VDPAU_H264_HIGH,
> +                                           PIX_FMT_NONE};

I agree with Michael that this seems very ugly and "painful". I could
imagine that it makes fallback to software easier for MPlayer, but that
special case is not enough to justify it.
Also from what I heard generally, the profile information in the file
is generally useless anyway, at least it can not be relied on, making
this even more questionable.

> @@ -2142,10 +2155,8 @@
>      s->quarter_sample = 1;
>      s->low_delay= 1;
>  
> -    if(avctx->codec_id == CODEC_ID_SVQ3)
> -        avctx->pix_fmt= PIX_FMT_YUVJ420P;
> -    else
> -        avctx->pix_fmt= PIX_FMT_YUV420P;
> +    // Set in decode_postinit() once initial parsing is complete
> +    avctx->pix_fmt = PIX_FMT_NONE;

What is the point of moving the SVQ3 case, I doubt the hardware
acceleration supports that?
Also without the profile stuff the pix_fmt could still be set here...

> @@ -7257,6 +7301,9 @@
>      H264Context *hx;
>      int i;
>  
> +    if(avctx->vdpau_acceleration) {
> +        return;
> +    } else

Useless {} and else

> @@ -7384,8 +7431,26 @@
>                 && (avctx->skip_frame < AVDISCARD_NONREF || hx->nal_ref_idc)
>                 && (avctx->skip_frame < AVDISCARD_BIDIR  || hx->slice_type_nos!=FF_B_TYPE)
>                 && (avctx->skip_frame < AVDISCARD_NONKEY || hx->slice_type_nos==FF_I_TYPE)
> -               && avctx->skip_frame < AVDISCARD_ALL)
> +               && avctx->skip_frame < AVDISCARD_ALL) {
> +#ifdef CONFIG_VDPAU
> +                if (avctx->vdpau_acceleration) {

And adding a ENABLE_VDPAU in MPlayer is a complete non-issue.
Given that very few people (at least developers) will compile FFmpeg
with this enabled, it is _very_ advisable to make the compiler at least
syntax-check as much of it as possible.


> @@ -7588,6 +7653,12 @@
>          h->prev_frame_num_offset= h->frame_num_offset;
>          h->prev_frame_num= h->frame_num;
>  
> +#ifdef CONFIG_VDPAU
> +        if (avctx->vdpau_acceleration) {
> +            ff_VDPAU_h264_picture_complete(h);
> +        }
> +#endif

Useless {}

> @@ -7600,6 +7671,9 @@
>           * past end by one (callers fault) and resync_mb_y != 0
>           * causes problems for the first MB line, too.
>           */
> +#ifdef CONFIG_VDPAU
> +        if (!avctx->vdpau_acceleration)
> +#endif
>          if (!FIELD_PICTURE)
>              ff_er_frame_end(s);

IMO not worth the #ifdef, should be just
>          if (!avctx->vdpau_acceleration && !FIELD_PICTURE)

> @@ -7973,4 +8047,35 @@
>      .long_name = NULL_IF_CONFIG_SMALL("H.264 / AVC / MPEG-4 AVC / MPEG-4 part 10"),
>  };
>  
> +#ifdef CONFIG_VDPAU
> +static av_cold int h264_vdpau_decode_init(AVCodecContext *avctx){
> +    if( avctx->thread_count > 1)
> +        return -1;
> +    if( !(avctx->slice_flags & SLICE_FLAG_CODED_ORDER) )
> +        return -1;
> +    if( !(avctx->slice_flags & SLICE_FLAG_ALLOW_FIELD) ){
> +        dprintf(avctx, "h264.c: VDPAU decoder does not set SLICE_FLAG_ALLOW_FIELD\n");
> +    }

IMO these each need a comment why and possibly why return -1 is the best
solution (e.g. would setting thread_count to 1 instead be acceptable/better?).

> +    render->info.h264.field_pic_flag                         = (s->picture_structure != PICT_FRAME) ? 1 : 0;
> +    render->info.h264.bottom_field_flag                      = (s->picture_structure == PICT_BOTTOM_FIELD) ? 1 : 0;

Uh, like useless code much? You could add ten more "? 1 : 0" and the
result still would not change a bit...

> Index: libavcodec/mpegvideo.c
> ===================================================================
> --- libavcodec/mpegvideo.c	(Revision 15958)
> +++ libavcodec/mpegvideo.c	(Arbeitskopie)
> @@ -954,6 +954,9 @@
>          XVMC_field_end(s);
>      }else
>  #endif
> +#ifdef CONFIG_VDPAU
> +    if(!s->avctx->vdpau_acceleration)
> +#endif
>      if(s->unrestricted_mv && s->current_picture.reference && !s->intra_only && !(s->flags&CODEC_FLAG_EMU_EDGE)) {
>              s->dsp.draw_edges(s->current_picture.data[0], s->linesize  , s->h_edge_pos   , s->v_edge_pos   , EDGE_WIDTH  );
>              s->dsp.draw_edges(s->current_picture.data[1], s->uvlinesize, s->h_edge_pos>>1, s->v_edge_pos>>1, EDGE_WIDTH/2);

IMO should be done without the #ifdef, too.

Greetings,
Reimar D?ffinger