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

[Alexandre Janniaux]

Hi,

On Tue, Dec 03, 2019 at 03:33:52PM +0100, erwan.tulou at gmail.com wrote:
> Hi all,
>
>    After diving into the code, I think we are not that far away from a clean
> solution.

Thank you for putting up all this in one post.

>
>   here is what I think
>
>   at vlc termination, desallocation should indeed be done in a lifo style,
> which means
>   first : stop and destroy all interfaces
>   second : stop and destroy playlist

It seems correct to me too.

>
> More generally, any interface(embedded video or not) should be able to be
> started and stopped at any time. The playlist should not be forced to stop
> because an interface is stopped.

This is imho an auxiliary question but I don't think it even
change anything regarding your issue. It gives a clearer
background thought so let's say yes anyway.

Just for the record though, there are one main interface
given by --intf and auxiliary control interfaces given by
--extraintf so we might find a simpler ground here for the
future.

>
> That means that interfaces with embedded video must gracefully ask to regain
> their ressources (vout_window_ReportClose). Most likely, it means that the
> video ES goes unselected, the audio visualization goes unchecked, ... and
> the player just keeps working.

That's a choice of behaviour. I like it because it quite like
on Android where you can put the player in background and
have the video without audio. However, it might make sense
to pause or stop the playback too. I personnaly don't care
about this.

But yes, when closing, the interfaces must gracefully ask to
regain their resources and become the only owner of the
window.

> At the vout window level, vout_window_Close() is **useless**. Today, it is
> simply used in place of the Close() function when unloading the vout window
> module. So, just remove it everywhere, and reinstate the Close().

I don't agree with that for the reasons I invoked in a
previous post here. `vout_window_Close` currently doesn't
exist and I think it brings a different concept than the
`vout_window_ReportClose` function which is far lower level.

To apply your idea though, you could add another couple of
request / event to signal the window client that it must stop
using it. My idea was to put this request/event at the level
of the client through a window token instead of the window.

> A recap of what we have at the vout_window level  :
> - Open () (vout window module Open function)
> - vout_window_enabled()
> - vout_window_disabled()
> - Close() (vout window module Close function)

Right.

>
> I suggest that the window ID is valid only in the enabled phase. It is up to
> each vout window implementation to choose whether the same window ID is
> reused or a new window ID is provided when vout_window_enabled() is called.
> Of course, when disabled, that does not  prevent the vout window module to
> continue advertizing for instance hotplugged new monitors or anything else,
> and also vlc core issuing commands as long as it is not the display
> accessing the window ID. What the vout window module does with the commands
> passed in disabled state is implementation dependent. (is it a no-op, does
> it retain the state for future uses, ... ?)

That's the current behaviour yes, even if hotplug is
currently barely supported by a few modules! :)

However in disabled state, it should retain the state for the
future enable request, so as to start displaying the window
immediately with the correct state.

>
> In short, at the core level, what is needed is:
> - implementing vout_window_ReportClose() (today it ends up in msg_Err .. +
> no-op)
> - just remove vout_window_Close() everywhere
>
> for the vout_window module
> -  reinstate Close() function instead of responding to vout_window_Close()

Same as my comments above, but it could be replaced with a
"create a new vout_window_FooBar". The event itself is
probably not meaningful so it could shortcircuit the module
implementation and just call the core-side callbacks
functions, which reduce the amount of work to only API side
and window client side.

> for GUI interface with vout window capabilities (Qt and skins2)
> - remove playlist forcefully being stopped at interface termination
> - gracefully ask to regain ressources (vout_window_ReportClose) whenever it
> is useful (and anyway needed at module termination whether because vlc is
> terminating or just the module gets unselected)

Correct to me too. I highlight here that only interfaces
have this need, and only for window they created.

> at the libvlc level,
> - a state-of-the-art desallocation (the lifo style) where the playlist is
> stopped and destroyed in fine after all interfaces have been stopped.

It's less important at the libvlc level with the current
mechanism as the window itself is expected to live longer.
Improving this will probably be a thing later.

>
> takeway:
> - vout_window_Close() is useless
> - vout_window_ReportClose() is key
>
> comments/critics (no animosity) welcomed

I hope my comments were correct regarding this. Don't
hesitate to PM me to say so if otherwise.

I think it can be solved with an added request like written
above. It is not the most elegant imho but it's quite close
to the mechanism I envision and would allow VLC to stop
crashing at exit with embedded windows. What do you think ?

Regards,
--
Alexandre Janniaux
Videolabs