[vlc-devel] [vlc-commits] skins2: ensure playlist gets stopped before terminating vlc.
erwan.tulou at gmail.com
erwan.tulou at gmail.com
Tue Dec 3 18:11:02 CET 2019
Interesting .. see comments inside the email.
On 03/12/2019 16:16, Alexandre Janniaux wrote:
> 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
> 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
>> 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
The behaviour is indeed to be refined as long as it ends up being in a
disabled state. Here, the disabled state officially tells the interface
that the windows id is not longer used by a display.
For Remi, this vout_window_ReportClose() was not meant to expect a
disabled state. It was simply reporting and it was up to the core to
decide whatever action to take (not necessarily go disabled).
If so, we need a new vout_window_RequestClose() that guarantees a
vout_window_disable() as a result.
>> 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.
Well, one can imagine vout_window_Close() meaning to truly close the
window id as opposed to vout_window_disable() meaning to logically close
it (implementation can hide it but keep it alive for a nicer transition
between media). For now the vout_window_close() is not used except
before unloading the module. That's why I said it is useless. But I do
agree it brings its own concept. Just is there a need for the function
? maybe ...
> 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)
>> 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, ... ?)
I forgot to mention a key thing.
vout_window_Enable() can fail. This is key to what I envision.
- interface (with embedded video) terminates
- its owned vout windows move to disabled states.
- the windows id are destroyed (actually the whole GUI is destroyed)
- as a vout window provider, the module still exists, still has to
service requests from the core such as a new vout_window_enable()
- if such a vout_window_enable() occurs, the vout window module returns
This seems already managed in the code. I am wondering what the core
does in such a situation. I would imagine vlc core acknowledges this
vout window module cannot be re-enabled any more, unload the module, and
try a new one. But, I did not go that far in the code to see what kind
of recovery vlc core does offer on such an event.
> 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.
Yes, indeed . Actually, most of the time, it won't even close the
window, just hide it for a nicer transition disable-enable. But, it
knows it can close it safely, should termination occur. If so, next
vout_window_enable() will get VLC_EGENERIC as explained just before.
>> In short, at the core level, what is needed is:
>> - implementing vout_window_ReportClose() (today it ends up in msg_Err .. +
>> - 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.
no sure I get everything here. Does that relate to vlc core needing a
new vout window, if the old one is no longer responsive
(vout_window_enabled() returning VLC_EGENERIC) ?
>> 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.
>> - 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 ?
> Alexandre Janniaux
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
More information about the vlc-devel