[FFmpeg-devel] [PATCH] ffprobe: allocate & free AVFrame per process_frame()

[Michael Niedermayer]

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.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131224/080e2a4b/attachment.asc>