[vlc-devel] [PATCH 1/3] upnp: add and use a callback listener interface
Shaleen Jain
shaleen at jain.sh
Wed Jun 27 13:25:52 CEST 2018
On Wed, 2018-06-27 at 03:47 -0700, Hugo Beauzée-Luyssen wrote:
> Hi,
>
> On Mon, Jun 25, 2018, at 6:49 AM, Shaleen Jain wrote:
> > Introduce a Listener interface to
> > UpnpInstanceWrapper that can be used
> > by modules to get UpnpEvent callbacks
> > from libupnp, decoupling UpnpInstanceWrapper
> > from any specific module or their members.
> > ---
> > modules/services_discovery/upnp.cpp | 127 +++++++++++-------------
> > ----
> > modules/services_discovery/upnp.hpp | 44 +++++++---
> > 2 files changed, 79 insertions(+), 92 deletions(-)
> >
> > diff --git a/modules/services_discovery/upnp.cpp b/modules/
> > services_discovery/upnp.cpp
> > index 7455ceb299..fc81a847a2 100644
> > --- a/modules/services_discovery/upnp.cpp
> > +++ b/modules/services_discovery/upnp.cpp
> > @@ -1,13 +1,14 @@
> > /
> > *******************************************************************
> > **********
> > * upnp.cpp : UPnP discovery module (libupnp)
> >
> > *******************************************************************
> > **********
> > - * Copyright (C) 2004-2016 VLC authors and VideoLAN
> > + * Copyright (C) 2004-2018 VLC authors and VideoLAN
> > * $Id$
> > *
> > * Authors: Rémi Denis-Courmont <rem # videolan.org> (original
> > plugin)
> > * Christian Henz <henz # c-lab.de>
> > * Mirsal Ennaime <mirsal dot ennaime at gmail dot com>
> > * Hugo Beauzée-Luyssen <hugo at beauzee.fr>
> > + * Shaleen Jain <shaleen at jain.sh>
> > *
> > * This program is free software; you can redistribute it and/or
> > modify
> > it
> > * under the terms of the GNU Lesser General Public License as
> > published by
> > @@ -84,6 +85,7 @@ static const char *const
> > ppsz_readible_satip_channel_lists[] = {
> > struct services_discovery_sys_t
> > {
> > UpnpInstanceWrapper* p_upnp;
> > + std::shared_ptr<SD::MediaServerList> p_server_list;
> > vlc_thread_t thread;
> > };
> >
> > @@ -93,8 +95,8 @@ struct access_sys_t
> > };
> >
> > UpnpInstanceWrapper* UpnpInstanceWrapper::s_instance;
> > +UpnpInstanceWrapper::Listeners UpnpInstanceWrapper::s_listeners;
> > vlc_mutex_t UpnpInstanceWrapper::s_lock = VLC_STATIC_MUTEX;
> > -SD::MediaServerList *UpnpInstanceWrapper::p_server_list = NULL;
> >
> > /*
> > * VLC callback prototypes
> > @@ -227,7 +229,7 @@ SearchThread( void *p_data )
> >
> > /* Search for media servers */
> > int i_res = UpnpSearchAsync( p_sys->p_upnp->handle(), 5,
> > - MEDIA_SERVER_DEVICE_TYPE, p_sys->p_upnp );
> > + MEDIA_SERVER_DEVICE_TYPE, MEDIA_SERVER_DEVICE_TYPE );
> > if( i_res != UPNP_E_SUCCESS )
> > {
> > msg_Err( p_sd, "Error sending search request: %s",
> > UpnpGetErrorMessage( i_res ) );
> > @@ -236,7 +238,7 @@ SearchThread( void *p_data )
> >
> > /* Search for Sat Ip servers*/
> > i_res = UpnpSearchAsync( p_sys->p_upnp->handle(), 5,
> > - SATIP_SERVER_DEVICE_TYPE, p_sys->p_upnp );
> > + SATIP_SERVER_DEVICE_TYPE, MEDIA_SERVER_DEVICE_TYPE );
> > if( i_res != UPNP_E_SUCCESS )
> > msg_Err( p_sd, "Error sending search request: %s",
> > UpnpGetErrorMessage( i_res ) );
> > return NULL;
> > @@ -256,20 +258,29 @@ static int Open( vlc_object_t *p_this )
> >
> > p_sd->description = _("Universal Plug'n'Play");
> >
> > - p_sys->p_upnp = UpnpInstanceWrapper::get( p_this, p_sd );
> > + p_sys->p_upnp = UpnpInstanceWrapper::get( p_this );
> > if ( !p_sys->p_upnp )
> > {
> > free(p_sys);
> > return VLC_EGENERIC;
> > }
> >
> > + p_sys->p_server_list =
> > std::make_shared<SD::MediaServerList>( p_sd );
> > + if ( unlikely( p_sys->p_server_list == NULL ) )
>
> make_shared will throw a std::bad_alloc in case of allocation
> failure, but it won't return NULL/nullptr
Yes, I missed that. I'll fix it.
>
> > + {
> > + msg_Err( p_sd, "Failed to create a MediaServerList");
> > + return VLC_EGENERIC;
> > + }
> > + p_sys->p_upnp->addListener( p_sys->p_server_list );
> > +
> > /* XXX: Contrary to what the libupnp doc states,
> > UpnpSearchAsync is
> > * blocking (select() and send() are called). Therefore, Call
> > * UpnpSearchAsync from an other thread. */
> > if ( vlc_clone( &p_sys->thread, SearchThread, p_this,
> > VLC_THREAD_PRIORITY_LOW ) )
> > {
> > - p_sys->p_upnp->release( true );
> > + p_sys->p_upnp->removeListener( p_sys->p_server_list );
> > + p_sys->p_upnp->release();
> > free(p_sys);
> > return VLC_EGENERIC;
> > }
> > @@ -286,7 +297,8 @@ static void Close( vlc_object_t *p_this )
> > services_discovery_sys_t *p_sys =
> > reinterpret_cast<services_discovery_sys_t *>( p_sd->p_sys );
> >
> > vlc_join( p_sys->thread, NULL );
> > - p_sys->p_upnp->release( true );
> > + p_sys->p_upnp->removeListener( p_sys->p_server_list );
> > + p_sys->p_upnp->release();
> > free( p_sys );
> > }
> >
> > @@ -702,8 +714,11 @@ void MediaServerList::removeServer( const
> > std::string& udn )
> > /*
> > * Handles servers listing UPnP events
> > */
> > -int MediaServerList::Callback( Upnp_EventType event_type,
> > UpnpEventPtr
> > p_event )
> > +int MediaServerList::onEvent( Upnp_EventType event_type,
> > UpnpEventPtr
> > p_event, void* p_user_data )
> > {
> > + if (p_user_data != MEDIA_SERVER_DEVICE_TYPE)
> > + return 0;
> > +
> > switch( event_type )
> > {
> > case UPNP_DISCOVERY_ADVERTISEMENT_ALIVE:
> > @@ -716,23 +731,14 @@ int MediaServerList::Callback(
> > Upnp_EventType
> > event_type, UpnpEventPtr p_event )
> > int i_res;
> > i_res =
> > UpnpDownloadXmlDoc( UpnpDiscovery_get_Location_cstr( p_discovery
> > ),
> > &p_description_doc );
> >
> > - MediaServerList *self =
> > UpnpInstanceWrapper::lockMediaServerList();
> > - if ( !self )
> > - {
> > - UpnpInstanceWrapper::unlockMediaServerList();
> > - return UPNP_E_CANCELED;
> > - }
> > -
> > if ( i_res != UPNP_E_SUCCESS )
> > {
> > - msg_Warn( self->m_sd, "Could not download device
> > description! "
> > + msg_Warn( m_sd, "Could not download device
> > description! "
> > "Fetching data from %s failed: %s",
> >
> > UpnpDiscovery_get_Location_cstr( p_discovery ),
> > UpnpGetErrorMessage( i_res ) );
> > - UpnpInstanceWrapper::unlockMediaServerList();
> > return i_res;
> > }
> > - self->parseNewServer( p_description_doc,
> > UpnpDiscovery_get_Location_cstr( p_discovery ) );
> > - UpnpInstanceWrapper::unlockMediaServerList();
> > + parseNewServer( p_description_doc,
> > UpnpDiscovery_get_Location_cstr( p_discovery ) );
> > ixmlDocument_free( p_description_doc );
> > }
> > break;
> > @@ -740,29 +746,19 @@ int MediaServerList::Callback(
> > Upnp_EventType
> > event_type, UpnpEventPtr p_event )
> > case UPNP_DISCOVERY_ADVERTISEMENT_BYEBYE:
> > {
> > const UpnpDiscovery* p_discovery = ( const
> > UpnpDiscovery* )p_event;
> > -
> > - MediaServerList *self =
> > UpnpInstanceWrapper::lockMediaServerList();
> > - if ( self )
> > - self-
> > > removeServer( UpnpDiscovery_get_DeviceID_cstr( p_discovery ) );
> >
> > - UpnpInstanceWrapper::unlockMediaServerList();
> > + removeServer( UpnpDiscovery_get_DeviceID_cstr( p_discovery
> > ) );
> > }
> > break;
> >
> > case UPNP_EVENT_SUBSCRIBE_COMPLETE:
> > {
> > - MediaServerList *self =
> > UpnpInstanceWrapper::lockMediaServerList();
> > - if ( self )
> > - msg_Warn( self->m_sd, "subscription complete" );
> > - UpnpInstanceWrapper::unlockMediaServerList();
> > + msg_Warn( m_sd, "subscription complete" );
> > }
> > break;
> >
> > case UPNP_DISCOVERY_SEARCH_TIMEOUT:
> > {
> > - MediaServerList *self =
> > UpnpInstanceWrapper::lockMediaServerList();
> > - if ( self )
> > - msg_Warn( self->m_sd, "search timeout" );
> > - UpnpInstanceWrapper::unlockMediaServerList();
> > + msg_Warn( m_sd, "search timeout" );
> > }
> > break;
> >
> > @@ -774,10 +770,7 @@ int MediaServerList::Callback( Upnp_EventType
> > event_type, UpnpEventPtr p_event )
> >
> > default:
> > {
> > - MediaServerList *self =
> > UpnpInstanceWrapper::lockMediaServerList();
> > - if ( self )
> > - msg_Err( self->m_sd, "Unhandled event, please report
> > ( type=%d )", event_type );
> > - UpnpInstanceWrapper::unlockMediaServerList();
> > + msg_Err( m_sd, "Unhandled event, please report ( type=%d
> > )",
> > event_type );
> > }
> > break;
> > }
> > @@ -1293,7 +1286,7 @@ static int Open( vlc_object_t *p_this )
> > return VLC_ENOMEM;
> >
> > p_access->p_sys = p_sys;
> > - p_sys->p_upnp = UpnpInstanceWrapper::get( p_this, NULL );
> > + p_sys->p_upnp = UpnpInstanceWrapper::get( p_this );
> > if ( !p_sys->p_upnp )
> > {
> > delete p_sys;
> > @@ -1311,7 +1304,7 @@ static void Close( vlc_object_t* p_this )
> > stream_t* p_access = (stream_t*)p_this;
> > access_sys_t *sys = (access_sys_t *)p_access->p_sys;
> >
> > - sys->p_upnp->release( false );
> > + sys->p_upnp->release();
> > delete sys;
> > }
> >
> > @@ -1537,26 +1530,14 @@ static char *getIpv4ForMulticast()
> >
> > #endif /* _WIN32 */
> >
> > -UpnpInstanceWrapper *UpnpInstanceWrapper::get(vlc_object_t
> > *p_obj,
> > services_discovery_t *p_sd)
> > +UpnpInstanceWrapper *UpnpInstanceWrapper::get(vlc_object_t *p_obj)
> > {
> > - SD::MediaServerList *p_server_list = NULL;
> > - if (p_sd)
> > - {
> > - p_server_list = new(std::nothrow) SD::MediaServerList(
> > p_sd );
> > - if ( unlikely( p_server_list == NULL ) )
> > - {
> > - msg_Err( p_sd, "Failed to create a MediaServerList");
> > - return NULL;
> > - }
> > - }
> > -
> > vlc_mutex_locker lock( &s_lock );
> > if ( s_instance == NULL )
> > {
> > UpnpInstanceWrapper* instance = new(std::nothrow)
> > UpnpInstanceWrapper;
> > if ( unlikely( !instance ) )
> > {
> > - delete p_server_list;
> > return NULL;
> > }
> >
> > @@ -1577,7 +1558,6 @@ UpnpInstanceWrapper
> > *UpnpInstanceWrapper::get(vlc_object_t *p_obj, services_disc
> > {
> > msg_Err( p_obj, "Initialization failed: %s",
> > UpnpGetErrorMessage( i_res ) );
> > delete instance;
> > - delete p_server_list;
> > return NULL;
> > }
> >
> > @@ -1589,7 +1569,6 @@ UpnpInstanceWrapper
> > *UpnpInstanceWrapper::get(vlc_object_t *p_obj, services_disc
> > {
> > msg_Err( p_obj, "Client registration failed: %s",
> > UpnpGetErrorMessage( i_res ) );
> > delete instance;
> > - delete p_server_list;
> > return NULL;
> > }
> >
> > @@ -1601,30 +1580,18 @@ UpnpInstanceWrapper
> > *UpnpInstanceWrapper::get(vlc_object_t *p_obj, services_disc
> > msg_Err( p_obj, "Failed to set maximum content length:
> > %s",
> > UpnpGetErrorMessage( i_res ));
> > delete instance;
> > - delete p_server_list;
> > return NULL;
> > }
> > s_instance = instance;
> > }
> > s_instance->m_refcount++;
> > - // This assumes a single UPNP SD instance
> > - if (p_server_list != NULL)
> > - {
> > - assert(!UpnpInstanceWrapper::p_server_list);
> > - UpnpInstanceWrapper::p_server_list = p_server_list;
> > - }
> > return s_instance;
> > }
> >
> > -void UpnpInstanceWrapper::release(bool isSd)
> > +void UpnpInstanceWrapper::release()
> > {
> > UpnpInstanceWrapper *p_delete = NULL;
> > vlc_mutex_lock( &s_lock );
> > - if ( isSd )
> > - {
> > - delete UpnpInstanceWrapper::p_server_list;
> > - UpnpInstanceWrapper::p_server_list = NULL;
> > - }
> > if (--s_instance->m_refcount == 0)
> > {
> > p_delete = s_instance;
> > @@ -1641,26 +1608,30 @@ UpnpClient_Handle
> > UpnpInstanceWrapper::handle() const
> >
> > int UpnpInstanceWrapper::Callback(Upnp_EventType event_type,
> > UpnpEventPtr p_event, void *p_user_data)
> > {
> > - VLC_UNUSED(p_user_data);
> > - vlc_mutex_lock( &s_lock );
> > - if ( !UpnpInstanceWrapper::p_server_list )
> > + for (Listeners::iterator iter = s_listeners.begin(); iter !=
> > s_listeners.end(); ++iter)
> > {
> > - vlc_mutex_unlock( &s_lock );
> > - /* no MediaServerList available (anymore), do nothing */
> > - return 0;
> > + (*iter)->onEvent(event_type, p_event, p_user_data);
> > }
> > - vlc_mutex_unlock( &s_lock );
> > - SD::MediaServerList::Callback( event_type, p_event );
> > +
> > return 0;
> > }
> >
> > -SD::MediaServerList *UpnpInstanceWrapper::lockMediaServerList()
> > +void UpnpInstanceWrapper::addListener(ListenerPtr listener)
> > {
> > - vlc_mutex_lock( &s_lock ); /* do not allow deleting the
> > p_server_list while using it */
> > - return UpnpInstanceWrapper::p_server_list;
> > + vlc_mutex_lock( &s_lock );
> > + if ( std::find( s_listeners.begin(), s_listeners.end(),
> > listener) !
> > = s_listeners.end() )
> > + return;
> > + s_listeners.push_back( std::move(listener) );
> > + vlc_mutex_unlock( &s_lock );
> > }
> >
> > -void UpnpInstanceWrapper::unlockMediaServerList()
> > +void UpnpInstanceWrapper::removeListener(ListenerPtr listener)
> > {
> > + vlc_mutex_lock( &s_lock );
> > + Listeners::iterator iter = std::find( s_listeners.begin(),
> > s_listeners.end(), listener );
> > + if ( iter == s_listeners.end() )
> > + return;
> > +
> > + s_listeners.erase( iter );
> > vlc_mutex_unlock( &s_lock );
> > }
> > diff --git a/modules/services_discovery/upnp.hpp b/modules/
> > services_discovery/upnp.hpp
> > index 14286276db..8c13265fc8 100644
> > --- a/modules/services_discovery/upnp.hpp
> > +++ b/modules/services_discovery/upnp.hpp
> > @@ -1,13 +1,14 @@
> > /
> > *******************************************************************
> > **********
> > * upnp.hpp : UPnP discovery module (libupnp) header
> >
> > *******************************************************************
> > **********
> > - * Copyright (C) 2004-2016 VLC authors and VideoLAN
> > + * Copyright (C) 2004-2018 VLC authors and VideoLAN
> > * $Id$
> > *
> > * Authors: Rémi Denis-Courmont <rem # videolan.org> (original
> > plugin)
> > * Christian Henz <henz # c-lab.de>
> > * Mirsal Ennaime <mirsal dot ennaime at gmail dot com>
> > * Hugo Beauzée-Luyssen <hugo at beauzee.fr>
> > + * Shaleen Jain <shaleen at jain.sh>
> > *
> > * This program is free software; you can redistribute it and/or
> > modify
> > it
> > * under the terms of the GNU Lesser General Public License as
> > published by
> > @@ -30,6 +31,7 @@
> >
> > #include <vector>
> > #include <string>
> > +#include <memory>
> >
> > #ifdef _WIN32
> > #include <windows.h>
> > @@ -64,26 +66,38 @@ namespace SD
> > */
> > class UpnpInstanceWrapper
> > {
> > +public:
> > + class Listener
> > + {
> > + public:
> > + virtual ~Listener() {}
> > + virtual int onEvent( Upnp_EventType event_type,
> > + UpnpEventPtr p_event,
> > + void* p_user_data ) = 0;
> > + };
> > +
> > +private:
> > + static UpnpInstanceWrapper* s_instance;
> > + static vlc_mutex_t s_lock;
> > + UpnpClient_Handle m_handle;
> > + int m_refcount;
> > + typedef std::shared_ptr<Listener> ListenerPtr;
> > + typedef std::vector<ListenerPtr> Listeners;
> > + static Listeners s_listeners;
> > +
> > public:
> > // This increases the refcount before returning the instance
> > - static UpnpInstanceWrapper* get(vlc_object_t* p_obj,
> > services_discovery_t *p_sd);
> > - void release(bool isSd);
> > + static UpnpInstanceWrapper* get( vlc_object_t* p_obj );
> > + void release();
> > UpnpClient_Handle handle() const;
> > - static SD::MediaServerList *lockMediaServerList();
> > - static void unlockMediaServerList();
> > + void addListener(ListenerPtr listener);
> > + void removeListener(ListenerPtr listener);
> >
> > private:
> > static int Callback( Upnp_EventType event_type, UpnpEventPtr
> > p_event, void* p_user_data );
> >
> > UpnpInstanceWrapper();
> > ~UpnpInstanceWrapper();
> > -
> > -private:
> > - static UpnpInstanceWrapper* s_instance;
> > - static vlc_mutex_t s_lock;
> > - UpnpClient_Handle m_handle;
> > - static SD::MediaServerList* p_server_list;
> > - int m_refcount;
> > };
> >
> > namespace SD
> > @@ -104,7 +118,7 @@ struct MediaServerDesc
> > };
> >
> >
> > -class MediaServerList
> > +class MediaServerList : public UpnpInstanceWrapper::Listener
> > {
> > public:
> >
> > @@ -114,7 +128,9 @@ public:
> > bool addServer(MediaServerDesc *desc );
> > void removeServer(const std::string &udn );
> > MediaServerDesc* getServer( const std::string& udn );
> > - static int Callback( Upnp_EventType event_type, UpnpEventPtr
> > p_event );
> > + int onEvent( Upnp_EventType event_type,
> > + UpnpEventPtr p_event,
> > + void* p_user_data ) override;
> >
> > private:
> > void parseNewServer( IXML_Document* doc, const std::string&
> > location );
> > --
> > 2.18.0
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
>
> Aside from that, LGTM
>
--
Regards,
Shaleen Jain
More information about the vlc-devel
mailing list