Closed
Bug 1131568
Opened 10 years ago
Closed 10 years ago
Update OpenTok library to a version that supports browser sharing and Stream replacing
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(firefox38 fixed, firefox39 fixed)
backlog | backlog+ |
People
(Reporter: mikedeboer, Assigned: mikedeboer)
References
Details
(Whiteboard: [screensharing])
Attachments
(2 files, 1 obsolete file)
(deleted),
application/x-compressed-tar
|
Details | |
(deleted),
patch
|
standard8
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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-
Assignee | ||
Comment 1•10 years ago
|
||
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
Comment 2•10 years ago
|
||
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 → ---
Assignee | ||
Comment 3•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(msander)
Assignee | ||
Comment 4•10 years ago
|
||
(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/
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)
Assignee | ||
Comment 7•10 years ago
|
||
(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)
Assignee | ||
Comment 8•10 years ago
|
||
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.
});
```
Assignee | ||
Comment 9•10 years ago
|
||
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)
Comment 11•10 years ago
|
||
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)
Comment 12•10 years ago
|
||
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.
Assignee | ||
Comment 13•10 years ago
|
||
(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)
Assignee | ||
Comment 14•10 years ago
|
||
(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!
Updated•10 years ago
|
Whiteboard: [screensharing]
Updated•10 years ago
|
backlog: Fx38+ → backlog+
Rank: 2
Flags: firefox-backlog+
Comment 15•10 years ago
|
||
Attached the release version of OpenTok JS SDK v2.5.0.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 39.1 - 9 Mar
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8566026 -
Attachment is obsolete: true
Attachment #8573189 -
Flags: review?(standard8)
Comment 17•10 years ago
|
||
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+
Comment 18•10 years ago
|
||
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.
Comment 19•10 years ago
|
||
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
Comment 20•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Assignee | ||
Comment 21•10 years ago
|
||
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 22•10 years ago
|
||
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+
Updated•10 years ago
|
status-firefox38:
--- → affected
Assignee | ||
Comment 23•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•