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

[Alexandre Janniaux]

Hi,

To sum up the current API:

- enable puts resources at disposition to the core and makes
  the window visible.

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

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

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

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

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