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

[Michael Niedermayer]

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.


> 
> The original intent of allocating the frame outside process_frame()
> was to avoid unnecessary alloc/free calls, but if that it's now
> required for some reasons then patch is fine.
> 
> Thanks.
> -- 
> FFmpeg = Fascinating & Fantastic Mastodontic Patchable Erotic Gem
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 3
"Rare item" - "Common item with rare defect or maybe just a lie"
"Professional" - "'Toy' made in china, not functional except as doorstop"
"Experts will know" - "The seller hopes you are not an expert"
-------------- 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/62ea092b/attachment.asc>