1193075 - Allow setting & changing the viewport on gUM tab sharing.

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[:shell escalante]

Hello: we are looking to implement bug 1187857, for independent scrolling on a tab that is being co-browsed.

roughly we need to:
    set the viewport on a tab sharing
    render in one side and send the images
    a constraint on gUM and capture the tab at specific coordinates 

Brad: Currently there are two ways to initialize the MediaEngineTabVideoSource, one is to lock the stream to the scroll position of the user, the other is to specify a rect to stream. This is determined by gUM constraints. What we'll want to do is to have a way to 1) change from "scroll with page" to specifying a rect and 2) a way to change the rect.

So, essentially the bits that you need to do this are already in MediaEngineTabVideoSource, the work that is needed is on the WebRTC side of things in providing a way to change those settings/constraints on the fly. As far as I know, that doesn't exist yet.

Comment 1

[Alex Gouaillard.]

* if the origin and the size (in world coordinate) where provided, it s quite easy to modify the code to take an arbitrary rectangle:
248   nsRect r(0, 0, nsPresContext::CSSPixelsToAppUnits((float)innerWidth),
249            nsPresContext::CSSPixelsToAppUnits((float)innerHeight));

* however, the problem is that the size (innerWidth/innerHeight) is provided by the local window and not the remote window,
195   int32_t innerWidth, innerHeight;
196   win->GetInnerWidth(&innerWidth);
197   win->GetInnerHeight(&innerHeight);
so we have to add a flag that would allow to read that value from a local member instead for this use case.

* We have a risk that the remote windows and the local window are not sync in term of size or pixel ratio, but the class as no way to know about that.

* if we use the constraint to pass those values, we would have to create a new object every time we want to change the value as the constraints are only parsed in the Allocate method, which is not ideal. The best would be to have a Get/Set API on either an origin/size pair of members (each being a pair of coordinate), or on a single object with 4 coordinates (image processing and computer vision world approaches differ there).

* if there is no veto to the proposal I'll go ahead and implement the solution above. I have some questions though:
- How to test this? Is there a C++ test that is already instantiating TabCapture I could modify?
- Is there any macro I should leverage to implement the Get/Set method?
- Is there anything regarding XPCom and the runnable thingy I should be careful about?

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Alex Gouaillard. from comment #1)
> * if the origin and the size (in world coordinate) where provided, it s
> quite easy to modify the code to take an arbitrary rectangle:
> 248   nsRect r(0, 0, nsPresContext::CSSPixelsToAppUnits((float)innerWidth),
> 249            nsPresContext::CSSPixelsToAppUnits((float)innerHeight));
> 
> * however, the problem is that the size (innerWidth/innerHeight) is provided
> by the local window and not the remote window,
> 195   int32_t innerWidth, innerHeight;
> 196   win->GetInnerWidth(&innerWidth);
> 197   win->GetInnerHeight(&innerHeight);
> so we have to add a flag that would allow to read that value from a local
> member instead for this use case.

