[FFmpeg-devel] [PATCH 3/3] lavd/xv: free resources on errors

[Lukasz M]

On 14 November 2013 12:33, Stefano Sabatini <stefasab at gmail.com> wrote:

> On date Wednesday 2013-11-13 23:40:47 +0100, Lukasz Marek encoded:
> > write_trailer callback leave not freed resources on errors.
> >
> > Signed-off-by: Lukasz Marek <lukasz.m.luki at gmail.com>
> > ---
> >  libavdevice/xv.c |   11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavdevice/xv.c b/libavdevice/xv.c
> > index bfa6ff5..008e818 100644
> > --- a/libavdevice/xv.c
> > +++ b/libavdevice/xv.c
> > @@ -120,15 +120,13 @@ static int xv_write_header(AVFormatContext *s)
> >                                       xv->window_x, xv->window_y,
> >                                       xv->window_width,
> xv->window_height,
> >                                       0, 0, 0);
> > -    if (!xv->window_title) {
> > -        if (!(xv->window_title = av_strdup(s->filename)))
> > -            return AVERROR(ENOMEM);
> > -    }
> > -    XStoreName(xv->display, xv->window, xv->window_title);
> > +    XStoreName(xv->display, xv->window, xv->window_title ?
> xv->window_title : s->filename);
> >      XMapWindow(xv->display, xv->window);
>
> Partially unrelated. Also maybe it is good to explicitly set
> window_title in the context.
>

Valgrind complained about not loaded xv->window_title, but this may be bug
inside ffmpeg.c
I will check it later if leak also occurs when xv->window_title is set by
ffmpeg's option.
Anyway, I'm not sure want you want? You prefer to copy s->filename to
xv->window_title or not?


>
> >
> > -    if (XvQueryAdaptors(xv->display, DefaultRootWindow(xv->display),
> &num_adaptors, &ai) != Success)
> > +    if (XvQueryAdaptors(xv->display, DefaultRootWindow(xv->display),
> &num_adaptors, &ai) != Success) {
> > +        XCloseDisplay(xv->display);
> >          return AVERROR_EXTERNAL;
> > +    }
> >      xv->xv_port = ai[0].base_id;
> >      XvFreeAdaptorInfo(ai);
> >
> > @@ -146,6 +144,7 @@ static int xv_write_header(AVFormatContext *s)
> >          av_log(s, AV_LOG_ERROR,
> >                 "Device does not support pixel format %s, aborting\n",
> >                 av_get_pix_fmt_name(encctx->pix_fmt));
> > +        XCloseDisplay(xv->display);
> >          return AVERROR(EINVAL);
> >      }
>
> LGTM.
>
> Probably cleaner: you do all the deinit stuff in write_trailer, and
> call it from write_header() in case of failure (e.g. how I did it in
> sdl.c).
>

OK. I will do it this way.