751172 - [B2G] SSDP advertising Service

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[Thinker Li [:sinker]]

Implement SSDP service that delegates advertising and responses for discovery requests for the registered services.  The function of finding services from other devices is not a part of this bug that should be in a follow-up bug.

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: proposed SSDP service interface

proposed interface for SSDP service, also include the device/service profile interface which required by SSDP service.

Comment 2

[Thinker Li [:sinker]]

Comment on attachment 635689 [details] [diff] [review]
proposed SSDP service interface

Review of attachment 635689 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/ssdp/nsISSDPService.idl
@@ +1,5 @@
> +#include "nsrootidl.idl"
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(9f0af628-6150-4ca3-863e-d0b88157f764)]
> +interface nsIServiceProfile: nsISupports

With an SSDP prefix? nsISSDPServiceProfile?

@@ +11,5 @@
> +  //TODO: action/event/state descriptor
> +};
> +
> +[scriptable, uuid(35511f8c-80a7-4477-add1-f563ee86c56c)]
> +interface nsIDeviceProfile : nsISupports

As well

@@ +17,5 @@
> +  attribute nsID uuid;
> +  attribute ACString urn;
> +  attribute ACString xmlDescriptor;
> +
> +  nsIDeviceProfile getEmbeddedDevice(in nsID deviceUUID);

See the comment for getServiceProfile.

@@ +20,5 @@
> +
> +  nsIDeviceProfile getEmbeddedDevice(in nsID deviceUUID);
> +  void asyncAddDevice(in nsIDeviceProfile deviceProfile);
> +  void asyncRemoveDevice(in nsID deviceUUID);
> +  nsIServiceProfile getServiceProfile(in nsID serviceUUID);

Async? I guess this is a small set of items.  You can just provide an enumerator; for example, nsISimpleEnumerator or alike.

@@ +22,5 @@
> +  void asyncAddDevice(in nsIDeviceProfile deviceProfile);
> +  void asyncRemoveDevice(in nsID deviceUUID);
> +  nsIServiceProfile getServiceProfile(in nsID serviceUUID);
> +  void asyncAddService(in nsIServiceProfile serviceProfile);
> +  void asyncRemoveService(in nsID serviceUUID);

Just remove the prefix |async| and return something like DOMRequest for async.

For example,

interface nsISSDPRequest : nsISupports
{
  attribute nsISSDPSuccessCallback onsuccess;
  attribute nsISSDPErrorCallback onerror;
};

@@ +37,5 @@
> +
> +  /**
> +   * host name used in SSDP advertisement
> +   */
> +  attribute ACString hostName;

Readonly?

@@ +45,5 @@
> +   * @param aHttpPort port number for http server
> +   * @param aExpiryTime SSDP advertisement expiration time
> +   * @param aHostName host name used in SSDP advertisement
> +   */
> +  void init(in unsigned long aHttpPort, in unsigned long aExpiryTime, in ACString aHostName);

We don't prefix argument names with |a| in IDL.  But, xpidl generates header files with |a| prefixed argument names.  So, we use argument names with |a| prefixed for native code.

@@ +50,5 @@
> +
> +  /**
> +   * start SSDP service
> +   */
> +  void start();

Async?

@@ +55,5 @@
> +
> +  /**
> +   * stop SSDP service
> +   */
> +  void stop();

Async?

