[FFmpeg-devel] [PATCH] Fix for rc_eq memory leak
Tue Aug 5 00:39:15 CEST 2008
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.
> 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?
More information about the ffmpeg-devel