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.doeffin...@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
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