Can you restart this service?  For example, stop it, and re-init it, then start it again.

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Thinker Li [:sinker] from comment #2)
> Comment on attachment 635689 [details] [diff] [review]
> proposed SSDP service interface
> 
> Review of attachment 635689 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/ssdp/nsISSDPService.idl
> @@ +1,5 @@
> > +#include "nsrootidl.idl"
> > +#include "nsISupports.idl"
> > +
> > +[scriptable, uuid(9f0af628-6150-4ca3-863e-d0b88157f764)]
> > +interface nsIServiceProfile: nsISupports
> 
> With an SSDP prefix? nsISSDPServiceProfile?
> 
originally I try to implement SSDP as a sub-service in UPnP service, therefore, I chose a service-neutral name for ServiceProfile/DeviceProfile. I can add "SSDP" prefix to the profile type since we are going to make SSDP an independent service.
I'll change the name and put the diff on bugzilla later.
> @@ +11,5 @@
> > +  //TODO: action/event/state descriptor
> > +};
> > +
> > +[scriptable, uuid(35511f8c-80a7-4477-add1-f563ee86c56c)]
> > +interface nsIDeviceProfile : nsISupports
> 
> As well
> 
see notes for nsIServiceProfile above.
> @@ +17,5 @@
> > +  attribute nsID uuid;
> > +  attribute ACString urn;
> > +  attribute ACString xmlDescriptor;
> > +
> > +  nsIDeviceProfile getEmbeddedDevice(in nsID deviceUUID);
> 
> See the comment for getServiceProfile.
> 
> @@ +20,5 @@
> > +
> > +  nsIDeviceProfile getEmbeddedDevice(in nsID deviceUUID);
> > +  void asyncAddDevice(in nsIDeviceProfile deviceProfile);
> > +  void asyncRemoveDevice(in nsID deviceUUID);
> > +  nsIServiceProfile getServiceProfile(in nsID serviceUUID);
> 
> Async? I guess this is a small set of items.  You can just provide an
> enumerator; for example, nsISimpleEnumerator or alike.
> 
Ok to provide enumerator interface for retrieving embedded device/service profile.
BTW, I suggest to provide a snapshot on device/service profile for those service user, since it's bug-prone to allow service user seeing real-time update introduced by other thread. Does this design match the convention in mozilla?
> @@ +22,5 @@
> > +  void asyncAddDevice(in nsIDeviceProfile deviceProfile);
> > +  void asyncRemoveDevice(in nsID deviceUUID);
> > +  nsIServiceProfile getServiceProfile(in nsID serviceUUID);
> > +  void asyncAddService(in nsIServiceProfile serviceProfile);
> > +  void asyncRemoveService(in nsID serviceUUID);
> 
> Just remove the prefix |async| and return something like DOMRequest for
> async.
> 
> For example,
> 
> interface nsISSDPRequest : nsISupports
> {
>   attribute nsISSDPSuccessCallback onsuccess;
>   attribute nsISSDPErrorCallback onerror;
> };
> 
I'll add the callback interface and rename these async interface.
> @@ +37,5 @@
> > +
> > +  /**
> > +   * host name used in SSDP advertisement
> > +   */
> > +  attribute ACString hostName;
> 
> Readonly?
> 
yes, should make it read only and can only change via |init| function.
> @@ +45,5 @@
> > +   * @param aHttpPort port number for http server
> > +   * @param aExpiryTime SSDP advertisement expiration time
> > +   * @param aHostName host name used in SSDP advertisement
> > +   */
> > +  void init(in unsigned long aHttpPort, in unsigned long aExpiryTime, in ACString aHostName);
> 
> We don't prefix argument names with |a| in IDL.  But, xpidl generates header
> files with |a| prefixed argument names.  So, we use argument names with |a|
> prefixed for native code.
> 
I'll remove the |a| prefix in IDL, diff will be provided later.
> @@ +50,5 @@
> > +
> > +  /**
> > +   * start SSDP service
> > +   */
> > +  void start();
> 
> Async?
> 
> @@ +55,5 @@
> > +
> > +  /**
> > +   * stop SSDP service
> > +   */
> > +  void stop();
> 
> Async?
> 
> Can you restart this service?  For example, stop it, and re-init it, then
> start it again.
Currently I implement start/stop in a synchronous way, since the procedure for start/stop is quite light-weight. I can make it async under the consideration of access service interface in multiple thread at the same time.
And sure, SSDP service is designed to allow re-init and restart.

Updated IDL will be put on bugzilla later.

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: p2 for SSDP IDL

update IDL accroding to Thinker's feedback. introduce nsISSDPServiceRequest/nsISSDPServiceCallback for async function invocation. introduce nsISSDPDeviceEnumeration/nsISSDPServiceEnumeration for retrieving embedded device/service profiles.

Comment 5

[Thinker Li [:sinker]]

(In reply to Shih-Chiang Chien from comment #4)
> Created attachment 637053 [details] [diff] [review]
> p2 for SSDP IDL
> 
> update IDL accroding to Thinker's feedback. introduce
> nsISSDPServiceRequest/nsISSDPServiceCallback for async function invocation.
> introduce nsISSDPDeviceEnumeration/nsISSDPServiceEnumeration for retrieving
> embedded device/service profiles.

Please upload full definition of IDL instead of the diff file aganist previous version.

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: full definition of SSDP IDL

SSDP IDL file

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: full diff of SSDP IDL V2

Comment 8

[Thinker Li [:sinker]]

Comment on attachment 638618 [details] [diff] [review]
full diff of SSDP IDL V2

Review of attachment 638618 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/ssdp/nsISSDPService.idl
@@ +5,5 @@
> +
> +[scriptable, uuid(3db3800c-fbb2-456e-b4a0-c48e419223b5)]
> +interface nsISSDPServiceCallback: nsISupports
> +{
> +  void onFinish(in nsresult rv);

onHandle? onFinish is weir for onError.
Also, I don't know if it is feasible to pass nsresult through interface/or callback.  You need a more meaningful type for errors here.

@@ +11,5 @@
> +
> +[scriptable, uuid(747cf538-ce98-4b61-8bf9-a38902610840)]
> +interface nsISSDPServiceRequest: nsISupports
> +{
> +  attribute nsISSDPServiceCallback onSuccess;

What is the value of rv argument for onSuccess?  If it is alway 0 (or something), you should define another callback interface to avoid passing a meaningless argument.

@@ +47,5 @@
> +  attribute ACString urn;
> +  attribute ACString xmlDescriptor;
> +
> +  nsISSDPDeviceEnumeration getEmbeddedDevices();
> +  nsISSDPServiceEnumeration getServices();

I am not sure if we should also make above two functions async.  Maybe we should.

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: SSDP IDL v3

separate onSuccess/onError callback
async method call for retrieving device/service profile

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: WIP for SSDP implementation

WIP for SSDP, including SSDP service API & profile management

Comment 11

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: WIP for SSDP v1

SSDP Service implementation, contains SSDP service operation, SSDP Profile edit/read operation. Both C++ and xpcshell testcases are provided.
remain 1 todo item - response M-SEARCH request from control point.

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 642228 [details] [diff] [review]
WIP for SSDP v1

We've already implemented SSDP on Android in http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/SimpleServiceDiscovery.jsm. Should be able to reuse it on Firefox OS.

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: WIP - ssdp.js and polyfill of network service discovery API

Implement SSDP control point and polyfill Network Service Discovery API in javascript.

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I've uploaded my current work to https://github.com/schien/ssdp.js

Comment 15

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: demo.patch

With the WIP patch in bug 745283, this patch provides a chrome page that can scan DLNA content provider and display image/audio by integrating ssdp.js and plug.play.js. (Many thanks to @rexboy for writing the chrome page)

The demo page is in chrome://global/content/player.html.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

why not use the SSDP support that's already in the tree? 

http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/SimpleServiceDiscovery.jsm

Comment 17

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #16)
> why not use the SSDP support that's already in the tree? 
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/
> SimpleServiceDiscovery.jsm

Hi Brad,
We want to provide DLNA functionality in webapp. Per discussion in bug 914579, we don't want to implement the Discovery API. So we cannot reuse the SimpleServiceDiscovery.jsm.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that it might be possible to reuse the same code as in SimpleServiceDiscovery.jsm.

But we would need to refactor that code so that it can run on top of the internal gecko interfaces that it's currently using (timers and sockets etc) as well as on top of standardized web APIs.

I'm not sure how much work is involved in doing that though.

Comment 19

[Jason Proctor]

will this use a standard or proposed standard protocol? RFC 6763 / SMPTE 2701, etc?

thanks for any info :-)

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Jason Proctor from comment #19)
> will this use a standard or proposed standard protocol? RFC 6763 / SMPTE
> 2701, etc?
> 
> thanks for any info :-)

No, this bug is focusing on SSDP protocol from UPnP standard but ssdp.js is opened for contributing other device discovery protocols.

Comment 21

[André Fiedler]

The SSDP API should require a user permission (like sharing the GPS does). Because with SSDP you can make a fingerprint of a network from user A. If this fingerprint is unique and user B has the same fingerprint, he is at the same location as user A. Another issue is, that you can easily detect/track which devices the user is owning (does he have a Apple TV?).