Closed
Bug 1066879
Opened 11 years ago
Closed 11 years ago
The SSDP implementation can sometimes take too long to discover a device.
Categories
(Firefox for Android Graveyard :: Screencasting, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: rbarker, Assigned: rbarker)
Details
Attachments
(1 file, 1 obsolete file)
4.36 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
Patch to increase number of SSDP packets sent by fennec.
Assignee | ||
Comment 2•11 years ago
|
||
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 3•11 years ago
|
||
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+
Assignee | ||
Comment 4•11 years ago
|
||
> 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.
Comment 5•11 years ago
|
||
Thanks for the information and thanks for the patch to make it better!
Comment 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
(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.
Assignee | ||
Comment 8•11 years ago
|
||
(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.
Assignee | ||
Comment 9•11 years ago
|
||
Addressed review comments.
Attachment #8488889 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8491684 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 10•11 years ago
|
||
Hi, could you provide a try run link (just to make sure everything works and build) thanks!
Keywords: checkin-needed
Assignee | ||
Comment 11•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 12•11 years ago
|
||
Comment 13•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Updated•5 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•