[vlc-devel] [PATCH v5 2/7] upnp: Add basic support for registering MediaRenderer service

Johan Gunnarsson johan.gunnarsson at gmail.com
Tue Nov 26 00:18:48 CET 2019


Den mån 25 nov. 2019 kl 10:04 skrev Steve Lhomme <robux4 at ycbcr.xyz>:
>
>
>
> On 2019-11-24 23:33, Johan Gunnarsson wrote:
> > This introduces a UPnP device handle that is independent of the UPnP client
> > handle. With this, modules can enable the SOAP server, web server and register
> > the media renderer services only when they are in use.
> > ---
> >   modules/services_discovery/upnp-wrapper.cpp | 109 +++++++++++++++++++-
> >   modules/services_discovery/upnp-wrapper.hpp |   7 ++
> >   2 files changed, 115 insertions(+), 1 deletion(-)
> >
> > diff --git a/modules/services_discovery/upnp-wrapper.cpp b/modules/services_discovery/upnp-wrapper.cpp
> > index bbff2f3061..2e59061437 100644
> > --- a/modules/services_discovery/upnp-wrapper.cpp
> > +++ b/modules/services_discovery/upnp-wrapper.cpp
> > @@ -36,19 +36,65 @@
> >   #include "upnp-wrapper.hpp"
> >   #include <vlc_cxx_helpers.hpp>
> >
> > +static const char *mediarenderer_desc =
> > +    "<?xml version=\"1.0\" encoding=\"utf-8\"?>"
> > +    "<root xmlns=\"urn:schemas-upnp-org:device-1-0\">"
> > +      "<specVersion>"
> > +        "<major>1</major>"
> > +        "<minor>0</minor>"
> > +      "</specVersion>"
> > +      "<device>"
> > +        "<deviceType>urn:schemas-upnp-org:device:MediaRenderer:1</deviceType>"
> > +        "<friendlyName>VLC media player</friendlyName>" /* TODO: include hostname */
> > +        "<manufacturer>VideoLAN</manufacturer>"
> > +        "<modelName>" PACKAGE_NAME "</modelName>"
> > +        "<modelNumber>" PACKAGE_VERSION "</modelNumber>"
> > +        "<modelURL>https://www.videolan.org/vlc/</modelURL>"
> > +        "<UDN>" UPNP_UDN "</UDN>" /* TODO: generate at each startup */
> > +        "<serviceList>"
> > +          "<service>"
> > +            "<serviceType>urn:schemas-upnp-org:service:RenderingControl:1</serviceType>"
> > +            "<serviceId>urn:upnp-org:serviceId:RenderingControl</serviceId>"
> > +            "<SCPDURL>/RenderingControlSCPD.xml</SCPDURL>"
> > +            "<controlURL>/upnp/control/RenderingControl</controlURL>"
> > +            "<eventSubURL>/upnp/event/RenderingControl</eventSubURL>"
> > +          "</service>"
> > +          "<service>"
> > +            "<serviceType>urn:schemas-upnp-org:service:ConnectionManager:1</serviceType>"
> > +            "<serviceId>urn:upnp-org:serviceId:ConnectionManager</serviceId>"
> > +            "<SCPDURL>/ConnectionManagerSCPD.xml</SCPDURL>"
> > +            "<controlURL>/upnp/control/ConnectionManager</controlURL>"
> > +            "<eventSubURL>/upnp/event/ConnectionManager</eventSubURL>"
> > +          "</service>"
> > +          "<service>"
> > +            "<serviceType>urn:schemas-upnp-org:service:AVTransport:1</serviceType>"
> > +            "<serviceId>urn:upnp-org:serviceId:AVTransport</serviceId>"
> > +            "<SCPDURL>/AVTransportSCPD.xml</SCPDURL>"
> > +            "<controlURL>/upnp/control/AVTransport</controlURL>"
> > +            "<eventSubURL>/upnp/event/AVTransport</eventSubURL>"
> > +          "</service>"
> > +        "</serviceList>"
> > +      "</device>"
> > +    "</root>";
> > +
> >   UpnpInstanceWrapper* UpnpInstanceWrapper::s_instance;
> >   UpnpInstanceWrapper::Listeners UpnpInstanceWrapper::s_listeners;
> >   vlc_mutex_t UpnpInstanceWrapper::s_lock = VLC_STATIC_MUTEX;
> >
> >   UpnpInstanceWrapper::UpnpInstanceWrapper()
> >       : m_client_handle( -1 )
> > +    , m_device_handle( -1 )
> >       , m_refcount( 0 )
> > +    , m_mediarenderer_refcount( 0 )
> >   {
> >   }
> >
> >   UpnpInstanceWrapper::~UpnpInstanceWrapper()
> >   {
> > -    UpnpUnRegisterClient( m_client_handle );
> > +    if( m_client_handle != -1 )
> > +        UpnpUnRegisterClient( m_client_handle );
>
> Nitpicking, this could be in a separate patch as it's unrelated to the
> added code. In fact it might be an issue to create a
> UpnpInstanceWrapper, release it right away with the current code.

You're right. I'll break it out of this patch stack.

>
> > +    if( m_device_handle != -1 )
> > +        UpnpUnRegisterRootDevice( m_device_handle );
> >       UpnpFinish();
> >   }
> >
> > @@ -104,6 +150,15 @@ UpnpInstanceWrapper *UpnpInstanceWrapper::get(vlc_object_t *p_obj)
> >               delete instance;
> >               return NULL;
> >           }
> > +
> > +        char *root = config_GetSysPath( VLC_PKG_DATA_DIR, "upnp" );
>
> This should probable be documented somewhere. At least in the commit log.
> The function can return NULL so you must check the returned value.

