[FFmpeg-devel] [PATCH] VC1 VDPAU: Mark missing reference frames as such.

[Reimar Döffinger]

On Tue, Oct 01, 2013 at 08:34:00PM +0200, Michael Niedermayer wrote:
> On Tue, Oct 01, 2013 at 08:04:34AM +0200, Reimar Döffinger wrote:
> > 
> > 
> > On 01.10.2013, at 03:15, Michael Niedermayer <michaelni at gmx.at> wrote:
> > 
> > > On Sat, Sep 28, 2013 at 11:13:35AM +0200, Reimar Döffinger wrote:
> > >> Currently the code passes some nonsense values as
> > >> references instead, causing corruption with NVidia's
> > >> and assertion failures with Mesa's implementation.
> > >> For non-corrupted input this mostly happens in
> > >> interlaced bitstreams, e.g.
> > >> http://samples.mplayerhq.hu/V-codecs/WMV9/interlaced/480i30__codec_WVC1__mode_2__framerate_29.970__type_2__preproc_17.wmv.
> > >> The != VDP_INVALID handle assert does not trigger
> > >> (and probably is quite nonsense) because the frames
> > >> are initialized to 0.
> > >> 
> > >> Signed-off-by: Reimar Döffinger <Reimar.Doeffinger at gmx.de>
> > >> ---
> > >> libavcodec/vdpau_vc1.c | 4 ++++
> > >> 1 file changed, 4 insertions(+)
> > >> 
> > >> diff --git a/libavcodec/vdpau_vc1.c b/libavcodec/vdpau_vc1.c
> > >> index 272b2d9..c6e3343 100644
> > >> --- a/libavcodec/vdpau_vc1.c
> > >> +++ b/libavcodec/vdpau_vc1.c
> > >> @@ -44,14 +44,18 @@ static int vdpau_vc1_start_frame(AVCodecContext *avctx,
> > >> 
> > >>     switch (s->pict_type) {
> > >>     case AV_PICTURE_TYPE_B:
> > >> +        if (s->next_picture_ptr) {
> > >>         ref = ff_vdpau_get_surface_id(&s->next_picture);
> > >>         assert(ref != VDP_INVALID_HANDLE);
> > >>         info->backward_reference = ref;
> > >> +        }
> > >>         /* fall-through */
> > >>     case AV_PICTURE_TYPE_P:
> > >> +        if (s->last_picture_ptr) {
> > > 
> > > vc1 contains quite a few NULL checks on srcy/uv, that is
> > > picture.f.data[0]
> > > dunno if that can be used here too
> > 
> > data[0] can validly be 0 as far as I can tell.
> > Also I check the _ptr ones because that's what the VC1 code I looked at did.
> 
> patch should be ok ten

I pushed it, though I admit looking at it once more I am not
100% what these _ptr ones being NULL means.
Though it is used at least like this:
    /* skip B-frames if we don't have reference frames */
    if (s->last_picture_ptr == NULL && (s->pict_type == AV_PICTURE_TYPE_B || s->droppable)) {
Either way the patch definitely does fix a real issue.

The old VDPAU codec code is not much help either, it has this:
        last = (struct vdpau_render_state *)s->last_picture.f.data[0];
        if (!last) // FIXME: Does this test make sense?
            last = render; // predict second field from the first

The difference being that the old API used data[0], but the new one
uses data[3]...
I wouldn't be surprised if there will be more bugs, and looking
at the code I'd be even more surprised if the other APIs like
VAAPI etc. weren't affect, too.
Which very much makes me wonder: is there anyone even using
any of the hwaccel stuff, or why are there neither bug reports
nor patches for it?
If MPlayer got reports within days of trying to use them...