[FFmpeg-devel] [PATCH] Fix for rc_eq memory leak

[Stefano Sabatini]

On date Monday 2008-08-04 15:39:15 -0700, Art Clarke encoded:
> On Mon, Aug 4, 2008 at 2:11 PM, Stefano Sabatini <
> stefano.sabatini-lala at poste.it> wrote:
> 
> > I continue to insist with a different solution, according to me we
> > should close the codecs even jsut after we call
> > av_input_find_stream_info(), having duplicated the same code in
> > avcodec_close() and av_close_input_stream() doesn't look like a
> > particularly good idea.

Rethinking at it, maybe my solution (to free rc_eq when the codec is
closed) is a wrong solution.

Suppose you want to close a codec, change some settings and then
reopen it again, in this case you *don't want* to change the
information contained in the context, you want indeed to actually keep
it.

I'm not sure if the priv_data and extradata contain informations which
should be kept in the context independant, but I think it's safe to
keep them.

So rc_eq (and eventually priv_data and extradata) should be freed just
before freeing the AVCodecContext itself, so your solution looks
right.

Maybe in order to avoid memory leaks a good idea could be to implement
an avcodec_context_free()/avcodec_context_close() function (the latter
could also call avcodec_close).

So a plan could be:
1) revert 14260

2) to free rc_eq and friends just before a codec context is freed,
   *or* use a new function for doing it.

Point 2 implies your patch.

> > Said that I'm not a maintainer of anything so I can just express my
> > opinion but I have no final word.
> >
> > As for an alternative solution, I started to hack a solution, then I
> > got stuck with some extradata related SIGSEGVs, so I left it apart,
> > I'll hopefully come back to it in some time.
> >
> 
> I originally went down the path you suggested to fix it (i.e. have
> av_input_find_stream_info clean up after itself), but I stopped once I
> realized some folks may have written programs that depend upon
> av_input_find_stream_info opening the codecs and leaving them open.  Fixing
> it "the right way" may (a) break code that accidentally depends on that
> behavior (i.e. buggy code that happens to work today) or (b) break code that
> purposefully depends on av_input_find_stream_info leaving the codecs open
> (i.e. highly optimized code that was written with the knowledge and
> expectation that av_input_find_stream_info leaves the codecs open).
> 
> Why would someone depend upon av_input_find_stream_info leaving the codec
> open?  Some codecs have relatively heavy opening procedures; they start
> parsing and decoding packets to try to figure out things like width, height,
> frame-rate etc.  libav appears to cache those packet reads in memory so that
> when an program starts doing calls to read-packets, the packets that
> av_input_find_stream_info read are retrieved from a buffer (rather than from
> disk).  I'm not 100% positive about this, but I believe if you did a
> avcodec_close at that stage you might invalidate some of those caches.
> 
> So my fix currently airs on the side of safety; if you ever get to the code
> I added, and rc_eq is not null, you can be 100% sure it needs to be freed.
> It might be because (a) of sloppy libav programming or (b) highly optimized
> libav programing, but regardless, it should be freed at that point.
> 
> The second reason why I made the one-line change here was because it appears
> the maintainers of ffmpeg prefer simple (and easy to understand) changes
> over more complex (but arguably more correct) changes.  Given the library is
> a complex piece of moving code, I think the simple changes over complex
> changes is the better approach.  The area of the code in question where I
> added the av_free() call already does cleanup of other codec-related items
> that were opened by av_input_find_stream_info, so it seems being complete
> there is the right thing to do.
> 
> All that said, I'm with you Stefano in that we need direction / thoughts
> from the maintainer.  Who maintains libavformat/utils.c?

Michael? (sorry for bothering you so often ;-))

Regards.
-- 
FFmpeg = Foolish and Faboulous Multipurpose Programmable Enigmatic Glue