Closed Bug 1066879 Opened 5 years ago Closed 5 years ago

The SSDP implementation can sometimes take too long to discover a device.

Categories

(Firefox for Android :: Screencasting, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: rbarker, Assigned: rbarker)

Details

Attachments

(1 file, 1 obsolete file)

On networks with high packet loss, it can take too long to discover a streaming device with SSDP. To increase the chance of discovery, the packets should be sent out with a delay between each send and sent multiple times.
Attached patch SSDP transmission increase v1 (obsolete) — Splinter Review
Patch to increase number of SSDP packets sent by fennec.
Comment on attachment 8488889 [details] [diff] [review]
SSDP transmission increase v1

I was seeing heavy packet loss on my home network. This change made discovery much more reliable.
Attachment #8488889 - Flags: review?(wjohnston)
Attachment #8488889 - Flags: review?(mark.finkle)
Comment on attachment 8488889 [details] [diff] [review]
SSDP transmission increase v1

>diff --git a/mobile/android/modules/SimpleServiceDiscovery.jsm b/mobile/android/modules/SimpleServiceDiscovery.jsm

>     this._searchSocket = socket;
>     this._searchTimeout.initWithCallback(this._searchShutdown.bind(this), SSDP_DISCOVER_TIMEOUT, Ci.nsITimer.TYPE_ONE_SHOT);

If we are pulling in Timer.jsm we might want to convert this usage in a new bug. Also, I just wanted to be sure you knew that this timer kills the discovery "wait" and might need to be extended. Maybe? Maybe not?

>+    let timeout = 500;
>+    // Send discovery packets out at 1 per second and send each 3 times to allow for packet loss on noisy networks.

Move the | let timeout | after the comment

>+    for (let ix = 0; ix < 3; ix++) {

ix -> attempt  ?

>+      for (let [key, target] of this._targets) {
>+        let t = target.target;

After you unbitrot (see below) this will likely be | let t = device.target; | at which point I'd like to rename "t" to "target" since it won't be ambiguous then.

>+        setTimeout(function() {
>+          let msgData = data.replace("%SEARCH_TARGET%", t);
>+          try {
>+            let msgRaw = converter.convertToByteArray(msgData);
>+            socket.send(SSDP_ADDRESS, SSDP_PORT, msgRaw, msgRaw.length);
>+          } catch (e) {
>+            log("failed to convert to byte array: " + e);
>+          }
>+        }, timeout);
>+        timeout += 1000;
>       }
>     }

I might be landing a few patches today that will bitrot this a little.

r+ with nits

Just a general question: When you say "takes to long" - what's the situation? Is this at startup? I think I noticed a lag at startup, but I assumed it was the "120 second" delay between discovery pings. I wanted to see if that could be the problem and make sure we were not waiting on the very first ping. Looking at the code, I see now that we are not.
Attachment #8488889 - Flags: review?(mark.finkle) → review+
> Just a general question: When you say "takes to long" - what's the
> situation? Is this at startup? I think I noticed a lag at startup, but I
> assumed it was the "120 second" delay between discovery pings. I wanted to
> see if that could be the problem and make sure we were not waiting on the
> very first ping. Looking at the code, I see now that we are not.

On my home network, the first discover packets would get dropped (it would never make to the machine being discovered \, UDP is unreliable) so I would either have to wait 120 seconds for it to be sent again or restart fennec. This change meant the device would always (in my limited testing) be discovered on the first try.
Thanks for the information and thanks for the patch to make it better!
Comment on attachment 8488889 [details] [diff] [review]
SSDP transmission increase v1

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

I think I'm ok with this. I hate the var ix variable naming. Can you use "attempt" or something?

::: mobile/android/modules/SimpleServiceDiscovery.jsm
@@ +159,5 @@
>        return;
>      }
>  
>      this._searchSocket = socket;
>      this._searchTimeout.initWithCallback(this._searchShutdown.bind(this), SSDP_DISCOVER_TIMEOUT, Ci.nsITimer.TYPE_ONE_SHOT);

It seems better to reuse this guy here for this. i.e. 

const MAX_TIME = 4000;
let startTime = Date.now();
this._searchTimeout.initWithCallback(() => {
  if (Date.now() - startTime < MAX_TIME) {
    tryToConnect();
  } else {
    this._searchShutdown();
  }
}, SSDP_DISCOVER_TIMEOUT, Ci.nsITimer.TYPE_ONE_SHOT);
tryToConnect();

But this won't handle a case where you get device A on try 1, and device B on try 2. Is that what's happening here?

@@ +162,5 @@
>      this._searchSocket = socket;
>      this._searchTimeout.initWithCallback(this._searchShutdown.bind(this), SSDP_DISCOVER_TIMEOUT, Ci.nsITimer.TYPE_ONE_SHOT);
>  
>      let data = SSDP_DISCOVER_PACKET;
> +    let timeout = 500;