UpnpSetWebServerRootDir can handle NULL as parameter, but I'll change
the logic to NULL check the string anyway.

>
> > +        if( (i_res = UpnpSetWebServerRootDir( root )) != UPNP_E_SUCCESS)
> > +        {
> > +            msg_Warn( p_obj, "UpnpSetWebServerRootDir failed: %s",
> > +                      UpnpGetErrorMessage( i_res ));
>
> Shouldn't we return NULL if we can't create the server ? Or we still
> have the client available in that case ?

Client can function without server, so I think it should continue and
try to return an instance in that case.

>
> > +        }
> > +        free( root );
> > +
> >           s_instance = instance;
> >       }
> >       s_instance->m_refcount++;
> > @@ -127,6 +182,11 @@ UpnpClient_Handle UpnpInstanceWrapper::client_handle() const
> >       return m_client_handle;
> >   }
> >
> > +UpnpDevice_Handle UpnpInstanceWrapper::device_handle() const
> > +{
> > +    return m_device_handle;
> > +}
> > +
> >   int UpnpInstanceWrapper::Callback(Upnp_EventType event_type, UpnpEventPtr p_event, void *p_user_data)
> >   {
> >       vlc::threads::mutex_locker lock( &s_lock );
> > @@ -152,3 +212,50 @@ void UpnpInstanceWrapper::removeListener(ListenerPtr listener)
> >       if ( iter != s_listeners.end() )
> >           s_listeners.erase( iter );
> >   }
> > +
> > +void UpnpInstanceWrapper::startMediaRenderer( vlc_object_t *p_obj )
> > +{
> > +    vlc::threads::mutex_locker lock( &s_lock );
> > +    if( m_mediarenderer_refcount == 0 )
> > +    {
> > +        int i_res;
> > +        if( (i_res = UpnpEnableWebserver( TRUE )) != UPNP_E_SUCCESS)
> > +        {
> > +            msg_Err( p_obj, "Failed to enable webserver: %s", UpnpGetErrorMessage( i_res ) );
> > +            return;
>
> It's probably good to return an error or false when it fails.

OK.

>
> > +        }
> > +        i_res = UpnpRegisterRootDevice2( UPNPREG_BUF_DESC,
> > +                                         mediarenderer_desc,
> > +                                         strlen(mediarenderer_desc),
> > +                                         1,
> > +                                         Callback,
> > +                                         nullptr,
> > +                                         &m_device_handle );
> > +        if( i_res != UPNP_E_SUCCESS )
> > +        {
> > +            msg_Err( p_obj, "Device registration failed: %s", UpnpGetErrorMessage( i_res ) );
>
> Same here.

OK.

>
> > +        }
> > +    }
> > +    m_mediarenderer_refcount++;
>
> You can move the ++ in the if() above.

If I understand you correctly, you want something like this:

if( m_mediarenderer_refcount++ == 0 ) ...

I don't like it because then I'll have to add
m_mediarenderer_refcount-- in the error paths.

>
> > +}
> > +
> > +void UpnpInstanceWrapper::stopMediaRenderer( vlc_object_t *p_obj )
> > +{
> > +    vlc::threads::mutex_locker lock( &s_lock );
> > +    m_mediarenderer_refcount--;
>
> You can move the -- in the if() below.
>
> > +    if( m_mediarenderer_refcount == 0 )
> > +    {
> > +        int i_res;
> > +        i_res = UpnpUnRegisterRootDevice( m_device_handle );
> > +        if( i_res != UPNP_E_SUCCESS )
> > +        {
> > +            msg_Err( p_obj, "Device unregistration failed: %s", UpnpGetErrorMessage( i_res ) );
> > +        }
> > +        m_device_handle = -1;
> > +        i_res = UpnpEnableWebserver( FALSE );
> > +        if( i_res != UPNP_E_SUCCESS )
> > +        {
> > +            msg_Warn( p_obj, "Failed to disable webserver: %s", UpnpGetErrorMessage( i_res ) );
> > +        }
> > +    }
> > +}
> > diff --git a/modules/services_discovery/upnp-wrapper.hpp b/modules/services_discovery/upnp-wrapper.hpp
> > index 03116c14a1..665edbf9ac 100644
> > --- a/modules/services_discovery/upnp-wrapper.hpp
> > +++ b/modules/services_discovery/upnp-wrapper.hpp
> > @@ -41,6 +41,8 @@
> >   #include <upnp.h>
> >   #include <upnptools.h>
> >
> > +#define UPNP_UDN "uuid:034fc8dc-ec22-44e5-a79b-38c935f11663"
> > +
> >   #if UPNP_VERSION < 10800
> >   typedef void* UpnpEventPtr;
> >   #else
> > @@ -71,7 +73,9 @@ private:
> >       static UpnpInstanceWrapper* s_instance;
> >       static vlc_mutex_t s_lock;
> >       UpnpClient_Handle m_client_handle;
> > +    UpnpDevice_Handle m_device_handle;
> >       int m_refcount;
> > +    int m_mediarenderer_refcount;
> >       typedef std::shared_ptr<Listener> ListenerPtr;
> >       typedef std::vector<ListenerPtr> Listeners;
> >       static Listeners s_listeners;
> > @@ -81,8 +85,11 @@ public:
> >       static UpnpInstanceWrapper* get( vlc_object_t* p_obj );
> >       void release();
> >       UpnpClient_Handle client_handle() const;
> > +    UpnpDevice_Handle device_handle() const;
> >       void addListener(ListenerPtr listener);
> >       void removeListener(ListenerPtr listener);
> > +    void startMediaRenderer( vlc_object_t *p_obj );
> > +    void stopMediaRenderer( vlc_object_t *p_obj );
> >
> >   private:
> >       static int Callback( Upnp_EventType event_type, UpnpEventPtr p_event, void* p_user_data );
> > --
> > 2.17.1
> >
> > _______________________________________________
> > 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


More information about the vlc-devel mailing list