[vlc-devel] [PATCH 5/5] libvlc: player: use vlc_player

[Rémi Denis-Courmont]

Le vendredi 10 mai 2019, 12:41:09 EEST Romain Vimont a écrit :
> On Thu, May 09, 2019 at 07:48:56PM +0300, Rémi Denis-Courmont wrote:
> > Le torstaina 9. toukokuuta 2019, 18.56.58 EEST Romain Vimont a écrit :
> > > @@ -340,16 +332,13 @@ int libvlc_audio_set_volume( libvlc_media_player_t
> > > *mp, int volume )
> > > ************************************************************************
> > > ***
> > > **/ int libvlc_audio_get_track_count( libvlc_media_player_t *p_mi )
> > > 
> > >  {
> > > 
> > > -    input_thread_t *p_input_thread = libvlc_get_input_thread( p_mi );
> > > -    int i_track_count;
> > > +    vlc_player_t *player = p_mi->player;
> > > +    vlc_player_Lock(player);
> > > 
> > > -    if( !p_input_thread )
> > > -        return -1;
> > > +    int ret = vlc_player_GetTrackCount(p_mi->player, AUDIO_ES);
> > > 
> > > -    i_track_count = var_CountChoices( p_input_thread, "audio-es" );
> > > -
> > > -    input_Release(p_input_thread);
> > > -    return i_track_count;
> > > +    vlc_player_Unlock(player);
> > > +    return ret;
> > > 
> > >  }
> > 
> > Application should (still) be allowed to call this, and most* other
> > functions further down the patch, from within event callbacks. But this
> > leads to deadlock or abort depending on build type.
> 
> Yes.
> 
> But we want to keep the libvlc API (which does not expose the lock to
> the client) unchanged, and we need to lock the player. So the lock must
> be recursive in some way:
>  1. always make the core vlc_player lock recursive;

As a clean slate design, it works. libpulse works that way.

But AFAICT, that won't be backward compatible. It still leads to a deadlock if 
the event is dispatched to another thread, which then calls into LibVLC.

If you want to go that "libpulse style" route, you should break compatibility 
explicitly, e.g. by ripping out the event manager and replacing with a simpler 
error-free callback setter.

-- 
Rémi Denis-Courmont