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

[erwan.tulou at gmail.com]

Hi,

    Interesting .. see comments inside the email.

On 03/12/2019 16:16, Alexandre Janniaux wrote:
> 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.

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)
> 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, ... ?)

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 
VLC_EGENERIC.

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 .. +
>> 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.

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.
>
>> 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel