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

[Thomas Guillem]

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