[FFmpeg-devel] [PATCH 3/3] lavd/xv: free resources on errors
Lukasz M
lukasz.m.luki at gmail.com
Thu Nov 14 15:53:18 CET 2013
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.
More information about the ffmpeg-devel
mailing list