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

[brezhoneg1 at yahoo.fr]

Hi,

On 03/12/2019 20:17, Alexandre Janniaux wrote:
> Hi,
>
> To sum up the current API:
>
> - enable puts resources at disposition to the core and makes
>    the window visible.

      vlc core asks for a valid platform-dependent window ID to be used 
by a display. That' all.

      Whether it is visible or not is up to the GUI. For instance, old 
skins with no provision for a video control still accept the call, 
provide a valid window id but never show it in normal mode, just in 
fullscreen mode.

      Once the call is accepted, the vout window module must NOT destroy 
the window id or let the Window Manager destroy it  (that can happen and 
I'm not sure all vlc plugins manage the right messages to prevent silent 
destroying of windows as Window Managers can be tricky at times. There 
are many Window Managers out there, and they may not all behave nicely)

>
> - disable remove those resources from the core. Nobody should
>    use them afterwards, but it's only part of the API contract
>    and the window is hidden.

      vlc core just tells the vout window module that he won't use the 
window ID anymore for the time being. That's all.

      The GUI can do whatever he wants. It can hide the window or 
destroy it or just leave the window visible with the last picture. 
That's up to the GUI designer (or skin designer) to do whatever they 
want. They just know it won't be refreshed with new content from the vlc 
core. Most often, it just hides the window, ready to show it again when 
re-enabled, or just do nothing (a widget video transitions from the last 
picture of the previous media to the first picture of the next media 
without even hide and show.).

      In this disabled state, destroying the window is allowed.

>
> - vout_window_Report* are functions called by the module
>    implementation to notify the core of window events. Because
>    those events are coming from the modules, they are expected
>    to come translated from the underlying window system.

As the name implies, the vout window module reports to vlc core user or 
system interactions (go fullscreen, resizing, hotkey pressed, mouse 
moved or clicked or double clicked...).

In the case of GUI with embedded video, something worth noting is that 
there are two channels : the GUI can either directly call the vlc_player 
api to perform an action or go via the vout window module and report the 
action (for instance, go fullscreen, hotkey pressed, ....). That's also 
why it wouldn't be difficult to just issue a call : unselect Video ES, 
uncheck audio visualizer without reporting Close. But, I'm not sure that 
is a very orthodox way to do things and it may be tricky if one has both 
a video ES and an audio visualizer (they are difficult to tell apart). 
You can view Interface with embedded video for the better (two ways to 
do several things) or the worse (tricky to untangle things when it comes 
to stopping)

>
> - vout_window_* (sorry, confusing naming) are requests from
>    the window clients (likely the core, but also modules like
>    video splitter) to the module implementation, with an
>    intended effect on the underlying window.
Not very familiar with the video splitter. I guess video splitter and 
audio visualization are just basic uses of vout_Create. They don't use 
the ressource concept in order to  reuse as much as possible already 
existing window id, the purpose being to provide transitions as smooth 
as possible. But I may be mistaken, especially regarding vlc 4.0
>
> - vout_window has a `ops` field containing the module
>    implementation callbacks to handle each request.
>
> - vout_window has a `owner` field containing the core
>    implementation callbacks to handle window notifications
>    and are called when `vout_window_Reportxxx` is called.
>
> - window clients create a window with a owner to handle
>    the different notifications and implement their behaviour.
>    the msg_ log `window closed` is one of them.
Sounds right, though I haven't checked the exact names lately.
>
> On Tue, Dec 03, 2019 at 06:11:02PM +0100, erwan.tulou at gmail.com wrote:
>> 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.
> What I was trying to say is that `RequestClose` doesn't mean
> `kill the window` but `someone in the window system asked to
> close the window, what should we do?` so imho what you
> suggest swap the semantic of the owner and the window
> implementation regarding that.
>
> If we are to implement a request to release the ownership of
> the window from window clients, it would be called from the
> core and would trigger directly the owner, so there is no
> need for an equivalent `ReportXxxx` as it won't even go
> through the module implementation itself.
>
>>>> 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 ...
>>
> There is a need for the function but it has never been
> implemented yet because it needs refactoring quite a lot of
> code.
>
> For example, with no-embedded window, you might want to stop
> the playback if the window is asked to be closed through
> `ReportClose` or conversely prevent closing window and
> instead expect stopping the input. The vout_window is a low
> level API with regard to window manipulation.
>
>>> 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.
> If vout_window_Enable fails, the core cannot start playback
> and the vout_Request fails. But if it fails it mostly means
> that resources have been killed one way or the other, like
> display connection was killed or the objects were killed by
> someone else outside of the app.
>
> It propagates down to the decoder, which signals that
> updating the format or output video context failed.
>
> So I don't think there is such to recover from this currently
> but the vout_thread might be removed from the input resource,
> that's a nice point. I didn't check the resource extensively
> though.
>
>>
>>> 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.
> Yes, it's an implementation detail for the window module.
>
>>>> 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) ?
> Actually no, I meant that instead of changing the semantic
> of vout_window_ReportClose, you could implement:
>
> + a vout_window_ReleaseClient() request
>
> + a release_client callback in the owner
>
> + the client-side (owner-side) for this callback in
>    vout_thread and visualization to make the vout_window
>    module safely disable.
>
>>>> 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
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel