On Sun, 11. Oct 22:04, Andriy Gelman wrote:
> On Sun, 11. Oct 22:44, Andreas Rheinhardt wrote:
> > Andriy Gelman:
> > > From: Andriy Gelman <andriy.gel...@gmail.com>
> > >
> > > Fixes #6334
> > >
> > > Signed-off-by: Andriy Gelman <andriy.gel...@gmail.com>
> > > ---
> > > libavformat/rtspdec.c | 17 ++++++++++-------
> > > 1 file changed, 10 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/libavformat/rtspdec.c b/libavformat/rtspdec.c
> > > index b519b6f1a2..623f478585 100644
> > > --- a/libavformat/rtspdec.c
> > > +++ b/libavformat/rtspdec.c
> > > @@ -720,7 +720,7 @@ static int rtsp_read_header(AVFormatContext *s)
> > > if (rt->rtsp_flags & RTSP_FLAG_LISTEN) {
> > > ret = rtsp_listen(s);
> > > if (ret)
> > > - return ret;
> > > + goto fail;
>
> >
> > This will add one ff_network_close() to this codepath. Is it really
> > certain that there was a corresponding ff_network_init()?
> > (And where is
> > the ff_network_init() that is cancelled in rtsp_read_close() anyway?)
>
> Besides my patch, there is an extra ff_network_init() in ff_rtsp_connect(),
> which is missing from rtsp_listen().
>
> This means there'll be an extra ff_network_close() call in listen mode (when
> the
> stream exits without errors).
>
> I think the best solution is to remove ff_network_init() from
> ff_rtsp_connect()
> and then remove ff_network_close() from rtsp_read_close() and the fail path of
> ff_rtsp_connect().
>
> Calling ff_network_init() separately in ff_rtsp_connect() seems redundant
> because the function is called in each url_alloc_for_protocol() anyway.. and
> it
> only complicates the cleanup after init failure.
>
> Something else I noticed when debugging the code:
> From the ffmpeg executable,
> avformat_network_deinit() is not called in the error path when
> rtsp_read_header() fails. This means we'll have an extra ff_network_init()
> call
> after the code exits following an error rtsp_read_header().
>
> I'm not sure if this is an issue for windows users... (i.e. if it leaves some
> socket resources open), or whether all the resources are automatically closed
> when the ffmpeg binary exits.
> Would it make sense to add an avformat_network_deinit() in the error path
> (maybe
> around ffmpeg_opt.c:1167?)
>
> >
> > > } else {
> > > ret = ff_rtsp_connect(s);
> > > if (ret)
> > > @@ -728,22 +728,25 @@ static int rtsp_read_header(AVFormatContext *s)
> > >
> > > rt->real_setup_cache = !s->nb_streams ? NULL :
> > > av_mallocz_array(s->nb_streams, 2 *
> > > sizeof(*rt->real_setup_cache));
> > > - if (!rt->real_setup_cache && s->nb_streams)
> > > - return AVERROR(ENOMEM);
> > > + if (!rt->real_setup_cache && s->nb_streams) {
> > > + ret = AVERROR(ENOMEM);
> > > + goto fail;
> > > + }
>
> >
> > With your patch, the calls to ff_network_init() and ff_network_close()
> > cancel each other out if the above allocation fails.
> >
>
> Yes, if libavformat is linked to the executable. All the
> ff_network_init()/ff_network_close() should cancel each other out.
>
> But from the ffmpeg binary we'll have +1 call to ff_network_init(), because
> avformat_network_deinit() is skipped in ffmpeg's error path (but
> avforamt_network_init() is still called).
>
sorry, it is called actually.. I had a mistake in the counts. Please disregard
this comment.
--
Andriy
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
