[FFmpeg-devel] [FFmpeg-cvslog] vp8: always update next_framep before returning from decode_frame().
Reimar.Doeffinger at gmx.de
Thu Feb 9 22:15:22 CET 2012
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];
vp8_release_frame(s, curframe, 1, 0);
if (!s->keyframe && (!s->framep[VP56_FRAME_PREVIOUS] ||
av_log(avctx, AV_LOG_WARNING, "Discarding interframe without a prior keyframe!\n");
ret = AVERROR_INVALIDDATA;
[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
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.
More information about the ffmpeg-devel