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

[Alexandre Janniaux]

Hi,

After some thoughs, surveys of locks and schematics I might
have a solution for allowing the vout_display::Open to block
the main thread without additional lock.

It seems that I can just move the display creation outside of
the display_lock, and then lock it to share it with sys while
applying the pending requested state after the Open() has
ended, so that the behaviour stays the same.

I'm just still unsure about whether is should be created in
window lock or not.

I'll do some testing and suggest a patch after Thomas's
patches are merged if no other points is raised in the
discussion, but it feels more natural than previous points
suggested here and when comparing to the git history and
git grep, it doesn't seem wrong.

Regards,
--
Alexandre Janniaux
Videolabs

On Fri, Dec 13, 2019 at 02:26:48PM +0100, Alexandre Janniaux wrote:
> 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel