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

erwan.tulou at gmail.com erwan.tulou at gmail.com
Mon Dec 2 13:11:28 CET 2019 

Hi,

On 02/12/2019 09:20, Alexandre Janniaux wrote:
> Hi,
>
> Actually stopping the playlist wont kill the video output
> module, only killing the input resource, meaning releasing
> the vlc_player will do.
>
> However, player and playlist might be used by multiple
> control interface and this problem is only a concern for
> the one providing the vout_window. Hence I believe it must
> be solved by the mechanism providing the window instead of
> relying on a much wider API like playlist and player.

I fully agree with that. And actually, the work is already partially 
done. A 'vout_window_ReportClose' is available to let the vout know that 
the windows ids won't be available any more. For now the function is not 
implemented

static void vout_display_window_CloseNotify(vout_window_t *window)
229 
<http://git.videolan.org/?p=vlc.git;a=blob;f=src/video_output/window.c;h=16ebf9ef5a2252b896dd5edddd66fc2bc3465f56;hb=HEAD#l229> 
{
230 
<http://git.videolan.org/?p=vlc.git;a=blob;f=src/video_output/window.c;h=16ebf9ef5a2252b896dd5edddd66fc2bc3465f56;hb=HEAD#l230> 
     /* TODO: Nowhere to dispatch to currently.
231 
<http://git.videolan.org/?p=vlc.git;a=blob;f=src/video_output/window.c;h=16ebf9ef5a2252b896dd5edddd66fc2bc3465f56;hb=HEAD#l231> 
      * Needs callback to ES output to deselect ES? */
232 
<http://git.videolan.org/?p=vlc.git;a=blob;f=src/video_output/window.c;h=16ebf9ef5a2252b896dd5edddd66fc2bc3465f56;hb=HEAD#l232> 
     msg_Err(window, "window closed");
233 
<http://git.videolan.org/?p=vlc.git;a=blob;f=src/video_output/window.c;h=16ebf9ef5a2252b896dd5edddd66fc2bc3465f56;hb=HEAD#l233> 
}

Implementing this function would kill two birds with one stone. When 
terminating vlc, GUI modules with embedded videos can use it to properly 
terminate vout. But, more generally, this closing of windows can occur 
at any time, and it is wise to also properly terminate vout if this occurs.

When saying 'terminate vout', it means terminate and desallocate the 
resource. This is a case where the window id must not be retained for a 
possible future use. In other words, as the result of issueing  
vout_window_ReportClose(), the response must be vout_window_Close().
Also, this is not only related to the player. It must be a more generic 
thing because vout can also be an audio visualization or anything else. 
Maybe the caller of vout_Create() has to provide a callback to deal with 
this event.

>
> I wrote part of it that I shared as a github branch some
> weeks ago, but it is not complete yet. I had also explored
> alternative solution like a vlc_player_disable-like
> mechanism (that I called vlc_player_ResetResource iirc) but
> I find it not very elegant.

Yet, as of today, when terminating vlc, it is not clear who is 
responsible for just properly stopping the playlist. I realized that 
deleting the playlist doesn't stop the decoders for instance. The Qt 
interface stops the playlist in MainInterface::closeEvent() 
(modules/gui/qt/main_interface.cpp) which is a weird place. For lack of 
a better place, the patch I wrote was just mimicking Qt because stopping 
the playlist has to be done somewhere.

I actually think it is the libvlc overall responsability to stop the 
playlist since starting the playlist can be done very early in the 
process without any GUI already set up.

this vlc_playlist_disable would be very simple, lock the playlist, stop 
it, unlock it + an enabled flag to prevent any new starting. And this is 
just triggering a stop. The actual stop will happen much later when the 
vout are indeed released by a proper desallocation mechanism between the 
GUI and the vout core and all the listeners on the player/vout are 
released by the GUI.


> Is skins2 interface embedded work using interface_widget
> from the Qt interface?

No, skins2 doesn't rely on Qt for vout windows. The Qt thing is only 
dialog boxes, menus and hotkeys management (including key accelerators 
from Qt). Yet, the full Qt Playlist and Player controllers are set up 
which means that skins2 is highly dependent on Qt indirectly.

Rgds
Erwan

>   
> Regards,
> --
> Alexandre Janniaux
> Videolabs
>
> On Fri, Nov 29, 2019 at 11:13:21PM +0100, erwan.tulou at gmail.com wrote:
>> On 29/11/2019 20:09, Rémi Denis-Courmont wrote:
>>> Le perjantaina 29. marraskuuta 2019, 20.43.29 EET Erwan Tulou a écrit :
>>>> vlc | branch: master | Erwan Tulou <erwan10 at videolan.org> | Fri Nov 29
>>>> 19:32:25 2019 +0100| [0594abc06cb1a57548a7c3bf977e3071e64ef022] |
>>>> committer: Erwan Tulou
>>>>
>>>> skins2: ensure playlist gets stopped before terminating vlc.
>>>>
>>>>> http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=0594abc06cb1a57548a
>>>>> 7c3bf977e3071e64ef022
>>>> ---
>>>>
>>>>    modules/gui/skins2/src/skin_main.cpp | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/modules/gui/skins2/src/skin_main.cpp
>>>> b/modules/gui/skins2/src/skin_main.cpp index 087dd82cc0..d52e904d8d 100644
>>>> --- a/modules/gui/skins2/src/skin_main.cpp
>>>> +++ b/modules/gui/skins2/src/skin_main.cpp
>>>> @@ -143,6 +143,12 @@ static void Close( vlc_object_t *p_this )
>>>>
>>>>        msg_Dbg( p_intf, "closing skins2 module" );
>>>>
>>>> +    // ensure the playlist is stopped
>>>> +    vlc_playlist_t *playlist = vlc_intf_GetMainPlaylist( p_intf );
>>>> +    vlc_playlist_Lock( playlist );
>>>> +    vlc_playlist_Stop ( playlist );
>>>> +    vlc_playlist_Unlock( playlist );
>>> This does not guarantee that the playlist is stopped, only that it was stopped
>>> until the Unlock. This whole patch does not make sense to me.
>> Yes, this is a lame way to work out a problem and it actually only solve one
>> part of the problem. (so far, the Qt interface does not offer a better
>> solution in that matter).
>>
>> Ideally, once libvlc_Quit() is issued, the right order of disabling/removing
>> thing seems the following :
>>
>> - disable the playlist (which means : stop all inputs, free all resources
>> such as vout and make sure it cannot start again)
>> - stop and desallocate the interfaces
>> - desallocate the playlist
>>
>> In particular, it is important that all vouts be freed __before__ the
>> interfaces are gone, because the window ids have to remain valid (embedded
>> video) as long as the vout are alive.
>>
>> maybe coding a vlc_playlist_disable() to be called first thing is the
>> solution ... any suggestions ?
>>
>> Rgds
>> Erwan
>>
>>
>>
>>
>>
>> _______________________________________________
>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20191202/d827c47e/attachment.html>

More information about the vlc-devel mailing list