[FFmpeg-devel] [PATCH][6/8] Add VA API accelerated VC-1 decoding (take 2)

[Kostya]

On Sat, Jan 31, 2009 at 12:34:24AM +0100, Gwenol? Beauchesne wrote:
> Hi,
> 
> Le 30 janv. 09 ? 19:26, Kostya a ?crit :
> 
> >> @@ -222,6 +222,7 @@ OBJS-$(CONFIG_ULTI_DECODER)            += ulti.o
> >> OBJS-$(CONFIG_VAAPI)                   += vaapivideo.o
> >> OBJS-$(CONFIG_VB_DECODER)              += vb.o
> >> OBJS-$(CONFIG_VC1_DECODER)             += vc1.o vc1data.o vc1dsp.o  
> >> msmpeg4data.o h263dec.o h263.o intrax8.o intrax8dsp.o  
> >> error_resilience.o mpegvideo.o
> >> +OBJS-$(CONFIG_VC1_VAAPI_DECODER)       += vc1.o vc1data.o vc1dsp.o  
> >> msmpeg4data.o h263dec.o h263.o intrax8.o intrax8dsp.o  
> >> error_resilience.o mpegvideo.o
> >> OBJS-$(CONFIG_VC1_VDPAU_DECODER)       += vdpauvideo.o vc1.o  
> >> vc1data.o vc1dsp.o msmpeg4data.o h263dec.o h263.o intrax8.o  
> >> intrax8dsp.o error_resilience.o mpegvideo.o
> >
> > Do you see any differences here to VDPAU decoder (hint: vaapi  
> > specific code)?
> 
> I chose to put a single OBJS-$(CONFIG_VAAPI) += vaapivideo.o for all  
> VA API accelerated codecs. I could drop this line and replicate  
> vaapivideo.o everywhere if you prefer. IOW, CONFIG_*_VAAPI_DECODER  
> implies CONFIG_VAAPI.

fine with me
 
> > [...]
> >> diff --git a/libavcodec/vaapivideo.c b/libavcodec/vaapivideo.c
> >> +/* Reconstruct bitstream PTYPE (7.1.1.4) */
> >> +static int vc1_get_PTYPE(VC1Context *v)
> >> +{
> >> +    MpegEncContext * const s = &v->s;
> >> +    if (v->profile < PROFILE_ADVANCED) {
> >> +        if (s->avctx->max_b_frames == 0) {
> >> +            switch (s->pict_type) {     // Table 33
> >> +            case FF_I_TYPE:     return 0;
> >> +            case FF_P_TYPE:     return 1;
> >> +            }
> >> +        }
> >> +        else {
> >> +            switch (s->pict_type) {     // Table 34
> >> +            case FF_P_TYPE:     return 0;
> >> +            case FF_I_TYPE:     return 1;
> >> +            case FF_B_TYPE:
> >> +            case FF_BI_TYPE:    return 2;
> >> +            }
> >> +        }
> >> +    }
> >> +    else {
> >> +        switch (s->pict_type) {         // Table 35
> >> +        case FF_I_TYPE:         return 0;
> >> +        case FF_P_TYPE:         return 1;
> >> +        case FF_B_TYPE:         return 2;
> >> +        case FF_BI_TYPE:        return 3;
> >> +        }
> >> +    }
> >> +    assert(0);
> >> +    return 0;
> >> +}
> >
> > Err, BI type = s->pict_type == FF_BI_TYPE && v->bi_type
> > s->pict_type = FF_BI_TYPE is set only during parsing frame header
> 
> Did you mean BI type = s->pict_type == FF_B_TYPE && v->bi_type ?

Oh, indeed.
 
> I will prepare a new patch on monday because this one requires visual  
> testing. BTW, if you know samples with such frames/conditions, this  
> will also help. As far as I could see, the ones I had (mostly from  
> mplayerhq samples) looked OK.

http://samples.mplayerhq.hu/V-codecs/WMV9/BI-frames/psychonauts.wmv

> I am also wondering if I shouldn't extend VA API to record bi_type  
> somewhere because VDPAU does need to distinguish B frames from BI  
> frames, it seems (vdpau.h). And SP/MP with max_b_frames != 0 use plain  
> 2 for both. So, it would be hard to reconstruct BI or B picture_type  
> in the VDPAU backend for VA API. Unless this doesn't matter in that  
> case, like I would guess? I admit I have not fully read the standard.  
> So, if you could enlight me on this one, this would be great. Thanks.

Since BI-frames are M$ hack (they should be treated as B-frames but
coded as I-frames, hence the name and troubles with them), declaring them
as FF_BI_TYPE would make libavcodec treat them as non-B-frames and I don't
want to amend conditions in random (i.e. not VC1-specific) places.
But since it's in the spec, decoders may expect BI-type if they decode
frames themselves (on bitstream level that does matter).

So I think it's better to pass correct frame type to decoder and make
it decide how to treat BI-frames.

> > in vc1.c - why do you always try to put testing VAAPI prior to VDPAU
> > in conditions? It got there first, you know.
> 
> Lame excuse: because some people are very picky about alphabetical  
> order, spaces, etc. I don't mind moving them down if you prefer. ;-)

I don't have any special preferences, so you may leave it as is. 

> Regards,
> Gwenole.