[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?
> 
> 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.

applied the simpler variant that removes the avframe clearing

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

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- 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/20131225/166d2043/attachment.asc>