[FFmpeg-cvslog] vsrc_buffer: remove dependency on AVFrame

[Stefano Sabatini]

On date Friday 2011-06-03 00:39:11 +0200, Etienne Buira wrote:
> On Thu, Jun 02, 2011 at 04:00:57PM +0200, Stefano Sabatini wrote:
[...]
> > I confirm there is a leak.
> > 
> > The leak is occurring in av_vsrc_buffer_add_video_buffer_ref(), the
> > buffer is already filled but the new picref is added to the cache
> > overwriting the old picref (which is never released).
> > 
> > Options:
> > 
> > 1) make av_vsrc_buffer_add_video_buffer_ref() abort if the buffer is
> >    already filled.
> >    This has a problem though, since we *want* to store the last added
> >    frame.
> > 
> > 2) Extend the API of vsrc_buffer. In particular we may add this:
> > 
> >    // it is already filled, reset it
> >    if (avfilter_poll_frame(vsrc_buffer->inputs[0] > 0)
> >       av_vsrc_buffer_reset(vsrc_buffer); // clean the buffer
> >    av_vsrc_buffer_add_frame(vsrc_buffer, &picture);
> > 
> >    Alternatively we may add a mode for *always* overwriting the last
> >    frame (during initialization or with a flag in
> >    av_vsrc_buffer_add_frame()), like in:
> > 
> >    av_vsrc_buffer_add_frame(vsrc_buffer, &picture, AV_VSRC_BUF_FLAG_OVERWRITE);
> > 
> > Comments are welcome.
> 
> Hi Stefano.
> 

> I don't know how data is flowing in avfilter, so please forgive me if I
> tell stupid things.

No don't worry, and don't forget that an "outsider" can often deliver
a precious unbiased point of view, having no previous knowledge of the
codebase is actually a good thing since she can spot things which
can't be seen by the original coder/designer since he's too much
plugged into his design and doing too many (conscious and unconscious)
assumptions when reading his own code.

> Wasn't 2, the previous way to handle added frames? If so, is there a
> reason that it could not be the way again?
> 
> The attached patch works for me.

> diff --git a/libavfilter/vsrc_buffer.c b/libavfilter/vsrc_buffer.c
> index d1e6ffd..095e368 100644
> --- a/libavfilter/vsrc_buffer.c
> +++ b/libavfilter/vsrc_buffer.c
> @@ -48,6 +48,7 @@ int av_vsrc_buffer_add_video_buffer_ref(AVFilterContext *buffer_filter, AVFilter
>                 "Buffering several frames is not supported. "
>                 "Please consume all available frames before adding a new one.\n"
>              );
> +        avfilter_unref_buffer(c->picref);
>          //return -1;

That's possible but there are cases when you may *want* to keep the
previous frame rather than simply discard it, and vsrc_buffer is
more-or-less public API so we shouldn't add too many constraints to
it.

That's why I lean towards solution 2, which seems the more flexible (a
reset function may be useful but it isn't related to this patch), I'll
send a patch for implementing that approach if I see no more comments.
-- 
FFmpeg = Fierce and Free Mysterious Pacific Elfic Gnome