Mark this as a const (put it at the top of this file if you want).

@@ +164,5 @@
>  
>      let data = SSDP_DISCOVER_PACKET;
> +    let timeout = 500;
> +    // Send discovery packets out at 1 per second and send each 3 times to allow for packet loss on noisy networks.
> +    for (let ix = 0; ix < 3; ix++) {

Make the three a const in the file.

@@ +166,5 @@
> +    let timeout = 500;
> +    // Send discovery packets out at 1 per second and send each 3 times to allow for packet loss on noisy networks.
> +    for (let ix = 0; ix < 3; ix++) {
> +      for (let [key, target] of this._targets) {
> +        let t = target.target;

This has changed now. Make sure to unbitrot.
Attachment #8488889 - Flags: review?(wjohnston) → review+
(In reply to Mark Finkle (:mfinkle) from comment #3)
> Comment on attachment 8488889 [details] [diff] [review]
> SSDP transmission increase v1
> 
> >diff --git a/mobile/android/modules/SimpleServiceDiscovery.jsm b/mobile/android/modules/SimpleServiceDiscovery.jsm
> 
> >     this._searchSocket = socket;
> >     this._searchTimeout.initWithCallback(this._searchShutdown.bind(this), SSDP_DISCOVER_TIMEOUT, Ci.nsITimer.TYPE_ONE_SHOT);
> 
> If we are pulling in Timer.jsm we might want to convert this usage in a new
> bug. Also, I just wanted to be sure you knew that this timer kills the
> discovery "wait" and might need to be extended. Maybe? Maybe not?
>

Hmm, We should probably derive SSDP_DISCOVER_TIMEOUT based on number of device types being discovered and number of iterations. I'll add that.
(In reply to Wesley Johnston (:wesj) from comment #6)
> Comment on attachment 8488889 [details] [diff] [review]
> SSDP transmission increase v1
> 
> Review of attachment 8488889 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I'm ok with this. I hate the var ix variable naming. Can you use
> "attempt" or something?
> 
:mfinkle had the same request, I'll change it.

> ::: mobile/android/modules/SimpleServiceDiscovery.jsm
> @@ +159,5 @@
> >        return;
> >      }
> >  
> >      this._searchSocket = socket;
> >      this._searchTimeout.initWithCallback(this._searchShutdown.bind(this), SSDP_DISCOVER_TIMEOUT, Ci.nsITimer.TYPE_ONE_SHOT);
> 
> It seems better to reuse this guy here for this. i.e. 
> 
> const MAX_TIME = 4000;
> let startTime = Date.now();
> this._searchTimeout.initWithCallback(() => {
>   if (Date.now() - startTime < MAX_TIME) {
>     tryToConnect();
>   } else {
>     this._searchShutdown();
>   }
> }, SSDP_DISCOVER_TIMEOUT, Ci.nsITimer.TYPE_ONE_SHOT);
> tryToConnect();
> 
> But this won't handle a case where you get device A on try 1, and device B
> on try 2. Is that what's happening here?
> 

The _searchTimeout timer is a TYPE_ONE_SHOT. Also, I want each discovery packet to be sent at a one second interval. I could reuse the timer however keeping track of which device I'm trying to discover each time it fires seems like too much overhead when I can just stash the state in each individual timer that fires.

> @@ +162,5 @@
> >      this._searchSocket = socket;
> >      this._searchTimeout.initWithCallback(this._searchShutdown.bind(this), SSDP_DISCOVER_TIMEOUT, Ci.nsITimer.TYPE_ONE_SHOT);
> >  
> >      let data = SSDP_DISCOVER_PACKET;
> > +    let timeout = 500;
> 
> Mark this as a const (put it at the top of this file if you want).
> 
> @@ +164,5 @@
> >  
> >      let data = SSDP_DISCOVER_PACKET;
> > +    let timeout = 500;
> > +    // Send discovery packets out at 1 per second and send each 3 times to allow for packet loss on noisy networks.
> > +    for (let ix = 0; ix < 3; ix++) {
> 
> Make the three a const in the file.
> 

Will do.

> @@ +166,5 @@
> > +    let timeout = 500;
> > +    // Send discovery packets out at 1 per second and send each 3 times to allow for packet loss on noisy networks.
> > +    for (let ix = 0; ix < 3; ix++) {
> > +      for (let [key, target] of this._targets) {
> > +        let t = target.target;
> 
> This has changed now. Make sure to unbitrot.

Already done.
Addressed review comments.
Attachment #8488889 - Attachment is obsolete: true
Keywords: checkin-needed
Hi, could you provide a try run link (just to make sure everything works and build) thanks!
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #10)
> Hi, could you provide a try run link (just to make sure everything works and
> build) thanks!

https://tbpl.mozilla.org/?tree=Try&rev=2ea5a50fb7b0

There currently aren't any tests for this part of the code.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/0888efcfc40c
Assignee: nobody → rbarker
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0888efcfc40c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.