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

[Alexandre Janniaux]

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);
 }
 
 /**
@@ -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);
             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