1210865 - Update OpenTok library to version 2.6.8

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Mark Banner (:standard8)]

Attachment: webrtc-js-2.6.7.tgz

Moving across from bug 1138851

Comment 2

[msander]

Attachment: webrtc-js-2.6.8.tgz

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

Comment 3

[Mark Banner (:standard8)]

Attachment: Update OpenTok library to version 2.6.8.

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

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

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

rs=dmose

Comment 5

[Pulsebot]

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

Comment 6

[Mark Banner (:standard8)]

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.

Comment 7

[Mark Banner (:standard8)]

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

Comment 8

[Mark Banner (:standard8)]

Attachment: 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.

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.

Comment 9

[Mark Banner (:standard8)]

Attachment: (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 c\

omplete.

Comment 10

[Mark Banner (:standard8)]

Attachment: (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.

The real -w diff this time.

Comment 11

[Mark Banner (:standard8)]

- 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.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

(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?

Comment 13

[Mark Banner (:standard8)]

(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.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 15

[Mark Banner (:standard8)]

(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.

Comment 16

[Pulsebot]

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

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Mark Banner (:standard8)]

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

Comment 19

[Mark Banner (:standard8)]

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.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 24

[Carsten Book [:Tomcat]]

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

Comment 25

[Carsten Book [:Tomcat]]

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

Comment 26

[Mark Banner (:standard8)]

(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.

Comment 27

[Mark Banner (:standard8)]

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

Approval Request Comment

Please see comment 18.

Comment 28

[Mark Banner (:standard8)]

Attachment: (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.

Approval Request Comment

Please see comment 18 and comment 19.

This is a clean patch to apply to beta.

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

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

This patch should also uplift to beta.

Comment 32

[Carsten Book [:Tomcat]]

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

Comment 33

[Bogdan Maris, Desktop Test Engineering]

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.

Comment 34

[Bogdan Maris, Desktop Test Engineering]

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