[vlc-devel] [RFC] Moving services discovery out of the playlist

[Romain Vimont]

On Sun, Jun 10, 2018 at 02:59:54PM +0300, Rémi Denis-Courmont wrote:
> Le sunnuntaina 10. kesäkuuta 2018, 14.28.59 EEST Romain Vimont a écrit :
> > Hi Rémi,
> > 
> > Thank you for your feedbacks.
> > 
> > On Fri, Jun 08, 2018 at 07:33:32PM +0300, Rémi Denis-Courmont wrote:
> > > Le perjantaina 8. kesäkuuta 2018, 18.05.58 EEST Romain Vimont a écrit :
> > > > Hi,
> > > > 
> > > > We would like to move services discovery out of the playlist, I explain
> > > > why and detail a proposal in this RFC:
> > > > <https://github.com/rom1v/vlc/blob/sd/rfc.md>
> > > > 
> > > > I would appreciate feedbacks ;-)
> > > 
> > > playlist_IsServicesDiscoveryLoaded() is deprecated because it's broken by
> > > design, and nobody proposed a sane alternative. Anything that tries to
> > > imitate is thus presumed broken by design too. And that seems to include
> > > media_browser_GetMediaSource().
> > 
> > AFAIU, playlist_IsServicesDiscoveryLoaded() is broken because it's
> > inherently racy.
> 
> The race is unavoidable if you want to show what SD are active. That's not 
> necessarily a problem.

In Qt at least, it was used only to decide whether
playlist_ServicesDiscoveryAdd() had to be called:

<https://git.videolan.org/?p=vlc.git;a=blob;f=modules/gui/qt/components/playlist/selector.cpp;h=7c95e6c640c2ca9b5a265c63e5f7e23b525de576;hb=HEAD#l361>

> > I think that media_browser_GetMediaSource() is more equivalent to
> > playlist_ServicesDiscoveryAdd(), except that it creates/loads the SD
> > only if it was not already loaded, without race conditions (unless
> > there is a bug). Is something wrong with this?
> 
> There are no ways to identify an SD uniquely. A string is definitely no such 
> way.

That's interesting. Indeed, my proposal assumes that it makes sense to
use the name/chain as an identifier to retrieve the expected service
discovery (like in the current implementation). I think we need a way to
identify/select a service discovery, anyway, don't we? Can we avoid it?

My goal here is just to move the SD stuff out of the playlist (I want to
implement things incrementally), so I try to keep the same behavior, but
if there is a better way, it is probably better to get a design that
solve both problems.

> That function worked before SD had a config chain, and before Lua SD was 
> added. It has never stopped working properly since then, and mostly probably 
> never will work properly again.

:(

> 
> > > Also I don't see the point in reference counting. SD instances are not
> > > reentrant, so I don't see what you can do with a reference to one.
> > 
> > Several concurrent clients (interfaces) may request a SD, in that case
> > GetMediaSource() returns the same media_source_t instance (containing
> > the same media tree).
> 
> > Reference counting is used to release the instance (and close the SD)
> > when no more clients use it (in C++, a shared_ptr could have been used
> > instead).
> 
> So how does that work with stateless interfaces such as HTTP that have nowhere 
> to keep the references?

I would say that either the HTTP server keeps the media_source_t
instance, or it just releases it on every HTTP request.

Managing a refcount of media sources is just an alternative to explicit
calls to AddServiceDiscovery() and RemoveServiceDiscovery() (it just
makes the media source sharable).