[FFmpeg-devel] [PATCH] ffprobe: allocate & free AVFrame per process_frame()
Michael Niedermayer
michaelni at gmx.at
Tue Dec 24 17:02:41 CET 2013
On Tue, Dec 24, 2013 at 03:00:50PM +0100, wm4 wrote:
> 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?
yes
> 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...
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
What does censorship reveal? It reveals fear. -- Julian Assange
-------------- 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/0c96c9cd/attachment.asc>
More information about the ffmpeg-devel
mailing list