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

[Alexandre Janniaux]

Hi,

Thank you for the detailed feedback on this !

Just to make sure we're on the same track, the goal here is to be able
to close windows created from the interfaces before the interfaces are
closed ?

On Mon, Dec 02, 2019 at 01:11:28PM +0100, erwan.tulou at gmail.com wrote:
> 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. For now the function is not implemented

Actually, those functions are there to signal that the user or WM
requested to close the window, but it's up to the window client to
remove the ES, close the other window in the case of a splitter, etc.

It would also be pretty reasonable to me if such requests were
"cancellable", meaning that you can prevent close window from doing
anything like in the current Windows behaviour.

I'm not sure that it is the correct layer to trigger window deletion
and I would prefer it to come at the window provision/usage level
instead of going through the window modules.

>
> static void vout_display_window_CloseNotify(vout_window_t *window)
> 229 <http://git.videolan.org/?p=vlc.git;a=blob;f=src/video_output/window.c;h=16ebf9ef5a2252b896dd5edddd66fc2bc3465f56;hb=HEAD#l229>
> {
> 230 <http://git.videolan.org/?p=vlc.git;a=blob;f=src/video_output/window.c;h=16ebf9ef5a2252b896dd5edddd66fc2bc3465f56;hb=HEAD#l230>
>     /* TODO: Nowhere to dispatch to currently.
> 231 <http://git.videolan.org/?p=vlc.git;a=blob;f=src/video_output/window.c;h=16ebf9ef5a2252b896dd5edddd66fc2bc3465f56;hb=HEAD#l231>
>      * Needs callback to ES output to deselect ES? */
> 232 <http://git.videolan.org/?p=vlc.git;a=blob;f=src/video_output/window.c;h=16ebf9ef5a2252b896dd5edddd66fc2bc3465f56;hb=HEAD#l232>
>     msg_Err(window, "window closed");
> 233 <http://git.videolan.org/?p=vlc.git;a=blob;f=src/video_output/window.c;h=16ebf9ef5a2252b896dd5edddd66fc2bc3465f56;hb=HEAD#l233>
> }
>
> Implementing this function would kill two birds with one stone. When
> terminating vlc, GUI modules with embedded videos can use it to properly
> terminate vout. But, more generally, this closing of windows can occur at
> any time, and it is wise to also properly terminate vout if this occurs.
>
> When saying 'terminate vout', it means terminate and desallocate the
> resource. This is a case where the window id must not be retained for a
> possible future use. In other words, as the result of issueing 
> vout_window_ReportClose(), the response must be vout_window_Close().
> Also, this is not only related to the player. It must be a more generic
> thing because vout can also be an audio visualization or anything else.
> Maybe the caller of vout_Create() has to provide a callback to deal with
> this event.

The window modules must stay open even if the window is closed,
triggering an input stop for instance. The reason behind this is that
the window modules is also monitoring notification from the underlying
window systems, like screen hotplug or dimension changes. I don't think
it is that easy to solve both problem with the same solution.

`vout_window_Close()` also looks like a request to the vout_window
module so it's probably a confusing name. `vout_window_ReportClose`
will report the action to the window user, which is currently a
vout_thread for this case.

And actually, because of `close` being a user/WM request, you will
probably not find a `vout_window_Close` because it makes not much sense
to have a user-action-grade request to the window while we already
control how it is used. The correct way to "prevent the window from
being used again" is destroying it and making sure it is not used
anywhere.

But indeed, you have the callback that you quoted above which is pretty
unimplemented client-side and should report the action through the
`vout_thread` or another mechanism to be useful (closing the ES,
stopping the playback, stopping all window in the splitter, maybe
changing configuration... it's up to the client).

I discuss a solution to this more extensively below.

>
> >
> > I wrote part of it that I shared as a github branch some
> > weeks ago, but it is not complete yet. I had also explored
> > alternative solution like a vlc_player_disable-like
> > mechanism (that I called vlc_player_ResetResource iirc) but
> > I find it not very elegant.
>
> Yet, as of today, when terminating vlc, it is not clear who is responsible
> for just properly stopping the playlist. I realized that deleting the
> playlist doesn't stop the decoders for instance. The Qt interface stops the
> playlist in MainInterface::closeEvent() (modules/gui/qt/main_interface.cpp)
> which is a weird place. For lack of a better place, the patch I wrote was
> just mimicking Qt because stopping the playlist has to be done somewhere.
>
> I actually think it is the libvlc overall responsability to stop the
> playlist since starting the playlist can be done very early in the process
> without any GUI already set up.
>
> this vlc_playlist_disable would be very simple, lock the playlist, stop it,
> unlock it + an enabled flag to prevent any new starting. And this is just
> triggering a stop. The actual stop will happen much later when the vout are
> indeed released by a proper desallocation mechanism between the GUI and the
> vout core and all the listeners on the player/vout are released by the GUI.

The point about who needs to stop the playlist is not important for me.
I think it's not the correct question for this issue.

Here for the issue mentionned first in this answer, you actually don't
care about being able to kill any window before the interface. The main
issue is killing the window provided by the interface before the
interface is killed.

Killing all windows while stopping the playlist would work but it
raises a lot of questions like this and puts strong constraint on the
API design.

What I suggested in the github branch and the technical meeting is to
have a interface-based window providing method in addition to the
module-based one, with client tokens allowing metadata passing and
commands on the actual window client. Note that it really concerns the
window client, for which we want to signal the window will not exist
anymore much like you wanted with the `vout_window_Closed` signal. It
means that the display part in `vout_thread`, the visualization, the
splitter using this mechanism could be notified the same way as you
wanted but with a different API that vout_window.

I posted a draft for the technical meeting, which is still incomplete
but should provide a more detailed (but complex) explanation:

https://github.com/alexandre-janniaux/vlc-rfcs/blob/master/RFCS/00001-window-provisioning.md

Here in the snippet I gave, you could do something as simple as the
following when stopping the interface to remove the window from the
clients. Here `sys` would be the interface one, and `handle` the
variable from the window provider callback that the interface must
store.

```
vlc_window_client_RequestTerminate(sys->handle);
```

I would be very happy to have feedback to be sure it solves the same
issue as yours, but my PoC of video integration is still unfinished.

>
>
> > Is skins2 interface embedded work using interface_widget
> > from the Qt interface?
>
> No, skins2 doesn't rely on Qt for vout windows. The Qt thing is only dialog
> boxes, menus and hotkeys management (including key accelerators from Qt).
> Yet, the full Qt Playlist and Player controllers are set up which means that
> skins2 is highly dependent on Qt indirectly.

Ok, then this code is probably obsolete and unused in 4.0 now. Thank
you for the info !

Regards,
--
Alexandre Janniaux
Videolabs