[vlc-devel] [PATCH] sd: remove -S/--services-discovery option

[Romain Vimont]

On Thu, Apr 15, 2021 at 03:45:23PM +0200, Alexandre Janniaux wrote:
> Hi,
> 
> I'm against the removal of this option, since the Qt interface
> is not supposed to be the only way to test this option, and
> starting every service discovery on start when you never use
> them is quite wasteful. Said otherwise, it has been made
> meaningless but is not supposed to be useless.

Currently, the media source API allows to:

 1. list the available media sources (just some meta [1])
 2. load a media source by its name [2] (available in the meta, e.g.
    `mtp`, `pulse`, `lua{sd='jamendo'}`…)
 3. navigate/listen to the media tree produced by the media source (out
    of scope for this discussion)

Loading a media source returns a refcounted instance: the SD will only
be opened on the first client and closed on the last client. The purpose
is to let several interfaces open their own services discoveries without
opening several time the same SD (which would be a waste of resources,
including network requests, etc.).

In that model, the list of "open" media sources is the union of the
media sources open by all the clients. It is not explicitly
stored/exposed in the core, like it was with the old playlist. In other
words, each client (interface) loads its own services discoveries as
needed, there is no "shared list". This allows a client to "close"
(release) a service discovery without removing it for other clients
(which may use it independently).

The option -S/--services-discoveries implies that we maintain a single
shared list of opened media sources in the core, used by all the
interfaces, in which we will append the SD on start.

Note that such a shared list, to be displayed in UI, would have the same
constraints of the new playlist regarding synchronization:
 - access from any thread
 - duplication to solve desynchronization from UI thread
 - apply the notified changes "in order" from the UI thread
 - resolve "outdated" requests (to add or remove an item)
 - …

Semantically, do we want the list of open SD/media sources to be shared
across all interfaces?

The interfaces might behave differently regarding SD. For example, Qt
opens all SD for a specific category when the corresponding UI view is
selected, and close them when the view is destroyed. As a consequence,
the open media sources would very tied to the Qt view state. Should that
list be reflected in other interfaces? Btw, in that case, what should we
do when Qt closes the SD but another interface is displaying it?

What should we do?
 1. Expose a single list of open media sources in the core, shared by
    all interfaces, so that we can keep the -S option from VLC3?
 2. Keep the current behavior, and implement SD/media source management
    in rc/cli. But then, what would be the meaning of -S? Should we add
    interface-specific or options to replace it?
 3. Another solution for #22687?

Regards

[1]: https://code.videolan.org/videolan/vlc/-/blob/a24a9b28992c66731354c2c8448e31f5f4ed37ba/include/vlc_media_source.h#L260-287
[2]: https://code.videolan.org/videolan/vlc/-/blob/a24a9b28992c66731354c2c8448e31f5f4ed37ba/include/vlc_media_source.h#L253-255

> 
> But anyway, regardless of whether we want to remove it or not,
> removing an option that was used on commandline will break
> commandline. The way to go is using the add_obsolete_*
> variants.
> 
> Regards,
> --
> Alexandre Janniaux
> Videolabs
> 
> On Thu, Apr 15, 2021 at 03:22:48PM +0200, Romain Vimont wrote:
> > In the old playlist, services discoveries were added as special nodes,
> > child of the root playlist node. The option -S/--services-discovery
> > gave the possibility to add some of these services discoveries nodes to
> > the playlist on start.
> >
> > Now, all the media sources are available via the media source
> > API. There are listed in the Qt views (depending on their category).
> >
> > As a consequence, this option is now meaningless. Remove it.
> >
> > Fixes #24592
> > ---
> >  src/libvlc-module.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/src/libvlc-module.c b/src/libvlc-module.c
> > index 8050b2eea1..a569f41b62 100644
> > --- a/src/libvlc-module.c
> > +++ b/src/libvlc-module.c
> > @@ -2206,10 +2206,6 @@ vlc_module_begin ()
> >                "Flatten files listed by extractors (archive)", NULL, true )
> >          change_volatile()
> >
> > -    set_subcategory( SUBCAT_PLAYLIST_SD )
> > -    add_string( "services-discovery", "", SD_TEXT, SD_LONGTEXT, true )
> > -        change_short('S')
> > -
> >  /* Interface options */
> >      set_category( CAT_INTERFACE )
> >      set_subcategory( SUBCAT_INTERFACE_GENERAL )
> > --
> > 2.31.0
> >
> > _______________________________________________
> > 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