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

[Kostya]

On Wed, Mar 18, 2009 at 06:48:46PM +0100, Gwenole Beauchesne wrote:
> On Wed, 18 Mar 2009, Kostya wrote:
> 
> > 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
> 
> Hey, please tell me exactly what you want. I had a quick glance at vc1.c 
> and I see:
> 
> if ()
> {
> }
> else
> {
> }
> 
> or
> 
> if(){
> }else{
> }
> 
> or
> 
> if () {
> } else {
> }
> 
> OK for sure for the latter?

Both of the latter are OK (i.e. with or without spaces but on the same line).
 
> >> (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?
> 
> I do maintain a separate tree for that. What I meant was inclusion of this 
> shall not depend strictly on future integration of the other bits. 
> Considering how long it takes for VA API to get in, I bet this will take 
> at least as long for VDPAU...

Don't give up, the more experience you get, the faster thing will go.
 
> > Also that does not mean you have to produce such patch immediately, I just
> > want to ensure it won't be forgotten.
> 
> Things need to get in in-order. The initial deal was to factorize for VA 
> API. Then, the VDPAU bits can be merged. Please don't mix the tasks.

Please note I'm not against applying this patch, I'm merely against applying it
and forgetting about VDPAU. Since you will work on it, no problems.

> > [...]
> >> @@ -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.
> 
> What do you mean? The conditions are still in the same order: CONFIG_* 
> then hw caps.

I mean putting if(avctx->hwaccel){} before VDPAU chunk. If you don't have some
technical reasons to do so please insert it between VDPAU and C decoding.