[vlc-devel] [vlc-commits] skins2: ensure playlist gets stopped before terminating vlc.

[erwan.tulou at gmail.com]

Hi,

On 02/12/2019 18:05, Rémi Denis-Courmont wrote:
> Le maanantaina 2. joulukuuta 2019, 14.11.28 EET erwan.tulou at gmail.com a écrit
> :
>> Hi,
>>
>> On 02/12/2019 09:20, Alexandre Janniaux wrote:
>>> Hi,
>>>
>>> Actually stopping the playlist wont kill the video output
>>> module, only killing the input resource, meaning releasing
>>> the vlc_player will do.
>>>
>>> However, player and playlist might be used by multiple
>>> control interface and this problem is only a concern for
>>> the one providing the vout_window. Hence I believe it must
>>> be solved by the mechanism providing the window instead of
>>> relying on a much wider API like playlist and player.
>> I fully agree with that. And actually, the work is already partially
>> done. A 'vout_window_ReportClose' is available to let the vout know that
>> the windows ids won't be available any more.
> No. That ReportClose does *not* mean that the window ID is no longer valid.
> Even back when the earlier incarnation of that callback (in the old vout core)
> "worked", all it did was request an asynchronous stop of the playlist
>
> It just means that the window system has advised that the window should be
> closed - normally because the user clicked on the window close button. The
> window ID must remain valid until the window is actually released by the core.
> Otherwise, you *will* introduce intractable races for the consumer of the
> window and, e.g., deadlock or crash OpenGL.

Okay, I fully agree with this, and that's why I am raising the issue 
with the new playlist.

vlc version 3.0 (the old playlist ) :

- stop the old playlist (this stops everything, all vout are freed)
- stop and destroy the interfaces (no problem since the vout are already 
gone)
- delete the playlist

vlc version 4.0 (the new playlist)

- stop the new playlist (this keeps a vlc player and the last vout with 
the vout window in a disabled state just in case we resume)
- stop the interfaces (Qt or skins2 terminates whatever vout is still 
running, luckily vout is disabled)
- delete the playlist (well, now we get rid of the vout window by 
issuing vout_window_close()  => crash expected !!!!!)

The problem is that :
- we cannot delete the playlist before stopping the interfaces (both Qt 
and skins2 expects a valid playlist throughout their lifetime)
- we cannot stop the interface before deleting the playlist, because 
vout_window_close() is still expected to be called (this is a callback 
function linked to the interface)

So, something has to be done to remedy this design issue. For now, there 
is a hack in skins2 to avoid crash and Qt uses --no-embedded-video, so 
it is not yet noticeable. Another workaround is that the 
vout_window_close() does nothing, and do all the cleaning up in 
vout_window_disable(). But then, why keep two functions if one needs to 
be a no-op.

Rgds
Erwan




>
> The GUI can and should call ReportClose when the interface is being closed by
> the user. But it *cannot* destroy the window immediately, nor can it wait for
> it to be released, which would cause a dead lock (been there done that).
>