[vlc-devel] [PATCH 3.x 0/2] RFC: libvlc_media_player: add async stop

[Alexandre Janniaux]

Hi,

On Mon, Oct 19, 2020 at 04:48:16PM +0200, Thomas Guillem wrote:
>
>
> On Mon, Oct 19, 2020, at 16:29, Rémi Denis-Courmont wrote:
> > Le maanantaina 19. lokakuuta 2020, 17.07.35 EEST Thomas Guillem a écrit :
> > > I don't think the current libvlc_media_player_stop() is usable since it
> > > is blocking. VLC ports usually call it from background threads, but it
> > > lead to deadlocks when the mainthread is calling libvlc_media_player getter
> > > functions.
> >
> > liblvc_media_player_stop() is inconvenient for sure, but I don't exactly
> > follow how it would be unusable.
>
> Indeed, inconvenient is a better word for that situation.
>
> > From what I have seen, deadlocks are usually
> > caused by threading mistakes in applications. In particular, if there is a
> > deadlock, how come libvlc_media_player_release() is not exhibiting the same
> > problem?
>
> libvlc_media_player_release() will block but not deadlock since there should not be any users interaction when it's called.
> By the way, I don't like the fact that it is refcounted since the user can't know if the call will block or not.
> Ideally, libvlc and mediaplayer should be destroyed after the UI is destroyed.
> >
> > If it's really necessary to update LibVLC 3.x, then that's that, but you're a
> > bit light on details here, IMO.
>
> Here is an example of deadlock https://code.videolan.org/-/snippets/1286/raw
>
> 1/ libvlc_media_list_player_play_item_at_index() that will call input_Close() is called from a background thread (as a hack, since it is a blocking call).
> 2/ The setting of the new media cause the ios vout to wait for the mainthread.
> 3/ libvlc_video_get_track_count() is called from the mainthread and wait for the input_thread that is closing.
>
>  => Deadlock.
>
> The point 2 is debatable, maybe we should fix the ios vout to not wait for the mainthread.

You cannot fix the iOS vout to not wait for the main thread during Open
without «blindly» returning VLC_SUCCESS without knowing whether the vout
will work or not. It could be ok for 3.0.x on iOS if we consider we'll
never provide a CALayer vout, but otherwise it's a bit against the design.

I had actually discussed this exact issue in the context of asynchronous
resize of window during the opening of display, which is 4.0 ground.
I typically solved it by not «binding» the window to the layer tree until
a display is attached, reporting the parent size once synchronously in the
open. In my opinion the debatable points are 1/ and 3/ but not 2/.

2/ is inherent to how UI works on iOS/macOS and can hardly be modified,
the modules has to comply to this and take care of this.

On the opposite, 1/ and 3/ are really brought by libvlc, and fixable in
VLC (though maybe not in 3.0, so your patch might be ok for this). For
instance, why is libvlc_video_get_track_count() even locking the player
or input thread whereas we provide an asynchronous way of sending those
information with the appropriate lock? It could just cache the last
reported state (considered «current» by the UI) and it would provide the
same result with a lock at the libvlc_media_player (thus, bypassing the
issues within VLCKit) instead of locking the whole pipeline.

Ideally, it's the application role to track it within the asynchronous
execution of callbacks and push state migration to its event loop. What
is suggested in the paragraph just above is just a «racy» cache but it's
no less than what is currently available in libVLC.

Likewise with libvlc_media_list_player_play_item_at_index, why does it
have to kill the media from the caller? The new design in libvlc 4 allows
for setting a «get_next_media» callback which would provide the necessary
glue to «set next player media» and «request next media to be played»
within the media_list_player (if not done already) so as to avoid doing
anything in the main thread. libvlc 3.0 could probably have a similar
mechanism with the old playlist to provide that instead.

>
> Anyway, the current situation forbid LibVLC users to set new media from the mainthread, that is why a background thread is used for 1/.

Yeah, I think this is quite important to solve for the LTS version, in a
way or in another.

> >
> > --
> > 雷米‧德尼-库尔蒙
> > http://www.remlab.net/
> >
> >
> >
> > _______________________________________________
> > 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