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

wm4 nfxjfg at googlemail.com
Tue Dec 24 13:40:58 CET 2013


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.


More information about the ffmpeg-devel mailing list