[FFmpeg-devel] [PATCH][7/8] Add VA API accelerated H.264 decoding (take 3)

[Stephen Warren]

Gwenole Beauchesne wrote:
> 
> On Mon, 2 Feb 2009, Gwenole Beauchesne wrote:
> 
> > I have finally fixed my ReferenceFrames[] list construction problem, see
> > "How to access the DPS" thread. More bitstreams are now working better
> > (no jittering). However, my VA API to VDPAU bridge now broke because the
> > former apparently needs DPB after execute_ref_pic_marking() with MMCO is
> > called whereas the latter needs it before.
> 
> This change was wrong, here is a new patch. BTW, new libva and vdpau-video
> (VA API to VDPAU bridge) are now available.
> <http://www.splitted-desktop.com/~gbeauchesne/>

I do have a couple comments/questions on this:

> @@ -7432,6 +7442,14 @@ static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size){
>              if((err = decode_slice_header(hx, h)))
>                 break;
>  
> +            if(err == 0 && IS_VAAPI_ENABLED(s)) {
> +                /* XXX: VASliceParameterBufferH264::slice_data_bit_offset
> +                   is computed relative to the GetBitContext initialized
> +                   from ptr returned by decode_nal(). So, this probably
> +                   won't work if the latter produced escaped bytes */
> +                ff_vaapi_h264_decode_slice(hx, &buf[buf_index-consumed], consumed);
> +            }
> +
>              s->current_picture_ptr->key_frame|= (hx->nal_unit_type == NAL_IDR_SLICE);
>              if(hx->redundant_pic_count==0 && hx->s.hurry_up < 5
>                 && (avctx->skip_frame < AVDISCARD_NONREF || hx->nal_ref_idc)

I note you put the VA API call outside the big if block immediately below
that patch, where the VDPAU call is. What's the reasoning behind that? I
assume that the placement of the VDPAU call allows decode skipping when
behind, in order to maintain AV sync.

> @@ -7453,6 +7471,10 @@ static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size){
>              hx->s.data_partitioning = 1;
>  
>              err = decode_slice_header(hx, h);
> +
> +            /* VA API only supports Baseline, Main and High profiles */
> +            if (err == 0 && IS_VAAPI_ENABLED(s))
> +                av_log(h->s.avctx, AV_LOG_ERROR, "Invalid nal_unit_type for VA API acceleration\n");
>              break;
>          case NAL_DPB:
>              init_get_bits(&hx->intra_gb, ptr, bit_length);

Does it make sense to duplicate this check for DPB and DPC? I suppose there
shouldn't be a DPB/DPC without a DPA first, but suppose it gets dropped due
to bitstream/parse errors?

We should enable this check for VDPAU too, although that's not a job for
your patch.

> @@ -7502,7 +7524,11 @@ static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size){
>              av_log(avctx, AV_LOG_DEBUG, "Unknown NAL code: %d (%d bits)\n", h->nal_unit_type, bit_length);
>          }
>  
> -        if(context_count == h->max_contexts) {
> +        if(IS_VAAPI_ENABLED(s)) {
> +            /* Make sure we never call execute_decode_slices() */
> +            context_count = 0;
> +        }
> +        else if(context_count == h->max_contexts) {
>              execute_decode_slices(h, context_count);
>              context_count = 0;
>          }

The VDPAU code handles this by making execute_decode_slices return immediately
if VDPAU is enabled. VDPAU and VA API should really be consistent. I think we
copied the VDPAU mechanism from existing MPEG-2 XvMC integration.

Which way do ffmpeg maintainters think is best? Should VDPAU adopt the
Mechanism in this patch, or should VA API adopt VDPAU's current mechanism?

Also, I note that VA API has a ff_vaapi_h264_frame_start, but VDPAU has no
equivalent, since it doesn't need to do the operations VA API does there, and
it grabs all the H.264 data it needs in ff_vdpau_h264_picture_complete. Again,
we should be consistent, to avoid issues where any of that data gets modified
in between those two locations, and thus causes differences between VA API and
VDPAU. I guess VDPAU should grow a frame_start() function to fix this.

Note however, that there's a reason that *some* of the information VDPAU uses
is picked up during frame_end time not frame_start time. My memory is poor,
but I *think* the call to execute_ref_pic_marking() right before the existing
ff_vdpau_h264_picture_complete() modifies one of the data fields VDPAU uses,
and I *think* that was h->frame_num (everything else comes from SPS/PPS
anyway, so shouldn't be modified after parsing those). It looks like VA API
grabs this during ff_vaapi_h264_frame_start() rather than 
ff_vaapi_h264_frame_end(), which I suspect is a bug.

-- 
nvpublic