Sure - this is a generalization of the existing code for "capture a specific rectangle"; where the capture rectangle can be updated from JS - and the sizes are not tied to the window, but set externally.  (Currently the "fixed at getUserMedia time" version just sets the value of mScrollWithPage, but doesn't pass coordinates in.)

jib/abr: this touches on "what sort of interface should this have"?  And related, how security/privacy sensitive is this compared to normal tab capture?  

The "how do the values get set" is something that should be part of a separate patch (in the set to handle this bug); that one will focus on the DOM APIs/constraints for interfacing to this.  For the internal patch to the actual capture code, we need to make sure we support it being set at creation, and being updated.

> * We have a risk that the remote windows and the local window are not sync
> in term of size or pixel ratio, but the class as no way to know about that.

Not the problem of this code; that's the problem of the application.

> * if we use the constraint to pass those values, we would have to create a
> new object every time we want to change the value as the constraints are
> only parsed in the Allocate method, which is not ideal. The best would be to
> have a Get/Set API on either an origin/size pair of members (each being a
> pair of coordinate), or on a single object with 4 coordinates (image
> processing and computer vision world approaches differ there).

Almost all APIs internally use origin/width/height, so let's stick with that.  
 
> * if there is no veto to the proposal I'll go ahead and implement the
> solution above. I have some questions though:
> - How to test this? Is there a C++ test that is already instantiating
> TabCapture I could modify?

abr: is Loop/Hello running tab-capture tests in browser-chrome?

I don't think tab capture landed with any tests in dom/media/tests/mochitest.  Basic constraint support should be tested in test_getUserMedia_constraints.html, and capture in test_getUserMedia_basicScreenshare.html/basicWindowshare.html (probably by adding another, or merging them into one test that hits window/screen/tab capture - so long as we don't see a need to make some of those platform-specific and not others - Windowshare may be a little confusing on mobile, for example, though tab capture likely makes sense.

> - Is there any macro I should leverage to implement the Get/Set method?

Look at other similar uses.  Since this isn't an (.idl/.webidl) interface, it won't generate method definitions for you.

> - Is there anything regarding XPCom and the runnable thingy I should be
> careful about?

Oh, there's a TON of things to be careful about XPCOM.  :-)  Runnables: look at existing usage.  Ownership of references is the trickiest thing to be wary of.  See WrapRunnable() uses for how to avoid building little nsRunnable children all over for simple "call this method on that thread" uses.

Most basic pointer: nsCOMPtr<> is for nsIFoo's (IDL interfaces), nsRefPtr<> is for Foo's/nsFoo's (concrete classes).

Comment 3

[Alex Gouaillard.]

Ok, so if I understand correctly:

1. at initialization time (in Allocate where the constraints are parsed), add an additional option, just like mScrollWithPage, to indicate if the scrolling is controlled externally or not.

here we have two possible options:
a. require that origin/width/height be passed in the constraints
b. let the origin/width/height be passed by API only (it is needed in any case to be able to update "on the fly" later

if b, there will likely be a gap between the first draw() and the call to the setViewPort API. During that gap, we could use the local widow info by default.
as soon as we get both the mRemoteScrolling set to True, and a viewPortProvided, Draw() would use those to compute the image to be rendered locally and sent.

There are small security implications, as you give control to someone else over what is rendered and sent without you knowing. However, it is still limited to the content of the tab.

About "how this is set", there are two interpretation of this sentence: the local API that allows the viewport to be set, which, IMHO belongs to this bug, and how the information is sent from the remote host to us, which is not IMHO in the scope of this bug. Agreed?

Clear about the tests.

The questions about the existence of macro to define accessors was triggered by my not finding accessor explicitly defined for getinnerwidth() even though they obviously are there. So I thought they were defined by macro. My bad.

Comment 4

[Adam Roach [:abr]]

(In reply to Randell Jesup [:jesup] from comment #2)

> Sure - this is a generalization of the existing code for "capture a specific
> rectangle"; where the capture rectangle can be updated from JS - and the
> sizes are not tied to the window, but set externally.  (Currently the "fixed
> at getUserMedia time" version just sets the value of mScrollWithPage, but
> doesn't pass coordinates in.)
> 
> jib/abr: this touches on "what sort of interface should this have"?  And
> related, how security/privacy sensitive is this compared to normal tab
> capture?


Rather than adding new methods here, I think we probably want new constraints to feed to the existing API points. This would allow initial settings to be established in the getUserMedia() call, and subsequent settings to be communicated by calling applyConstraints() on the MediaStreamTrack.

I'll note that the MST constraints set already includes "width" and "height". On first blush, this seems like what we'd want to use to set width and height of the capture window:

http://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaTrackConstraintSet

We then have the issue of setting the offset. Given the way constraints are set up, the most consistent thing would be to define two variables; one for the horizontal offset, and a second for the vertical offset. I propose "originX" and "originY".

In terms of security and privacy, tab capture is highly problematic already -- which is why it's not exposed to content (at least, not yet). With the "fixed window" constraint, we already have the ability to send to a remote party content other than that being viewed locally, so we've already opened that little corner of Pandora's box. Being able to select the viewport, of course, increases the exposure here, but only incrementally.

In practice, I think we don't really need to worry about these issues until we go through a more rigorous analysis around how to expose tab sharing to content code at all.

> > * We have a risk that the remote windows and the local window are not sync
> > in term of size or pixel ratio, but the class as no way to know about that.
> 
> Not the problem of this code; that's the problem of the application.

Yep.

> 
> > * if we use the constraint to pass those values, we would have to create a
> > new object every time we want to change the value as the constraints are
> > only parsed in the Allocate method, which is not ideal.

http://w3c.github.io/mediacapture-main/getusermedia.html#dfn-applyconstraints

I think the applyConstraints() work is still underway for our codebase -- coordinate with jib on this (I'm setting ni? for jib so he can fill in details here)



> > - How to test this? Is there a C++ test that is already instantiating
> > TabCapture I could modify?
> 
> abr: is Loop/Hello running tab-capture tests in browser-chrome?

I could investigate this, but Mark or Dan should know off the top of their heads. Setting ni? to both of them (whoever answers can clear the other person's flag).

Comment 5

[Alex Gouaillard.]

Note from IRC: jib working on ApplyConstraints():
https://bugzilla.mozilla.org/show_bug.cgi?id=912342
We might want to wait on it.

Comment 6

[Adam Roach [:abr]]

(In reply to Alex Gouaillard. from comment #5)
> Note from IRC: jib working on ApplyConstraints():
> https://bugzilla.mozilla.org/show_bug.cgi?id=912342
> We might want to wait on it.

I think we'd want to land the "set viewport at stream creation time" part, and then have a follow-up bug to allow live modification that blocks on 912342.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I agree with Adam in comment 4 about offsetX/offsetY constraints being the way to go here. That means there's work here to get that working with getUserMedia for initial values, which I think can happen in parallel with Bug 912342.

All screensharing constraints are nicely orthogonal to each other, which means we side-step the full algorithm using FlattenedConstraints [1].

As for testing, test_getUserMedia_constraints.html unfortunately includes only the most rudimentary tests, little more than exercising the code, due to Bug 1088621. Screen-sharing code OTOH works without hardware AFAIK, hence should be more testable in the tree, but if I search for scrollWithPage it seems to pop up in loop-specific tests, so that's where I would put new tests here [2].  

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineTabVideoSource.cpp?rev=0a8484884b6b#134
[2] http://mxr.mozilla.org/mozilla-central/search?string=scrollWithPage&find=&findi=&filter=&hitlimit=&tree=mozilla-central

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #7)
> so that's where I would put new tests here [2].  

Or not. I'm not opposed to adding general tests of screensharing constraints outside of loop either.

Comment 9

[Mark Banner (:standard8)]

(In reply to Adam Roach [:abr] from comment #4)
> > > - How to test this? Is there a C++ test that is already instantiating
> > > TabCapture I could modify?
> > 
> > abr: is Loop/Hello running tab-capture tests in browser-chrome?
> 
> I could investigate this, but Mark or Dan should know off the top of their
> heads. Setting ni? to both of them (whoever answers can clear the other
> person's flag).

We don't have any of these tests currently. I'd like to get a basic one to check enabling of tab sharing running for our functional tests, but that's waiting on other things at the moment.

Comment 10

[Alex Gouaillard.]

Attachment: WIP: first patch for the bug. Might be complete, compiles, tested.

Compiles. Should do the work. Minimum upgrade to the mochitest to expect the new fields, but the behavior hasn't been tested.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8649234 [details] [diff] [review]
WIP: first patch for the bug. Might be complete, compiles, tested.

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

Overall the tab sharing code could use some more comments about how it works, so feel free to add some.

Another thing to consider is that the existing tab sharing constraints are all sender-side and presumably affects what's sent, whereas here we're talking about constraints receiver-side.

As discussed on IRC, manual testing seems to be the next step and should reveal more.

::: dom/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +130,5 @@
>                aConstraints.mBrowserWindow.Value() : -1;
>    mScrollWithPage = aConstraints.mScrollWithPage.WasPassed() ?
>                      aConstraints.mScrollWithPage.Value() : true;
> +  mSetViewport = aConstraints.mSetViewport.WasPassed() ?
> +                 aConstraints.mSetViewport.Value() : false;

Please add setViewport to the comment directly above about these not being proper constraints.

@@ +135,5 @@
>  
>    FlattenedConstraints c(aConstraints);
>  
> +  if( mSetViewport ) {
> +    // suppose there is no overflow

Not sure what is meant by this comment. Can you elaborate?

@@ +137,5 @@
>  
> +  if( mSetViewport ) {
> +    // suppose there is no overflow
> +    mOriginX = c.mOriginX.Clamp(DEFAULT_TABSHARE_VIDEO_MAX_WIDTH);
> +    mOriginY = c.mOriginY.Clamp(DEFAULT_TABSHARE_VIDEO_MAX_HEIGHT);

If no originX/Y constraints are passed in, mOriginX/Y is set to the browser's default width/height here (General numeric constraints are implemented as bounds around some browser and/or hardware default). Is that what we want?

I'm thinking that for originX/Y, this bounds model doesn't make much sense (since I understand from jesup this rectangle is virtual anyways), so in the spirit of how we've added other tab sharing settings, I think we should just implement them as long for now. Simpler and more reflective of what we support.

@@ +140,5 @@
> +    mOriginX = c.mOriginX.Clamp(DEFAULT_TABSHARE_VIDEO_MAX_WIDTH);
> +    mOriginY = c.mOriginY.Clamp(DEFAULT_TABSHARE_VIDEO_MAX_HEIGHT);
> +  } else {
> +    mOriginX = 0;
> +    mOriginY = 0;

Is this all the setViewport boolean does? Is there a case where scrollWithPage is false where offsets should NOT work?

setViewport seems redundant to me if we default offsetX/Y to 0.

@@ +213,5 @@
>  
>    float pixelRatio;
>    win->GetDevicePixelRatio(&pixelRatio);
> +  const int viewportWidth  = innerWidth  - mOriginX;
> +  const int viewportHeight = innerHeight - mOriginY;

Here the viewport shrinks as the offsets increase, which seems scary to me, though there may be parts of how this is ultimately rendered (and sent) that I'm not following.

As discussed in IRC lets look at this again after some testing to make sure this works.

::: dom/media/webrtc/MediaEngineTabVideoSource.h
@@ +78,5 @@
>      int mBufWidthMax;
>      int mBufHeightMax;
>      int64_t mWindowId;
>      bool mScrollWithPage;
> +    bool mSetViewport;;

nit: extra ;

::: dom/webidl/MediaTrackConstraintSet.webidl
@@ +39,1 @@
>      boolean scrollWithPage;

We might get into trouble someday for only implementing the simple "set" nature of constraints here (e.g. boolean rather than ConstrainBoolean) - like when we add capabilities - but that's for another bug since the same applies to existing browserWindow, scrollWithPage (and even mediaSource).

With orthogonal settings like this, implementing anything more than this subset is arguably redundant.

::: dom/webidl/MediaTrackSupportedConstraints.webidl
@@ +29,4 @@
>      boolean mediaSource = true;
>  
>      // Experimental https://bugzilla.mozilla.org/show_bug.cgi?id=1131568#c3
> +    //              https://bugzilla.mozilla.org/show_bug.cgi?id=1193075

Since there's no spec for this, a link directly to some explanation of the feature would be best. If there's no good link, an explanation here in the comment would work.

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

> whereas here we're talking about constraints receiver-side.

Scratch that, as from re-reading comment 0 the setting of viewport seems to be meant to be a sender-side constraint that affects what subset is sent (rather than sending everything).

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

From chatting with abr on irc today I have a clearer picture: Implementation-wise it's "guest scrolling" on the sender's side of a secondary rendered view dedicated to the peerConnection that's not seen by the sender. What's sent is what's seen by the viewer. It's up to the app (Hello) to use data channels or something to get the viewer's origin offsets to the sender side where they're set as constraints.

But it seems we need one more piece of information: viewerWidth and viewerHeight.

Today in scrollWithPage mode (the default), the size of the area shared is the always the size of the area seen by the sender. i.e. even when the viewer has a much bigger higher-resolution window, they can't see more area than the sender. But once the viewer can scroll independently, they can already see beyond what the sender sees at the moment, so limiting them to the sender's width and height seems artificial.

Comment 14

[Romain Testard [:RT]]

Shell, this is dependency for us to implement independent scrolling and we're trying to understand whether there is a chance we can implement independent scrolling with 44.
Can you please clarify if this is scheduled to be addressed in the 44 time-frame?

Comment 15

[:shell escalante]

Hi Maire, 
Hi Jan-Ivar,

Are there plans to take this from WIP patch to release patch in the near future?  we're looking to utilize in 44.  Is there something needed from Hello team feedback?

Comment 16

[Maire Reavy [:mreavy]]

I talked with Alex and Jib, and Jib is going to start working on this bug as soon as Jib's applyConstraints work is done -- which should wrap up in the next week or so.  So I expect we'll be able to land support for this in the Fx44 Nightly cycle.

Comment 17

[Romain Testard [:RT]]

Hi Maire, is this still fair to assume that this will be done in the Fx44 Nightly cycle?
We're currently assuming to implement independent scrolling in Hello in the Fx45 cycle and it will depend on it so I'm checking if my current plan is feasible.

Comment 18

[Maire Reavy [:mreavy]]

We're still targeting Firefox 44.  There were some regressions affecting Dev Edition and Beta that we needed to deal with, but they are landed. This is jib's next focus.

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1193075 - add viewport constraints for independent scrolling in tab sharing

Bug 1193075 - add viewport constraints for independent scrolling in tab sharing

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

So is there a spec for the new dictionary properties, or at least a spec bug?

I don't see anything initializing the new member variables of MediaEngineTabVideoSource.
So, initialize to 0 in ctor?

Comment 21

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Those variables, like the existing mScrollWithPage and mWindowId are instead initialized in ::Start. Is that not sufficient?

These constraints, including their existing siblings scrollWithPage and browserWindow, are driven by the Hello application needing to implement tab sharing. The constraints are currently limited to chrome applications unless a user sets media.navigator.permission.disabled=false in about:config AND add their domain to media.getusermedia.screensharing.allowed_domains

Further complicating this, our screen sharing API doesn't yet follow the latest spec in this area [1].

Instead it follows an earlier (mostly abandoned) incarnation [2]. Which one should I file a spec bug on?

[1] http://w3c.github.io/mediacapture-screen-share/
[2] http://fluffy.github.io/w3c-screen-share/#screen-based-video-constraints/

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Start is guaranteed to be called?

Shouldn't we file spec bugs on [1] so that we don't need any prefs?
But if the Gecko specific constraints are behind prefs, fine.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8679252 [details]
MozReview Request: Bug 1193075 - add viewport constraints for independent scrolling in tab sharing

r+ given that the new webidl stuff isn't really exposed to the web in general.

Comment 24

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8679252 [details]
MozReview Request: Bug 1193075 - add viewport constraints for independent scrolling in tab sharing

Bug 1193075 - add viewport constraints for independent scrolling in tab sharing

Comment 25

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Try seems busted, seeing if a rebase will fix it.

Comment 26

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Olli Pettay [:smaug] from comment #22)
> Start is guaranteed to be called?

No, but it is guaranteed to be called before any action on these variables. 

> Shouldn't we file spec bugs on [1] so that we don't need any prefs?

Yes we should, though the pref also has to do with the security implications of screen-sharing an active browser still being worked out.

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8679252 [details]
MozReview Request: Bug 1193075 - add viewport constraints for independent scrolling in tab sharing

https://reviewboard.mozilla.org/r/23403/#review20901

::: dom/media/MediaManager.cpp:1883
(Diff revision 2)
> +          vc.mBrowserWindow.Construct(outer->WindowID());

Add a comment explaining what case this covers

::: dom/media/webrtc/MediaTrackConstraints.h:88
(Diff revision 2)
> +  , mViewportHeight(aOther.mViewportHeight, advanced){}

nit: space before {}

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #26)
> (In reply to Olli Pettay [:smaug] from comment #22)
> > Start is guaranteed to be called?
> 
> No, but it is guaranteed to be called before any action on these variables. 

Then it is more future proof to initialize the variables to some value (0?). Random values just
tend to make debugging harder.

Comment 29

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Sorry, make that s/Start/Allocate/. I'll initialize them in the constructor.

Comment 30

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8679252 [details]
MozReview Request: Bug 1193075 - add viewport constraints for independent scrolling in tab sharing

Bug 1193075 - add viewport constraints for independent scrolling in tab sharing

Comment 31

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8679252 [details]
MozReview Request: Bug 1193075 - add viewport constraints for independent scrolling in tab sharing

Incorporated feedback: initialize all native types to 0 values (also removed redundant null initialization in two cases for smart pointer types).

Carrying forward r=smaug, jesup.

Comment 32

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Usage recap:

To use the demo https://jsfiddle.net/jib1/uax4gom5/ , first so this:

 1. In about:config, set media.navigator.permission.disabled = false
 2. Add ",*.jshell.net" to media.getusermedia.screensharing.allowed_domains

Hit the [Start] button, and then wiggle the sliders to scroll the content horizontally or vertically.

Two known bugs:

1) You must hit the [Stop] button before refreshing or closing the page, to avoid an assert.

2) nsIPresShell::RENDER_IGNORE_VIEWPORT_SCROLLING doesn't seem to work, so user-scrolling still registers.

I'm filing bugs on these next.

Comment 33

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #32)
>  1. In about:config, set media.navigator.permission.disabled = false

Uh, I meant set media.navigator.permission.disabled = true

Comment 34

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1193075 - add tab-sharing test.

Bug 1193075 - add tab-sharing test.

Comment 36

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8679778 [details]
MozReview Request: Bug 1193075 - add tab-sharing test.

https://reviewboard.mozilla.org/r/23537/#review21013

::: dom/media/tests/mochitest/test_getUserMedia_basicTabshare.html:30
(Diff revision 1)
> +        fake: false

Is it necessary to set `fake: false`?

::: dom/media/tests/mochitest/test_getUserMedia_basicTabshare.html:47
(Diff revision 1)
> +          }))

Can we somehow tests that the stream still plays after these constraints have been applied?

Comment 37

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1193075 - test that streams still play after constraints have been applied.

Bug 1193075 - test that streams still play after constraints have been applied.

Comment 38

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8680186 [details]
MozReview Request: Bug 1193075 - test that streams still play after constraints have been applied.

https://reviewboard.mozilla.org/r/23577/#review21113

::: dom/media/tests/mochitest/mediaStreamPlayback.js:67
(Diff revision 1)
> +    var lastStreamTime = this.mediaStream.currentTime;

`mediaStream.currentTime` is not part of the spec so we shouldn't rely on it.

::: dom/media/tests/mochitest/mediaStreamPlayback.js:75
(Diff revision 1)
> +                   CANPLAYTHROUGH_TIMEOUT_LENGTH, "verifyPlaying timed out")

Create a new `VERIFYPLAYING_TIMEOUT_LENGTH` and remove the old `CANPLAYTHROUGH_TIMEOUT_LENGTH` and `TIMEUPDATE_TIMEOUT_LENGTH` if they're no longer used.

::: dom/media/tests/mochitest/test_getUserMedia_basicTabshare.html:38
(Diff revision 1)
> +        return playback.verifyPlaying()

Please also double check for other tests using `startMedia()` and update them with `verifyPlaying()` if you haven't already.

::: dom/media/tests/mochitest/test_getUserMedia_basicTabshare.html:44
(Diff revision 1)
> +          .then(() => playback.verifyPlaying()) // still playing

A potential problem here could be that as we verifyPlaying() we return true because the changes applied have not yet been propagated to the MSG-thread, or for that matter back to the main thread.

I think the easiest solution to that in this case is to wait for onresize before verifyPlaying, since I assume this applyConstraints() causes a resize of the video.

Something like
`.then(() => Promise.all([ testVideo.srcObject.getVideoTracks()[0].applyConstraints()
                         , listenUntil(testVideo, "resize")
                         ]))`

Comment 39

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #38)
> `mediaStream.currentTime` is not part of the spec so we shouldn't rely on it.

It's already in the test here http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/mediaStreamPlayback.js?rev=29309e60a4bf&mark=98-98#64 so removing it seems orthogonal to this patch.

> A potential problem here could be that as we verifyPlaying() we return true
> because the changes applied have not yet been propagated to the MSG-thread,
> or for that matter back to the main thread.
> 
> I think the easiest solution to that in this case is to wait for onresize
> before verifyPlaying, since I assume this applyConstraints() causes a resize
> of the video.

There's no resize in this test, but I can probably add that.

>                          , listenUntil(testVideo, "resize")

Good idea. Will try it.

Comment 40

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8680186 [details]
MozReview Request: Bug 1193075 - test that streams still play after constraints have been applied.

Bug 1193075 - test that streams still play after constraints have been applied.

Comment 41

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1193075 - make { scrollWithPage: false } not scroll with page.

Bug 1193075 - make { scrollWithPage: false } not scroll with page.

Comment 42

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8680509 [details]
MozReview Request: Bug 1193075 - make { scrollWithPage: false } not scroll with page.

https://reviewboard.mozilla.org/r/23631/#review21125

Comment 43

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Last patch is based on Bug 1218851 comment 1.

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b53b26799619
https://hg.mozilla.org/integration/mozilla-inbound/rev/36e9f2bb1d22
https://hg.mozilla.org/integration/mozilla-inbound/rev/b737a84b9faf
https://hg.mozilla.org/integration/mozilla-inbound/rev/c03bf59e4061

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b53b26799619
https://hg.mozilla.org/mozilla-central/rev/36e9f2bb1d22
https://hg.mozilla.org/mozilla-central/rev/b737a84b9faf
https://hg.mozilla.org/mozilla-central/rev/c03bf59e4061

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b53b26799619
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/36e9f2bb1d22
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b737a84b9faf
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/c03bf59e4061

Comment 47

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion