[vlc-devel] [PATCH] lib: media: wait preparse end before release

[Alexandre Janniaux]

Hi,

Ok for the signal in the lock.

I don't think I need to signal in the other case, since releasing
the libvlc_media while using it to ask for preparsing looks like
an application race. I'll remove it.

Regards,
--
Alexandre Janniaux
Videolabs

On Thu, Feb 11, 2021 at 05:23:48PM +0100, Thomas Guillem wrote:
>
>
> On Thu, Feb 11, 2021, at 16:32, Alexandre Janniaux wrote:
> > This is a quick fix for #25296 actually reverting the commit
> > a708c988091d6bc770212f5deeaba45e2395f4b2, replacing the Hold/Release
> > lifecycle used there by an ad-hoc Hold/Release which actually defer the
> > actual release by blocking instead of releasing in the preparser thread.
> >
> > Indeed, libvlc_media is holding the instance used to create it, though
> > it only needs it for preparsing. If the last reference of the media is
> > hold by the preparser, libvlc_media will be released by the preparser
> > thread and the libvlc_instance potentially released from there too.
> > Since killing the libvlc_instance will join the preparser thread, it
> > cannot be done from the preparser thread, or would lead to the failure
> > detailed in #25296. Written differently, only the final client should
> > make use of Hold/Release, directly through the adequate function or
> > indirectly through the creation of new objects.
> >
> > In addition, a708c988091d6bc770212f5deeaba45e2395f4b2 cannot just be
> > reverted as it was fixing cases where the libvlc_media_t object were
> > released before preparsing is finished, and the libvlc application
> > cannot just wait for the preparsing event to arrive since it must still
> > typically unlock the event manager and manage subitems.
> >
> > Ideally, the libvlc_media would just not have a strong reference to the
> > libvlc instance, which would allow detaching the libvlc state from the
> > libvlccore state, but it cannot be done just yet, since the preparsing
> > API must be splitted from libvlc_media first.
> >
> > Fix #25296
> > ---
> >  lib/media.c          | 29 ++++++++++++++++++++++++++---
> >  lib/media_internal.h |  6 ++++++
> >  2 files changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/media.c b/lib/media.c
> > index 19283d4f44..fa9939e3d8 100644
> > --- a/lib/media.c
> > +++ b/lib/media.c
> > @@ -432,7 +432,12 @@ static void input_item_preparse_ended(input_item_t *item,
> >              return;
> >      }
> >      send_parsed_changed( p_md, new_status );
> > -    libvlc_media_release( p_md );
> > +
> > +    vlc_mutex_lock(&p_md->parsed_lock);
> > +    assert(p_md->worker_count > 0);
> > +    p_md->worker_count--;
> > +    vlc_mutex_unlock(&p_md->parsed_lock);
> > +    vlc_cond_signal(&p_md->idle_cond);
>
> signal while locked please.
>
> >  }
> >
> >  /**
> > @@ -508,6 +513,9 @@ libvlc_media_t * libvlc_media_new_from_input_item(
> >      vlc_mutex_init(&p_md->parsed_lock);
> >      vlc_mutex_init(&p_md->subitems_lock);
> >
> > +    vlc_cond_init(&p_md->idle_cond);
> > +    p_md->worker_count = 0;
> > +
> >      p_md->state = libvlc_NothingSpecial;
> >
> >      /* A media descriptor can be a playlist. When you open a playlist
> > @@ -652,6 +660,12 @@ void libvlc_media_release( libvlc_media_t *p_md )
> >      /* Cancel asynchronous parsing (if any) */
> >      libvlc_MetadataCancel( p_md->p_libvlc_instance->p_libvlc_int, p_md );
> >
> > +    /* Wait for all async tasks to stop. */
> > +    vlc_mutex_lock( &p_md->parsed_lock );
> > +    while( p_md->worker_count > 0 )
> > +        vlc_cond_wait( &p_md->idle_cond, &p_md->parsed_lock );
> > +    vlc_mutex_unlock( &p_md->parsed_lock );
> > +
> >      if( p_md->p_subitems )
> >          libvlc_media_list_release( p_md->p_subitems );
> >
> > @@ -862,13 +876,22 @@ static int media_parse(libvlc_media_t *media,
> > bool b_async,
> >          if (parse_flag & libvlc_media_do_interact)
> >              parse_scope |= META_REQUEST_OPTION_DO_INTERACT;
> >
> > -        libvlc_media_retain(media);
> > +        /* Note: we cannot keep parsed_lock when calling libvlc_MetadataRequest
> > +         * because it might also be used to submit the state synchronously
> > +         * which would result in recursive lock. */
> > +        vlc_mutex_lock(&media->parsed_lock);
> > +        media->worker_count++;
> > +        vlc_mutex_unlock(&media->parsed_lock);
> > +
> >          ret = libvlc_MetadataRequest(libvlc, item, parse_scope,
> >                                       &input_preparser_callbacks, media,
> >                                       timeout, media);
> >          if (ret != VLC_SUCCESS)
> >          {
> > -            libvlc_media_release(media);
> > +            vlc_mutex_lock(&media->parsed_lock);
> > +            media->worker_count--;
> > +            vlc_mutex_unlock(&media->parsed_lock);
> > +            vlc_cond_signal(&media->idle_cond);
>
> Why are you signalling here?
>
> >              return ret;
> >          }
> >      }
> > diff --git a/lib/media_internal.h b/lib/media_internal.h
> > index b8646abce2..511a0aad15 100644
> > --- a/lib/media_internal.h
> > +++ b/lib/media_internal.h
> > @@ -48,6 +48,12 @@ struct libvlc_media_t
> >      vlc_mutex_t parsed_lock;
> >      vlc_mutex_t subitems_lock;
> >
> > +    /* Idle protection to prevent the media from being released during
> > +     * preparsing. The preparse will be cancelled but the release will
> > +     * be blocking until no async code is using the media anymore. */
> > +    vlc_cond_t idle_cond;
> > +    size_t worker_count;
> > +
> >      libvlc_media_parsed_status_t parsed_status;
> >      bool is_parsed;
> >      bool has_asked_preparse;
> > --
> > 2.30.1
> >
>
> Otherwise, LGTM.
>
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel