[FFmpeg-devel] [FFmpeg-cvslog] vp8: always update next_framep[] before returning from decode_frame().

[Reimar Döffinger]

On Thu, Feb 09, 2012 at 11:57:28AM -0800, Ronald S. Bultje wrote:
> On Thu, Feb 9, 2012 at 11:49 AM, Reimar Döffinger
> <Reimar.Doeffinger at gmx.de> wrote:
> > I find that attribution a bit strange when I sent a patch for this
> > issue already on Dec 11.
> 
> Not to me. The issue was disclosed to me by these people, therefore
> they get attribution for disclosing the issue to me.

Sorry, I had assumed you were still reading over at least one of
the FFmpeg lists.

> > That patch also fixed a few other issues, for example that failure
> > to allocate a frame would not update the reference frames according
> > to header data.
> > I am not completely sure but I also think that your patch might
> > still leave a frame that was freed in the reference frame list.
> 
> I don't think that's possible. A free()'ed frame is either by
> definition not referenced (that's the condition under which it is
> free()'ed in vp8_decode_frame()), or it is allocated-but-not-decoded.
> That second, however, is not possible, since there is no return or
> goto err statement after vp8_alloc_frame() in vp8_decode_frame(),
> except for the return at the end of the function.

The path I see is:
curframe = s->framep[VP56_FRAME_CURRENT] = &s->frames[i];
[...]
    if (curframe->data[0])
            vp8_release_frame(s, curframe, 1, 0);
[...]
    if (!s->keyframe && (!s->framep[VP56_FRAME_PREVIOUS] ||
                         !s->framep[VP56_FRAME_GOLDEN] ||
                         !s->framep[VP56_FRAME_GOLDEN2])) {
        av_log(avctx, AV_LOG_WARNING, "Discarding interframe without a prior keyframe!\n");
        ret = AVERROR_INVALIDDATA;
        goto err;

[decode done for this frame]

    prev_frame = s->framep[VP56_FRAME_CURRENT];
[prev_frame not NULL, but freed]
        if (prev_frame && s->segmentation.enabled && !s->segmentation.update_map)
            ff_thread_await_progress(prev_frame, mb_y, 0);

In addition, framep not being updated on allocation error I think leads to
the code thinking that all reference frames are available even though
it is only the reference frames for the _previous_ frame are available.

> > Attached is the diff between the current libav code and the code
> > as I patched it.
> 
> Does it fix a particular issue? If so, please send me a link to the
> bug report and provide a sample that crashes.
> 
> If no such bugs and/or samples exist, I will assume the patch is not necessary.

I'm sorry, but I find it very much insulting that you
find my comments not even worth considering unless
I first show proof of a bug.
The place where I saw the _need_ for the patch is that I think
the current code is missing some kind of "invariant" that makes
it easy to verify that no "crap" ends up in the reference lists
in corner-cases.
My patch attempted to fix that by making sure (assuming I did not
mess up) that only two paths exist:
1) abort right away, no change
2) update exactly in the same way as for a successfully decoded frame,
   but on error explicitly set to NULL whereever the current frame
   would have ended up.

The current code as far as I can tell still allows for corner-cases
like VP56_FRAME_CURRENT being updated but nothing else.
If I am wrong feel free to flame me for wasting your time, but I'd
appreciate not being just disregarded because I have no sample
to prove anything.

Reimar