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

[Romain Vimont]

Hi,

On Sat, Apr 17, 2021 at 01:59:39AM +0200, Pierre Ynard via vlc-devel wrote:
> > 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.
> 
> It does not imply that.

You're right. I dismissed other possibilities too quickly.

> It means preloading some SD modules. Which is
> not a meaningless concept, and that's one reason I don't think it should
> be removed.

I'm totally OK not to remove the option. It just remains to define what
to do ;)

> Apart of maintaining a single shared list, there are other ways to
> handle this:
> 
> 1/ Having the core initialization code load and take a reference on
> every passed SD module. Not terribly useful as this gives no way to
> access, browse and play whatever was discovered, but at least it allows
> running particular SD modules and making sure they never get unloaded.

Hmmm, yes, probably not very useful.

> 2/ Same as 1/ and also exposing the list of SD preloaded by the core, so
> interfaces concerned by SD can use them.

If it's not modifiable, I'm not fond of exposing a separate list of SD
opened by the core. If it's modifiable, it is a "shared list" similar to
what I described in my message.

> 3/ Having each interface concerned by SD parse, process and honor -S
> themselves.

That's a possibility. I wondered if we needed interface-specific
options, but indeed it can be the same option for everyone.

> > 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.
> 
> Great, indiscriminately launching queries to plenty of services, in
> particular third-party network ones, for literally just looking around
> the interface. Exactly what we need to generate distrust from users like
> me. That must scale well and be robust too, if one of the services is
> slow, returns thousands of entries, or is buggy or goes outdated and
> fails or crashes.

I described the current Qt behavior as an illustration of UI-specific
behavior (to explain my preference not to expose a shared list of
SD/media sources).

But we can discuss separately what SD should be enabled by default (if
any). I just tested on current master, in "Browse" (which shows local
devices and LAN devices), I have:
 - Applications (as a folder), containing each open app on the desktop,
   that I can "capture"
 - Desktop "screen://" (to capture the whole screen)
 - Built-in Audio Analog Stereo "pulse://alsa_input.pci-..."
 - Integrated Camera "v4l2:///dev/video0"

(It seems I have nothing discoverable on my LAN.)

I agree they should probably not all be enabled by default without
explicit activation.

In Discover/Services, there is a list of lua "playlists":
 - librivox
 - appletrailers
 - jamendo
 - etc.

Only the list of available media sources names is used (struct
vlc_media_source_meta). The SD are not open until explicitly requested
by the user (by double-clicking on Jamendo for example). So I think the
behavior is OK there.

> >  2. Keep the current behavior, and implement SD/media source
> >     management in rc/cli.
> 
> Don't tell me that the people who removed the SD from the playlist,
> where it was naturally accessible to every interface, intend that only
> bothering to implement the new API in Qt is fine and sufficient?...

AFAIK, rc/cli was the only remaining code using services discoveries
using the old playlist (e.g. calling the old
playlist_ServicesDiscoveryAdd()) that has not been rewritten.

> I think that SD browsing should be implemented not only in all
> interfaces that support SD loading (or supported it before that feature
> was removed because "not worth it"), but also in all interfaces that can
> browse and play media from the playlist, and could play SD items loaded
> with -S into the old playlist tree.
> 
> I guess you might find that maintaining in the core a tree of loaded
> SD items might indeed help with that, but I don't know why or how the
> playlist was overhauled, so I wouldn't be able to tell.

The old playlist handled too many different things.

We wanted a new playlist containing a simple list of items, acting
as a media provider for a player, without unrelated responsibilities.
Storing a tree of items discovered by services discoveries was one of
these unrelated responsibilities.

Now, services discoveries are handled separately, and items can be added
into a playlist to be played (the "tree" of discovered items is not
itself a playlist). By the way, handling discovered items as playlist
items caused unexpected behaviors: for example, if an item failed, the
playlist started to play the next discovered one.

Regards