Mark Banner (:standard8) Assignee
	Description • 9 years ago

Once bug 1210861 is fixed, we should update to the latest version of the sdk.

Release Notes: https://tokbox.com/developer/sdks/js/release-notes.html

- New method for programmatically flagging an issue encountered when using OpenTok.js. See the documentation for the OT.reportIssue() method and Reporting an issue.
- Support for OT.getDevices() in Internet Explorer. See the documentation for OT.getDevices() and Setting the camera and microphone used by the publisher. 

There's not too much there that we need at this stage, but it also should support mediaDevices.enumerateDevices for gUM so we can completely drop the workaround that bug 1138851 is waiting to drop.

	Mark Banner (:standard8) Assignee
	Comment 1 • 9 years ago

Moving across from bug 1138851

	msander
	Comment 2 • 9 years ago

Patch for ICE servers not being set some times due to a race condition.

	Mark Banner (:standard8) Assignee
	Comment 3 • 9 years ago

This updates us to the latest version. I've given it a test run in a few places and can't see any issues.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 4 • 9 years ago

Comment on attachment 8680578 [details] [diff] [review]
Update OpenTok library to version 2.6.8.

rs=dmose

	Pulsebot
	Comment 5 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/dcd113d0102e

	Mark Banner (:standard8) Assignee
	Comment 6 • 9 years ago

Note: We decided not to wait on bug 1210861 as the percentage of users affected by that is relatively small, but the potential for improvement for users of Hello is quite big. We won't know for sure how big it will be until it is out, but we need to get it out there to find out, hence pushing it out now.

	Mark Banner (:standard8) Assignee
	Comment 7 • 9 years ago

Unfortunately I had to back this out as it seems to break the data channel setup that we're using on Hello. I'll have to investigate that more next week to see what's happening here.

https://hg.mozilla.org/integration/fx-team/rev/b3a64c90e34f

	Mark Banner (:standard8) Assignee
	Comment 8 • 9 years ago

This should fix the issues I was seeing with data channels and the new sdk (this is a separate patch to the sdk patch).

From my testing, I think the sdk is more dependent on timing that it was before. Previously, we'd send a message via TokBox's messaging protocol to indiciate to setup the data channel. However, that doesn't necessarily guarentee that a remote stream has been subscribed to. Hence, getting the data channel would sometimes abort as the subscriber hadn't been fully set up.

I've also added a functional test for this, my local testing indicates it would have potentially caused some intermittents in the tests, but I think its less frequent as we're not split across two instances, and we also turn off some of the routines around grabbing ICE entries that would delay the setups.

	Mark Banner (:standard8) Assignee
	Comment 9 • 9 years ago

omplete.

	Mark Banner (:standard8) Assignee
	Comment 10 • 9 years ago

The real -w diff this time.

	Mark Banner (:standard8) Assignee
	Comment 11 • 9 years ago

- Main part of this patch is making the getDataChannel for the subscriber happen after the  notification of subscription completion. This ensures we'll be able to set up correctly.

- I've verified with old versions of FF, without the data channel code, that this still works - it generates an error on the console, but text chat isn't enabled.

- A lot of the changes in otSdkDriver_test.js are whitespace, due to moving more things to beforeEach functions, and adding in additional describe layers. Some tests are moved/extended to deal with the moved code.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 12 • 9 years ago

