[FFmpeg-devel] [PATCH] ffprobe: allocate & free AVFrame per process_frame()
wm4
nfxjfg at googlemail.com
Tue Dec 24 15:00:50 CET 2013
On Tue, 24 Dec 2013 14:36:13 +0100
Michael Niedermayer <michaelni at gmx.at> wrote:
> On Tue, Dec 24, 2013 at 01:40:58PM +0100, wm4 wrote:
> > On Tue, 24 Dec 2013 13:24:39 +0100
> > Michael Niedermayer <michaelni at gmx.at> wrote:
> >
> > > On Tue, Dec 24, 2013 at 12:47:18PM +0100, Stefano Sabatini wrote:
> > > > On date Tuesday 2013-12-24 00:42:31 +0100, Michael Niedermayer encoded:
> > > > > Fixes memleak
> > > > >
> > > > > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > > > > ---
> > > > > ffprobe.c | 17 +++++++++--------
> > > > > 1 file changed, 9 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/ffprobe.c b/ffprobe.c
> > > > > index 2bb72ab..b675f8b 100644
> > > > > --- a/ffprobe.c
> > > > > +++ b/ffprobe.c
> > > > > @@ -1783,13 +1783,13 @@ static void show_frame(WriterContext *w, AVFrame *frame, AVStream *stream,
> > > > >
> > > > > static av_always_inline int process_frame(WriterContext *w,
> > > > > AVFormatContext *fmt_ctx,
> > > > > - AVFrame *frame, AVPacket *pkt)
> > > > > + AVPacket *pkt)
> > > > > {
> > > > > AVCodecContext *dec_ctx = fmt_ctx->streams[pkt->stream_index]->codec;
> > > > > AVSubtitle sub;
> > > > > int ret = 0, got_frame = 0;
> > > > > + AVFrame *frame = av_frame_alloc();
> > > > >
> > > > > - avcodec_get_frame_defaults(frame);
> > > > > if (dec_ctx->codec) {
> > > > > switch (dec_ctx->codec_type) {
> > > > > case AVMEDIA_TYPE_VIDEO:
> > > > > @@ -1806,8 +1806,10 @@ static av_always_inline int process_frame(WriterContext *w,
> > > > > }
> > > > > }
> > > > >
> > > > > - if (ret < 0)
> > > > > + if (ret < 0) {
> > > > > + av_frame_free(&frame);
> > > > > return ret;
> > > > > + }
> > > > > ret = FFMIN(ret, pkt->size); /* guard against bogus return values */
> > > > > pkt->data += ret;
> > > > > pkt->size -= ret;
> > > > > @@ -1822,6 +1824,8 @@ static av_always_inline int process_frame(WriterContext *w,
> > > > > if (is_sub)
> > > > > avsubtitle_free(&sub);
> > > > > }
> > > > > + av_frame_free(&frame);
> > > > > +
> > > > > return got_frame;
> > > > > }
> > > > >
> > > > > @@ -1853,7 +1857,6 @@ static int read_interval_packets(WriterContext *w, AVFormatContext *fmt_ctx,
> > > > > const ReadInterval *interval, int64_t *cur_ts)
> > > > > {
> > > > > AVPacket pkt, pkt1;
> > > > > - AVFrame *frame = NULL;
> > > > > int ret = 0, i = 0, frame_count = 0;
> > > > > int64_t start = -INT64_MAX, end = interval->end;
> > > > > int has_start = 0, has_end = interval->has_end && !interval->end_is_offset;
> > > > > @@ -1887,7 +1890,6 @@ static int read_interval_packets(WriterContext *w, AVFormatContext *fmt_ctx,
> > > > > }
> > > > > }
> > > > >
> > > > > - frame = av_frame_alloc();
> > > > > while (!av_read_frame(fmt_ctx, &pkt)) {
> > > > > if (selected_streams[pkt.stream_index]) {
> > > > > AVRational tb = fmt_ctx->streams[pkt.stream_index]->time_base;
> > > > > @@ -1920,7 +1922,7 @@ static int read_interval_packets(WriterContext *w, AVFormatContext *fmt_ctx,
> > > > > }
> > > > > if (do_read_frames) {
> > > > > pkt1 = pkt;
> > > > > - while (pkt1.size && process_frame(w, fmt_ctx, frame, &pkt1) > 0);
> > > > > + while (pkt1.size && process_frame(w, fmt_ctx, &pkt1) > 0);
> > > > > }
> > > > > }
> > > > > av_free_packet(&pkt);
> > > > > @@ -1932,11 +1934,10 @@ static int read_interval_packets(WriterContext *w, AVFormatContext *fmt_ctx,
> > > > > for (i = 0; i < fmt_ctx->nb_streams; i++) {
> > > > > pkt.stream_index = i;
> > > > > if (do_read_frames)
> > > > > - while (process_frame(w, fmt_ctx, frame, &pkt) > 0);
> > > > > + while (process_frame(w, fmt_ctx, &pkt) > 0);
> > > > > }
> > > > >
> > > > > end:
> > > > > - av_frame_free(&frame);
> > > > > if (ret < 0) {
> > > > > av_log(NULL, AV_LOG_ERROR, "Could not read packets in interval ");
> > > > > log_read_interval(interval, NULL, AV_LOG_ERROR);
> > > >
> > > > Question: is the leak a regression, depending on the frame API dance?
> > > > If not, why didn't we spot it before?
> > >
> > > libav changed the API in 37a749012aaacc801fe860428417a6d7b81c103f
> > > so that the free becomes required
> > >
> > > its not hard to fix that but then we are subtilely incompatible to
> > > libav and i suspect most users of the libs wont be happy about such
> > > differences in lifetime and deallocation behavior of some fields.
> > > its a two sided sword
> > > be compatible with current libav or be compatible to how the API worked
> > > in libav and ffmpeg in the past.
> >
> > This looks really suspicious to me. Reusing the frame should not leak.
> > With refcounting, this was recently fixed (the decoder seems to unref
> > incoming AVFrames), and for non-refcounted frames it was never needed.
> > Something is probably very wrong here, and user applications will be
> > silently affected too.
> >
> > Looking at the code, I have no idea what could be wrong, though.
>
> The commit messsage says it straight
> "Make a copy of the side data, so the caller can use av_frame_unref/free
> on non-refcounted frames, eliminating the need for
> avcodec_get_frame_defaults()/avcodec_free_frame()."
>
> if you make a copy, it has to be freed ... somewhere
> the old API freed the last frame on the next decode use or close
> (ignoring get/release buffer hadling of the pixels here)
> but now it has to be explicitly freed (for example by passing the
> AVFrame into the decoder again or using *_frame_free()
>
> ffprobe does reuse the AVFrame but it does clear it first
> we could drop that clear but IIRC the old API at some point in the
> past required user apps to do that clear.
Uh what? Are you saying side data is somehow treated differently from
buffer data? E.g. looking at avcodec_decode_audio4, nowhere does it say
you have to do anything special to the AVFrame. (It does say that you
have to use av_frame_unref() in the refcounted case, which is
probably not needed anymore by now. But for the unrecounted case, it
doesn't say anything.)
I'm probably missing something obvious here...
More information about the ffmpeg-devel
mailing list