[FFmpeg-devel] [PATCH] vc1.c: add support for HWAccel (take 2)

[Kostya]

On Wed, Mar 18, 2009 at 06:06:31PM +0100, Gwenole Beauchesne wrote:
> Hi,
> 
> On Tue, 10 Mar 2009, Kostya wrote:
> 
> >>@@ -4277,6 +4279,15 @@ static int vc1_decode_frame(AVCodecContext *avctx,
> >>     s->me.qpel_put= s->dsp.put_qpel_pixels_tab;
> >>     s->me.qpel_avg= s->dsp.avg_qpel_pixels_tab;
> >>
> >>+    if (avctx->hwaccel) {
> >>+        if (avctx->hwaccel->start_frame(avctx, buf, buf_size) < 0)
> >>+            return -1;
> >>+        if (avctx->hwaccel->decode_slice(avctx, buf_start, (buf + 
> >>buf_size) - buf_start) < 0)
> >>+            return -1;
> >>+        if (avctx->hwaccel->end_frame(avctx) < 0)
> >>+            return -1;
> >>+    }
> >>+    else
> >
> ><don't forget proper indent here>
> 
> Well, that was an "optimization" to get a smaller patch (only '+') and 
> reduce the next one when removing VDPAU with only '-'s instead. ;-) 
> Anyway, reindented correctly as you can see in the attachment.

Actually I don't like
    }
    else
instead of
    } else

And you should not worry about that kind of optimisation.

> >>     if ((CONFIG_VC1_VDPAU_DECODER || CONFIG_WMV3_VDPAU_DECODER)
> >>         &&s->avctx->codec->capabilities&CODEC_CAP_HWACCEL_VDPAU)
> >>         ff_vdpau_vc1_decode_picture(s, buf_start, (buf + buf_size) - 
> >>         buf_start);
> >
> >Those two chunks make me a bit uneasy and not because of your changes.
> >
> >Patch OK in the case you swear to convert VDPAU to hwaccel architecture
> >(I don't like to have too many special cases).
> 
> I am sorry but I can't swear anything because

> (i) AVHWAccel is not really 
> a "special" case, it's the new default HW acceleration framework,

Well, I'm plain C codec developer, so hardware acceleration added in one form
or another is a special case to me (since the call does not proceed to actual
frame decoding routine).

> (ii) you 
> can't swear either that all VA API bits can get in by the end of the week,

I don't care about them but since you are the man behind AVHWAccel, why not
convert existing instances to AVHWAccel?

Example:
old vc1.c:
  decode_frame(){
   parse();
   decode();
  }

after VDPAU:
  decode_frame(){
   parse();
   if(codec_cap & VDPAU)
    decode_vdpau();
   else
    decode();

with this patch:
  decode_frame(){
   parse();
   if(codec_cap & VDPAU)
    decode_vdpau();
   else if(hwaccel)
    hwaccel->decode();
   else
    decode();
  }

what I like to see:
  decode_frame(){
   parse();
   if(hwaccel)
    hwaccel->decode();
   else
    decode();
  }

> (iii) this is independent from vc1.c,
> (iv) what if I did not actually have 
> the HW to work on VDPAU support in the first place? Blind coding? Well, 
> this process can work with patient enough testers, according to reports I 
> got for my initial VAAPI-to-VDPAU bridge code for VC-1. ;-)

Yes, we are accustomed to that here. Also since the code is already here, you
just have to reorganize it a bit to fit into HWAccel.

Also that does not mean you have to produce such patch immediately, I just
want to ensure it won't be forgotten.

[...]
> @@ -4277,8 +4279,16 @@ static int vc1_decode_frame(AVCodecContext *avctx,
>      s->me.qpel_put= s->dsp.put_qpel_pixels_tab;
>      s->me.qpel_avg= s->dsp.avg_qpel_pixels_tab;
>  
> -    if ((CONFIG_VC1_VDPAU_DECODER || CONFIG_WMV3_VDPAU_DECODER)
> -        &&s->avctx->codec->capabilities&CODEC_CAP_HWACCEL_VDPAU)
> +    if (avctx->hwaccel) {
> +        if (avctx->hwaccel->start_frame(avctx, buf, buf_size) < 0)
> +            return -1;
> +        if (avctx->hwaccel->decode_slice(avctx, buf_start, (buf + buf_size) - buf_start) < 0)
> +            return -1;
> +        if (avctx->hwaccel->end_frame(avctx) < 0)
> +            return -1;
> +    }
> +    else if ((CONFIG_VC1_VDPAU_DECODER || CONFIG_WMV3_VDPAU_DECODER)
> +             &&s->avctx->codec->capabilities&CODEC_CAP_HWACCEL_VDPAU)
>          ff_vdpau_vc1_decode_picture(s, buf_start, (buf + buf_size) - buf_start);
>      else {
>          ff_er_frame_start(s);

No need to swap conditions here.