Closed Bug 1131568 Opened 5 years ago Closed 5 years ago

Update OpenTok library to a version that supports browser sharing and Stream replacing

Categories

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

defect
Points:
2

Tracking

(firefox38 fixed, firefox39 fixed)

RESOLVED FIXED
mozilla39
Iteration:
39.1 - 9 Mar
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
Blocking Flags:
backlog backlog+

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Whiteboard: [screensharing])

Attachments

(2 files, 1 obsolete file)

For sharing a browser window, aka 'tab sharing', we'll need a version of the SDK that officially supports gUM video source value of 'browser'.
Flags: qe-verify-
Additionally, this version of the SDK needs to support replaceStream() on an already published stream.
Summary: Update OpenTok library to a version that supports browser sharing → Update OpenTok library to a version that supports browser sharing and Stream replacing
triaging to blocking-loop:Fx38+ for tracking this release, all bugs in 38+ and 38? now, will move to "backlog+" for Fx39.  After that - we won't triage to a specific release any longer.  we will work from an ordered backlog.
backlog: --- → Fx38+
Target Milestone: mozilla38 → ---
After a talk with Brad (:blassey) past Friday, we figured out that the browser sharing code has changed significantly from the gUM perspective:

 - mediaSource 'browser' as constraint is still valid
 - new constraint `browserWindow: <StringOrNumber>` is a required, unique identifier to signal the user agent which (content) window the start sharing.
 - new constraint `scrollWithPage: <Boolean>` is an optional flag that signals the user agent to follow the users' scroll position while streaming the viewport. Defaults to `false`

This means that the upcoming SDK version needs to support passing these three constraints to the Publisher:

```js
var publisher = OT.initPublisher($(".screenShare"), {
  videoSource: "browser",
  browserWindow: "673bc16c-5c76-4efe-acc4-391b3ece2937",
  scrollWithPage: true
});
```

When the user switches a tab, we'd do another call to replace the stream:

```js
publisher.publishVideo(true, {
  videoSource: "browser",
  browserWindow: "9ce3c006-f10a-4e7a-9fae-bb978c69232f",
  scrollWithPage: true
});
```

... which will do a replaceStream() effectively after a new gUM call with the updated constraints.

