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

[Stefano Sabatini]

On date Friday 2013-11-15 00:36:20 +0100, Lukasz M encoded:
> On 14 November 2013 15:53, Lukasz M <lukasz.m.luki at gmail.com> wrote:
> 
> > 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.
> >>
> >
> Removed that hunk. I remember I got valgrind complaining about that
> allocation, but cannot reproduce it now.
> 
> >
> >> > -    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).
> >
> >
> Updated this way.

> From c758e15f21f6ae3236f6938d675c8947c3a333a7 Mon Sep 17 00:00:00 2001
> From: Lukasz Marek <lukasz.m.luki at gmail.com>
> Date: Wed, 13 Nov 2013 23:40:47 +0100
> Subject: [PATCH 3/3] lavd/xv: free resources on errors
> 
> xv_write_header callback leave not freed resources on errors.
> 
> Signed-off-by: Lukasz Marek <lukasz.m.luki at gmail.com>
> ---
>  libavdevice/xv.c |   53 +++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 20 deletions(-)

LGTM, will apply it soon, thanks.
-- 
FFmpeg = Faithful & Furious Muttering Political Emblematic Gigant