1039666 - Basic tests for Desktop & Window Screensharing

Status: RESOLVED FIXED

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

Description

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

This bug is for basic tests for desktop screensharing; likely we'll want more complex ones later.

This is not for the UI per se

Comment 1

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

Attachment: basic tests for desktop screensharing

WIP but seems to work in local tests

Comment 2

[Nils Ohlmeier [:drno]]

Comment on attachment 8457130 [details] [diff] [review]
basic tests for desktop screensharing

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

Looks good as a very basic smoke test.

We should have more detailed tests (in separate BZ ticket depending on the feature bugs):
- switch from camera to screenshare
- switch from screenshare back to camera
- add a screenshare video stream to an existing PC
- remove a screenshare video stream from an existing PC
- screenshare and camera video in two separate PCs
- test the different constraints for desktop, application, browser and tab

::: dom/media/tests/mochitest/mochitest.ini
@@ +24,5 @@
>  [test_getUserMedia_basicAudio.html]
>  skip-if = (toolkit == 'gonk' && debug) # debug-only failure
>  [test_getUserMedia_basicVideo.html]
> +skip-if = (toolkit == 'gonk') # no screenshare on b2g
> +[test_getUserMedia_basicScreenshare.html]

skip-if need to under the new test case [] line
And we need to skip this on Android as well, or?

@@ +57,5 @@
>  skip-if = toolkit == 'gonk' # b2g(Bug 960442, video support for WebRTC is disabled on b2g)
>  [test_peerConnection_basicVideo.html]
>  skip-if = toolkit == 'gonk' # b2g(Bug 960442, video support for WebRTC is disabled on b2g)
> +[test_peerConnection_basicScreenshare.html]
> +skip-if = toolkit == 'gonk' # b2g(Bug 960442, video support for WebRTC is disabled on b2g)

Please remove or update the comment.
Again I think we need to skip Android as well.

::: dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html
@@ +39,5 @@
> +    getUserMedia(constraints, function (aStream) {
> +      checkMediaStreamTracks(constraints, aStream);
> +
> +      var playback = new LocalMediaStreamPlayback(testVideo, aStream);
> +      playback.playMedia(false, function () {

Looks like this actually internally stops the playback already:

http://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/mediaStreamPlayback.js#43

which calls this:

http://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/mediaStreamPlayback.js#160

which calls .pause()

Maybe it makes more sense to call playMediaWithStreamStop():
http://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/mediaStreamPlayback.js#196
instead?

::: dom/media/tests/mochitest/test_peerConnection_basicScreenshare.html
@@ +13,5 @@
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    bug: "796888",
> +    title: "Basic video-only peer connection"

Please update bug number and description.

Comment 3

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

Attachment: basic tests for desktop screensharing

Comment 4

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

Attachment: basic tests for desktop screensharing

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8458061 [details] [diff] [review]
basic tests for desktop screensharing

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

LGTM with small NITs.

::: dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html
@@ +40,5 @@
> +      checkMediaStreamTracks(constraints, aStream);
> +
> +      var playback = new LocalMediaStreamPlayback(testVideo, aStream);
> +      playback.playMediaWithStreamStop(false, function () {
> +        aStream.stop();

Sorry for not being precise enough in my last review: if you use playMediaWithStreamStop() I think you no longer need to call .stop() yourself. Just the .finish() call in the success callback should be enough.

::: dom/media/tests/mochitest/test_peerConnection_basicWindowshare.html
@@ +12,5 @@
> +<body>
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    bug: "1038926",

Is there a reason why the PC tests refer to other bugs then the gUM tests?
I would prefer them to be aligned (with no preference for which number).

Comment 6

[Maire Reavy [:mreavy]]

The latest patch has tests for Window sharing as well.  Needs to land with or after the Window sharing bug (Bug 1038926).

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/727bd16b29d3
FYI, I realized I didn't address the last nits here; I'll land that as a followup under the existing r+.

For the entire landing: https://tbpl.mozilla.org/?tree=Try&rev=c9bd8d240a61

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/727bd16b29d3

Comment 9

[Nils Ohlmeier [:drno]]

Sorry for not spotting this at my review, but these tests actually do not test screen or window sharing. They simply just send a regular fake video stream, because the media constraints in these test are for Google Chrome.

Comment 10

[Nils Ohlmeier [:drno]]

Attachment: bug_1039666_fix_screen_share_test.patch

This should hopefully fix the tests. It should have the correct media constraints and sets the permissions before running the test.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=24e7fd007e88

Comment 11

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

Comment on attachment 8459948 [details] [diff] [review]
bug_1039666_fix_screen_share_test.patch

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

With the same changes duplicated in to the getusermedia screen/window share tests, r+.  Thanks!

Comment 12

[Nils Ohlmeier [:drno]]

Attachment: bug_1039666_fix_screen_share_test.patch

This patch also fixes the gUM screen share tests.

Carrying forward r+=jesup

Try run: https://tbpl.mozilla.org/?tree=Try&rev=52c85cfbbc49

Comment 13

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

Attachment: Enable Screen/windowsharing tests for all but OSX 10.6 and XP

Comment 14

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

Attachment: Disable screen/windowsharing for OSX 10.6 and WinXP

Comment 15

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

Comment on attachment 8465857 [details] [diff] [review]
Disable screen/windowsharing for OSX 10.6 and WinXP

10.6 screen sharing crashes (window is ok).  WinXP window sharing works; screen sharing works for some people; but on tbpl the tests never exit (VM issues?)  Disabling for both for now.

Comment 16

[Nils Ohlmeier [:drno]]

Comment on attachment 8465856 [details] [diff] [review]
Enable Screen/windowsharing tests for all but OSX 10.6 and XP

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

I don't quite understand why we need a new patch for this, as it seems to implement exactly the same functionality as attachment 8460064 [details] [diff] [review] did already.
And I'm not a big fan of enabling the screen sharing on the global level in head.js, as we now won't have any way to distinguish on the receiving side if we are receiving a screen share or the video feed.

But at least this tests the screen sharing properly now.

Comment 17

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/801a74e1dfdd
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c12508e3b69

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4659598fe4
fix for suppressing on b2g with the wrong skip-if (toolkit instead of buildapp)

Comment 19

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

Attachment: suppress 'nonqueued' IPC assertions (bug 1047842) in window/screensharing r=

Comment 20

[Ed Morley [:emorley]]

Unfortunately I had to revert https://hg.mozilla.org/integration/mozilla-inbound/rev/4c12508e3b69 since it conflicted with changes blassey landed on fx-team, that had merged to mozilla-central ahead of this (bug 1037424). This can reland after a rebase (this was one of several conflicts during today's merges; didn't seem worth the risk of me resolving in a hurry):

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a28657e4996

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b50533a4472c

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/801a74e1dfdd
https://hg.mozilla.org/mozilla-central/rev/cd4659598fe4
https://hg.mozilla.org/mozilla-central/rev/9b2e7a87fac5
https://hg.mozilla.org/mozilla-central/rev/b50533a4472c

Comment 23

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

Comment on attachment 8465856 [details] [diff] [review]
Enable Screen/windowsharing tests for all but OSX 10.6 and XP

Note: this applies to the full set of patches here (minus any that were in the 33 uplift from nightly)

Approval Request Comment
[Feature/regressing bug #]: screensharing

[User impact if declined]: no test coverage on Aurora

[Describe test coverage new/current, TBPL]: This is for the test coverage, plus disabling the feature on 10.6/XP which have problems.

[Risks and why]: Very low risk; only browser code changing is the disables.

[String/UUID change made/needed]: none

Comment 24

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/11f3be6168d5
https://hg.mozilla.org/releases/mozilla-aurora/rev/b62866c074ea
https://hg.mozilla.org/releases/mozilla-aurora/rev/d606f42b8372
https://hg.mozilla.org/releases/mozilla-aurora/rev/81631d5c98b4