Does this make sense?
Flags: needinfo?(msander)
(In reply to Mike de Boer [:mikedeboer] from comment #3)
>  - new constraint `browserWindow: <StringOrNumber>` is a required, unique
> identifier to signal the user agent which (content) window the start sharing.

s/the/to/
Blocks: 1133493
Flagging Jonathan to take a look at this since he's implementing the change to the SDK.
Flags: needinfo?(msander) → needinfo?(jonathan)
(In reply to Mike de Boer [:mikedeboer] from comment #3)
> After a talk with Brad (:blassey) past Friday, we figured out that the
> browser sharing code has changed significantly from the gUM perspective:
> 
>  - mediaSource 'browser' as constraint is still valid
>  - new constraint `browserWindow: <StringOrNumber>` is a required, unique
> identifier to signal the user agent which (content) window the start sharing.
>  - new constraint `scrollWithPage: <Boolean>` is an optional flag that
> signals the user agent to follow the users' scroll position while streaming
> the viewport. Defaults to `false`
> 
> This means that the upcoming SDK version needs to support passing these
> three constraints to the Publisher:
> 
> ```js
> var publisher = OT.initPublisher($(".screenShare"), {
>   videoSource: "browser",
>   browserWindow: "673bc16c-5c76-4efe-acc4-391b3ece2937",
>   scrollWithPage: true
> });
> ```
> 
> When the user switches a tab, we'd do another call to replace the stream:
> 
> ```js
> publisher.publishVideo(true, {
>   videoSource: "browser",
>   browserWindow: "9ce3c006-f10a-4e7a-9fae-bb978c69232f",
>   scrollWithPage: true
> });
> ```
> 
> ... which will do a replaceStream() effectively after a new gUM call with
> the updated constraints.
> 
> Does this make sense?

Hi Mike,

Jonathan here, I'm working on this issue at TokBox.

Is there a way for us to get an hand on a build of FF that supports "browser" as a capture surface type + "browserWindow"? Also, how do we get a the "browserWindow" value? I would like to test that we don't brake any invariants in the Publisher.
Flags: needinfo?(jonathan) → needinfo?(mdeboer)
(In reply to Jonathan from comment #6)
> Hi Mike,
> 
> Jonathan here, I'm working on this issue at TokBox.
> 
> Is there a way for us to get an hand on a build of FF that supports
> "browser" as a capture surface type + "browserWindow"? Also, how do we get a
> the "browserWindow" value? I would like to test that we don't brake any
> invariants in the Publisher.

Hi Jonathan!

Here are links to builds that show the feature at work:

Windows 64bit (choose the installer .exe or .zip file): http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mdeboer@mozilla.com-34e10da490e9/try-win64/
OSX: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mdeboer@mozilla.com-34e10da490e9/try-macosx64/firefox-38.0a1.en-US.mac.dmg
Linux: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mdeboer@mozilla.com-34e10da490e9/try-linux64/firefox-38.0a1.en-US.linux-x86_64.tar.bz2

I'll put up the patched version of the SDK I used for your reference, which should contain the pointers you need.
Flags: needinfo?(mdeboer)
Attached patch SDK updates for browser sharing (obsolete) — Splinter Review
Please ignore the `dump()` statements :) They're a poor man's console.log which I used to verify that the constraints are populated correctly.

So the new stuff I pass to OT.initPublisher() is:

```js
OT.initPublisher($('.stream'), {
  videoSource: "browser",
  browserWindow: windowId, // This is passed by the consumer, privileged browser code in our case. It is optional.
  scrollWithPage: true // Optional.
});
```
The code snippet in comment 8 is wrong! :/

```js
OT.initPublisher($(".stream"), {
  videoSource: "browser",
  constraints: {
    // `browserWindow` is passed by the consumer, privileged browser code in our case.
    // It is optional.
    browserWindow: windowId, 
    scrollWithPage: true // Optional.
  }
});
```

So I missed the 'constraint' namespace here. I put it in there, because that way I needn't refactor the SDK code, just augment it in the Firefox-specific handler.

How does this look to you?
Flags: needinfo?(jonathan)
Thanks Mike, that is awesome.
Flags: needinfo?(jonathan)
By the way Mike, how can a get a "windowId"? You are mentioning a "privileged browser code", how can I do that?
Flags: needinfo?(mdeboer)
Another thing:
- can I assume safely that for a given Publisher instance you guys do not change the "videoSource" (it will stay set to "browser")
- can I assume you do not change the value of "scrollWithPage" after Publisher initialization

If the answers to both questions is yes I'll add a "switchAcquiredWindow" method to "Publisher" that takes a windowId as parameter and does the tracks replacement.
(In reply to Jonathan from comment #11)
> By the way Mike, how can a get a "windowId"? You are mentioning a
> "privileged browser code", how can I do that?

You can't at the moment, it's not easilly accessible atm. Right now it's exposed to Loop/ Hello content via `navigator.mozLoop.getActiveTabWindowId(function(err, windowId) {})`. There is no way for you to easily retrieve it.
However, when this becomes a prominent browser feature, it'll most likely work the same as window sharing where the user selects the window to share from a dropdown in the sharing doorhanger as part of the gUM flow, but then for tabs instead of windows.
That's why I noted the `browserId` as an optional constraint. For Loop/ Hello we have an advanced use case where we want to follow the user while switching tabs, so we make use of the optional constraint! This has the nice property that we can bypass the gUM approval doorhanger for better UX.
Flags: needinfo?(mdeboer)
(In reply to Jonathan from comment #12)
> Another thing:
> - can I assume safely that for a given Publisher instance you guys do not
> change the "videoSource" (it will stay set to "browser")

Yes.

> - can I assume you do not change the value of "scrollWithPage" after
> Publisher initialization

Yes.

> If the answers to both questions is yes I'll add a "switchAcquiredWindow"
> method to "Publisher" that takes a windowId as parameter and does the tracks
> replacement.

Sounds great!
Whiteboard: [screensharing]
backlog: Fx38+ → backlog+
Rank: 2
Flags: firefox-backlog+
Attached file webrtc-js-2.5.0.tgz
Attached the release version of OpenTok JS SDK v2.5.0.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 39.1 - 9 Mar
Comment on attachment 8573189 [details] [diff] [review]
Patch v1: update the OpenTok SDK to version 2.5.0 and enable Share My Tabs option

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

So replaceTrack is probably going to land today, so I think we could land this... or we can wait a little for that to go in. I guess a day or two for it to be partially not working isn't too bad.
Attachment #8573189 - Flags: review?(standard8) → review+
Given the most recent comments on bug 1136871 and we haven't yet resolved bug 1137634 (that I've just remembered):

I don't think we should land this with the enabling of share my tabs - that'll just confuse the testers that are excited to see it IMO (and then not be clear to them when it will work)

I do think we should land the sdk and get that out for general testing though.
As Mike is out today, I landed this without the enabling of sharing tabs - I'll split that out to a separate bug.

https://hg.mozilla.org/integration/fx-team/rev/84f76b49f7fe
Target Milestone: --- → mozilla39
Blocks: 1140313
https://hg.mozilla.org/mozilla-central/rev/84f76b49f7fe
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8573189 [details] [diff] [review]
Patch v1: update the OpenTok SDK to version 2.5.0 and enable Share My Tabs option

Approval Request Comment
[Feature/regressing bug #]: Loop/ Hello screensharing milestone
[User impact if declined]: User will see a new option that allows her/ him to share a window or Firefox tabs inside a room (aka. conversation).
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: minor
[String/UUID change made/needed]: n/a
Attachment #8573189 - Flags: approval-mozilla-aurora?
Comment on attachment 8573189 [details] [diff] [review]
Patch v1: update the OpenTok SDK to version 2.5.0 and enable Share My Tabs option

approving as part of the entire uplift of tab sharing feature.
Attachment #8573189 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.