(In reply to Mark Banner (:standard8) from comment #8)
> 
> I've also added a functional test for this, my local testing indicates it
> would have potentially caused some intermittents in the tests, but I think
> its less frequent as we're not split across two instances, and we also turn
> off some of the routines around grabbing ICE entries that would delay the
> setups.


I'm confused here.  Are you saying the functional test that you've added to the patch is likely to have intermittent failures?  Or something else?

	Mark Banner (:standard8) Assignee
	Comment 13 • 9 years ago

(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #12)
> (In reply to Mark Banner (:standard8) from comment #8)
> > 
> > I've also added a functional test for this, my local testing indicates it
> > would have potentially caused some intermittents in the tests, but I think
> > its less frequent as we're not split across two instances, and we also turn
> > off some of the routines around grabbing ICE entries that would delay the
> > setups.
> 
> 
> I'm confused here.  Are you saying the functional test that you've added to
> the patch is likely to have intermittent failures?  Or something else?

The functional test would have caught intermittents before the rest of the patch was applied.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 14 • 9 years ago

Comment on attachment 8682754 [details] [diff] [review]
(diff -w) Change how Loop's data channels are setup to cope with the newer SDK that doesn't allow setting them up until subscription is complete.

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

Looks good, r=dmose with comments addressed as you see fit.

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +671,5 @@
>            console.error(signalError);
>          }
>        });
> +
> +      sdkSubscriberObject._.getDataChannel("text", {}, function(err, channel) {

Reaching inside a Subscriber to use an undocumented API seems... fragile.  Since this is what seems to be used through the file, I assume this is what tokbox is asking us to use?

@@ +708,5 @@
>       * Handles receiving the signal that the other end of the connection
>       * has subscribed to the stream and we're ready to setup the data channel.
>       *
>       * We get data channels for both the publisher and subscriber on reception
>       * of the signal, as it means that a) the remote client is setup for data

According to your description of the fix, the comments above aren't quite right.  If you could update them, that would be great.

::: browser/components/loop/test/shared/otSdkDriver_test.js
@@ +1106,5 @@
> +
> +            sinon.assert.notCalled(session.signal);
> +          });
> +
> +          it("should get the data channel after subscribe is complete", function() {

I'd say 'after streamCreated is triggered on the session', since that's a more accurate description of what's being tested here.

	Mark Banner (:standard8) Assignee
	Comment 15 • 9 years ago

(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #14)
> Comment on attachment 8682754 [details] [diff] [review]
> > +      sdkSubscriberObject._.getDataChannel("text", {}, function(err, channel) {
> 
> Reaching inside a Subscriber to use an undocumented API seems... fragile. 
> Since this is what seems to be used through the file, I assume this is what
> tokbox is asking us to use?

Yes, that's the API we were given to use.

	Pulsebot
	Comment 16 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/aa429ba5cf28
https://hg.mozilla.org/integration/fx-team/rev/0823cc068192

	Carsten Book [:Tomcat]
	Comment 17 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/aa429ba5cf28
https://hg.mozilla.org/mozilla-central/rev/0823cc068192

	Mark Banner (:standard8) Assignee
	Comment 18 • 9 years ago

Comment on attachment 8680578 [details] [diff] [review]
Update OpenTok library to version 2.6.8.

Approval Request Comment
[Feature/regressing bug #]: Firefox Hello
[User impact if declined]: This is an upgrade to the sdk from OpenTok that fixes a case where a race condition could lead to failed call setups. We'd like to roll this out sooner to get the fix out to our users.
[Describe test coverage new/current, TreeHerder]: Code has unit tests and functional tests. Landed in m-c for a while now, also has been shipped on the Hello standalone UI and so the upgraded version is used for one half of conversations.
[Risks and why]: Low - sdk shipped by third party, only minor additional changes required on our side (see separate patch), we also have some metrics tracking to keep an eye on successful conversations.
[String/UUID change made/needed]: None

	Mark Banner (:standard8) Assignee
	Comment 19 • 9 years ago

Comment on attachment 8681975 [details] [diff] [review]
Change how Loop's data channels are setup to cope with the newer SDK that doesn't allow setting them up until subscription is complete.

Approval Request Comment
[Feature/regressing bug #]: See comment 18
[User impact if declined]: See comment 18
[Describe test coverage new/current, TreeHerder]: See comment 18
[Risks and why]: Low, minor change to the way some of the setup code works. This has been on our standalone web site for a while now, and on mozilla-central. Our metrics monitoring hasn't picked up any issues.
[String/UUID change made/needed]: None

Note: We're going to request these for beta as well, but there's a minor bitrot fix to the tests needed, so I'm generating a new patch.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 20 • 9 years ago

Comment on attachment 8680578 [details] [diff] [review]
Update OpenTok library to version 2.6.8.

Given that this has baked on Nightly for over 2 weeks and Mark's comment that the metrics on Hello calls have not given any indication of possible issues, let's uplift to Aurora44.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 21 • 9 years ago

Comment on attachment 8681975 [details] [diff] [review]
Change how Loop's data channels are setup to cope with the newer SDK that doesn't allow setting them up until subscription is complete.

Aurora44+

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 22 • 9 years ago

Mark, should we add these to 44 release notes when it goes to Beta? Or should these go into 43 release notes as you mentioned a possibility of uplifting this to Beta as well. Btw, I feel this is an unusually large changeset for a library/sdk update.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 23 • 9 years ago

Requesting QE team to test this fix as it impacts Hello calls.

	Carsten Book [:Tomcat]
	Comment 24 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/11329c457f99
https://hg.mozilla.org/releases/mozilla-aurora/rev/d574b73912fc

	Carsten Book [:Tomcat]
	Comment 25 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/11329c457f99
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d574b73912fc

	Mark Banner (:standard8) Assignee
	Comment 26 • 9 years ago

(In reply to Ritu Kothari (:ritu) from comment #22)
> Mark, should we add these to 44 release notes when it goes to Beta? Or
> should these go into 43 release notes as you mentioned a possibility of
> uplifting this to Beta as well.

I'm not sure what you feel needs release noting here. There should be improvements to connectivity rates, but we don't yet know how much.

> Btw, I feel this is an unusually large
> changeset for a library/sdk update.

The diffs are often larger than expected due to the way the opentok sdk is generated. However, this is slightly bigger change (2.5.x -> 2.6.x) than I'd normally request for an uplift, but the functional changes weren't affecting us much, so it seemed reasonable.

	Mark Banner (:standard8) Assignee
	Comment 27 • 9 years ago

Comment on attachment 8680578 [details] [diff] [review]
Update OpenTok library to version 2.6.8.

Approval Request Comment

Please see comment 18.

	Mark Banner (:standard8) Assignee
	Comment 28 • 9 years ago

Approval Request Comment

Please see comment 18 and comment 19.

This is a clean patch to apply to beta.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 29 • 9 years ago

Comment on attachment 8693512 [details] [diff] [review]
(beta patch) Change how Loop's data channels are setup to cope with the newer SDK that doesn't allow setting them up until subscription is complete.

Improves OpenTok sdk setup, OK to uplift to beta. 
Looks like a big patch but most of that is changes in tests.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 30 • 9 years ago

If this has problems landing in 43 beta 8, though, I'd like to hold it back to release with 44.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 31 • 9 years ago

Comment on attachment 8680578 [details] [diff] [review]
Update OpenTok library to version 2.6.8.

This patch should also uplift to beta.

	Carsten Book [:Tomcat]
	Comment 32 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/634347ba4383
https://hg.mozilla.org/releases/mozilla-beta/rev/880b47c05d42

	Bogdan Maris, Desktop QA
	Comment 33 • 9 years ago

We held hello calls across platforms (Windows 7 64-bit, Mac OS X 10.11.1, Ubuntu 14.04 64-bit and Windows 10 64-bit) using Firefox 43 beta 9 and we did not encountered any issues regarding video/audio, call drops etc.
Leaving QA+ for verification in 44 and 45.

	Bogdan Maris, Desktop QA
	Comment 34 • 9 years ago

Also verified using latest Nightly 45.0a1 and latest Developer Edition 44.0a2 across platforms.