[FFmpeg-cvslog] vsrc_buffer: remove dependency on AVFrame

[Etienne Buira]

On Thu, Jun 02, 2011 at 04:00:57PM +0200, Stefano Sabatini wrote:
> On date Thursday 2011-06-02 01:27:01 +0200, Etienne Buira wrote:
> > On Thu, Jun 02, 2011 at 01:07:18AM +0200, Stefano Sabatini wrote:
> > > On date Wednesday 2011-06-01 19:53:48 +0200, Etienne Buira wrote:
> > > > On Thu, May 19, 2011 at 11:33:08PM +0200, Stefano Sabatini wrote:
> > > > > ffmpeg | branch: master | Stefano Sabatini <stefano.sabatini-lala at poste.it> | Sat May  7 21:35:08 2011 +0200| [6070b7e1c520e9ca389403bae20a2ad04c7d54c7] | committer: Stefano Sabatini
> > > > > 
> > > > > vsrc_buffer: remove dependency on AVFrame
> > > > > 
> > > > > Rename av_vsrc_buffer_add_frame to
> > > > > av_vsrc_buffer_add_video_buffer_ref(), and change its inteface to make
> > > > > it accept in input an AVFilterBufferRef rather than an AVFrame.
> > > > > 
> > > > > This way the interface can be used without requiring the
> > > > > inclusion/installation of libavcodec headers.
> > > > > 
> > > > > > http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=6070b7e1c520e9ca389403bae20a2ad04c7d54c7
> > > > 
> > > > Hi all.
> > > > 
> > > > This commit seems to introduce/exhibit a serious memory leak that still
> > > > shows up in current master. Seems that frames that are decoded, but not
> > > > encoded (-ss) are not freed.
> > > 
> > > Have you evidence that there is currently a leak (I mean e.g. by using
> > 
> > Hit C-c before OOM killer got into the place. Usually my system only
> > swaps very old pages, here it saturated io.
> > 
> > > valgrind)? Also can you confirm that the leak if any was introduced by
> > > this commit and was not already present before?
> > 
> > git bisect, and checked that 6070b7e1c520e9ca389403bae20a2ad04c7d54c7^
> > does not show this behaviour.
> > Did you try my command (with -ss placed like I did holding a big enough
> > offset, eventually same filters too)?
> 
> 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.

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.
-------------- next part --------------
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;
     }