[vlc-devel] Handling deadlock between display:Open() and vout_window_ReportSize()

[Alexandre Janniaux]

Hi,

I splitted the iOS vout display modules into a window module
and an opengl provider module, so as to use the same GLES2
vout display as on Linux and Android, provide the ground work
for handling different vout than OpenGL on iOS and refactor
to use the vout_window concepts and event handling, which
removes the need for vout_display_SendEvent* on iOS.

Overall, this is working fine, but because of the hard
requirements related to UI and main thread on iOS, I sometime
run into the following deadlock:

+ the vout_thread starts.
+ it enables the UIView window.
    => the UIView register to its superview in the main
       thread.
+ it locks the display_lock and starts opening the GLES2 vout
  display.
    => the UIView gets notified of a resize from the main
       thread, calls vout_window_ReportSize, get stuck in the
       display_lock in the main_thread.
+ the display module creates an OpenGL provider, which needs
  to register the openGL-backed UIView surface from the main
  thread, which is stuck. It cannot be done asynchronously
  because we need to tell the core whether it was created
  correctly.

To fix this, I'd like to change the requirements on the vout
display interface, and explicitly allow the vout_display
modules to block the main_thread in the Open() and handle
this correctly.

To achieve this, I need something like the following
incomplete diff, which is NOT written against Thomas's
patches for now. It adds a third lock in the display module,
which is basically never locked in the vout_thread except
during the vout_display initialization.

I'm not sure whether this is correct or enough, so I come
here for getting advice on this, especially from people
having experience with the window handling and may have an
alternative implementation for this.

I also don't know whether it's enough or not, because the
main thread seems to also be able to get stuck in
vlc_player_Delete as well, which maybe might be called during
the Open() of the display? (I've not checked this but got in
similar deadlock already).

I also only locked for resize (because it's my current issue)
but other states must be handled like this too. Previously
we didn't have this issue with embedded window because they
were handled with asynchronous vout controls.

Thank you for your feedback,

diff --git a/src/video_output/video_output.c b/src/video_output/video_output.c
index 659ede546b..193d9e69ca 100644
--- a/src/video_output/video_output.c
+++ b/src/video_output/video_output.c
@@ -420,14 +420,27 @@ void vout_ChangeDisplaySize(vout_thread_t *vout,
                             unsigned width, unsigned height)
 {
     vout_thread_sys_t *sys = vout->p;
-
     assert(!sys->dummy);

+    vlc_mutex_lock(&sys->resize_lock);
+    sys->window_width  = width;
+    sys->window_height = height;
+    if (sys->display == NULL)
+    {
+        /* We haven't created the display yet, maybe we received the
+         * resize event a bit too soon. */
+        vlc_mutex_unlock(&sys->resize_lock);
+        return;:
+    }
+
     /* DO NOT call this outside the vout window callbacks */
+
     vlc_mutex_lock(&sys->display_lock);
     if (sys->display != NULL)
         vout_display_SetSize(sys->display, width, height);
     vlc_mutex_unlock(&sys->display_lock);
+
+    vlc_mutex_unlock(&sys->resize_lock);
 }

 void vout_ChangeDisplayFilled(vout_thread_t *vout, bool is_filled)
@@ -1603,14 +1616,25 @@ static int vout_Start(vout_thread_t *vout, vlc_video_context *vctx, const vout_c

     num = sys->source.dar.num;
     den = sys->source.dar.den;
+
     vlc_mutex_lock(&sys->display_lock);
     vlc_mutex_unlock(&sys->window_lock);

-    sys->display = vout_OpenWrapper(vout, sys->splitter_name, &dcfg, vctx);
+    vout_display_t *display = vout_OpenWrapper(vout, sys->splitter_name,
+                                               &dcfg, vctx);
+
+    /* Resize lock prevent from getting state update but allow the window to
+     * post its update to be executed after display is created. */
+    vlc_mutex_lock(&sys->resize_lock);
+
+    sys->display = display;
     if (sys->display == NULL) {
         vlc_mutex_unlock(&sys->display_lock);
+        vlc_mutex_unlock(&sys->resize_lock);
         goto error;
     }
+    // TODO: read and set size before leaving here
+    vlc_mutex_unlock(&sys->resize_lock);

     vout_SetDisplayCrop(sys->display, num, den, x, y, w, h);

@@ -1946,6 +1970,7 @@ vout_thread_t *vout_Create(vlc_object_t *object)
     /* Display */
     sys->display = NULL;
     vlc_mutex_init(&sys->display_lock);
+    vlc_mutex_init(&sys->resize_lock);

     /* Window */
     sys->display_cfg.window = vout_display_window_New(vout);
diff --git a/src/video_output/vout_internal.h b/src/video_output/vout_internal.h
index 0ca81871fe..8859edcd43 100644
--- a/src/video_output/vout_internal.h
+++ b/src/video_output/vout_internal.h
@@ -183,6 +183,7 @@ struct vout_thread_sys_t
     vout_display_cfg_t display_cfg;
     vout_display_t *display;
     vlc_mutex_t     display_lock;
+    vlc_mutex_t    *resize_lock;

     picture_pool_t  *private_pool;
     picture_pool_t  *display_pool;


Regards
--
Alexandre Janniaux
Videolabs