1380346 - Improve screenshare testing

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, enhancement, P3)

Description

[Dan Minor [:dminor]]

As per https://bugzilla.mozilla.org/show_bug.cgi?id=1379743#c5 we could do a lot better with our screenshare testing.

Comment 1

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

Should be doable to write a test that opens e.g. a black window then overtake it with a red window and check for pixel updates (after bug 1395289 I'd say in four quadrants of the capture).

Comment 2

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

We can do something similar to tests we already have for checking color here [1]

[1] https://dxr.mozilla.org/mozilla-central/rev/db7f19e26e571ae1dd309f5d2f387b06ba670c30/dom/media/tests/mochitest/test_peerConnection_replaceVideoThenRenegotiate.html#72

Comment 3

[Andreas Pehrson [:pehrsons]]

Agree. We can make a 4x4 canvas cover the entire viewport - then check a couple of pixels in each quadrant (they have different colors) in the screenshare to see that we get what we expect.

I can take this.

Comment 4

[Dan Minor [:dminor]]

There are also webrtc.org screensharing unit tests. They are not currently enabled, I'll file a separate bug to get them running.

Comment 5

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1380346 - Modernize test_gUM_basicScreenShare.html.

Review commit: https://reviewboard.mozilla.org/r/179892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179892/

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1380346 - CaptureStreamTestHelper shouldn't need a full-size canvas to extract a pixel.

Our previous strategy was to draw the *entire* video we wanted a pixel from to
a canvas, then extract the pixel we wanted from that. This was all right for
most of our cases where the video was sufficiently small.

However, drawImage is more powerful than that -- we really just need a 1x1
canvas and draw the pixel we want to look at straight in.

Review commit: https://reviewboard.mozilla.org/r/179894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179894/

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1380346 - Let CaptureStreamTestHelper2D.drawColor draw squares wherever you want.

It was supporting a simpler case of only drawing in the upper left corner of
the input canvas. This supports that by default still, but also allows the
caller to exactly specify coordinates and size of the rectangle to draw.

Review commit: https://reviewboard.mozilla.org/r/179896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179896/

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1380346 - Verify screensharing content in mochitest.

This verifies that screen content is captured correctly by drawing to a canvas
that is full screen and comparing to pixels in the captured stream.

Note that going fullscreen requires the tab (and window) to be in the foreground
and having focus.

Review commit: https://reviewboard.mozilla.org/r/179898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179898/

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1380346 - Apply applyConstraints() properly.

Review commit: https://reviewboard.mozilla.org/r/179904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179904/

Comment 11

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

Comment on attachment 8908225 [details]
Bug 1380346 - Modernize test_gUM_basicScreenShare.html.

https://reviewboard.mozilla.org/r/179892/#review187772

::: dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html:62
(Diff revision 1)
> +    await Promise.all([
> +      () => testVideo.srcObject.getVideoTracks()[0].applyConstraints({
>          mediaSource: 'screen',
>          width: 200,
>          height: 200,
>          frameRate: {
>            min: '5',
>            max: '10'
>          }
> -      }
> -    ];
> -    return Promise.resolve()
> +      }),
> +      () => listenUntil(testVideo, "resize", () => true),
> +    ]);

This is good. Another pattern I like is:

    let p = listenUntil(testVideo, "resize", () => true);
    await testVideo.srcObject.getVideoTracks()[0].applyConstraints(c);
    await p;

I go back and forth on which is more readable.

Comment 12

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

Comment on attachment 8908226 [details]
Bug 1380346 - CaptureStreamTestHelper shouldn't need a full-size canvas to extract a pixel.

https://reviewboard.mozilla.org/r/179894/#review187776

::: dom/canvas/test/captureStream_common.js:62
(Diff revision 1)
>     * |video| as an array of the pixel's color channels: [R,G,B,A].  Allows
>     * optional scaling of the drawImage() call (so that a 1x1 black image

scaling is no longer optional afaict

::: dom/canvas/test/captureStream_common.js:66
(Diff revision 1)
> -  getPixel: function (video, offsetX, offsetY, width, height) {
> +  getPixel: function (video, offsetX, offsetY) {
>      offsetX = offsetX || 0; // Set to 0 if not passed in.
>      offsetY = offsetY || 0; // Set to 0 if not passed in.

use argument defaults to remove need for comment.

Comment 13

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

Comment on attachment 8908227 [details]
Bug 1380346 - Let CaptureStreamTestHelper2D.drawColor draw squares wherever you want.

https://reviewboard.mozilla.org/r/179896/#review187778

Comment 14

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

Comment on attachment 8908228 [details]
Bug 1380346 - Verify screensharing content in mochitest.

https://reviewboard.mozilla.org/r/179898/#review187780

\o/

::: commit-message-de597:6
(Diff revision 1)
> +Note that going fullscreen requires the tab (and window) to be in the foreground
> +and having focus.

maybe add this as a code comment as well?

::: dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html:36
(Diff revision 1)
> +        {dx: 0, dy: 0},
> +        {dx: 1, dy: 3},
> +        {dx: 8, dy: 5},
> +      ];
> +      for (let {dx, dy} of areaSamples) {
> +        let x = offsetX + dx + internalX; 

trailing space

Comment 15

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

Comment on attachment 8908243 [details]
Bug 1380346 - Apply applyConstraints() properly.

https://reviewboard.mozilla.org/r/179904/#review187788

Comment 16

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8908226 [details]
Bug 1380346 - CaptureStreamTestHelper shouldn't need a full-size canvas to extract a pixel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179894/diff/1-2/

Comment 17

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8908227 [details]
Bug 1380346 - Let CaptureStreamTestHelper2D.drawColor draw squares wherever you want.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179896/diff/1-2/

Comment 18

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8908228 [details]
Bug 1380346 - Verify screensharing content in mochitest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179898/diff/1-2/

Comment 19

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8908243 [details]
Bug 1380346 - Apply applyConstraints() properly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179904/diff/1-2/

Comment 20

[Andreas Pehrson [:pehrsons]]

Assertion failure on windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1cda02d4f1fae6c85e96f82ac13690299fca3c1

Looks like we're capturing the mouse cursor on the wrong thread.

Comment 21

[Andreas Pehrson [:pehrsons]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7a237b0925667fe2b71e13b4b92b87bdc056308

Comment 22

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8908225 [details]
Bug 1380346 - Modernize test_gUM_basicScreenShare.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179892/diff/1-2/

Comment 23

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8908226 [details]
Bug 1380346 - CaptureStreamTestHelper shouldn't need a full-size canvas to extract a pixel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179894/diff/2-3/

Comment 24

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8908227 [details]
Bug 1380346 - Let CaptureStreamTestHelper2D.drawColor draw squares wherever you want.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179896/diff/2-3/

Comment 25

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8908228 [details]
Bug 1380346 - Verify screensharing content in mochitest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179898/diff/2-3/

Comment 26

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8908243 [details]
Bug 1380346 - Apply applyConstraints() properly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179904/diff/2-3/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/455e69f2d765
Modernize test_gUM_basicScreenShare.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/7550c9bb04d7
CaptureStreamTestHelper shouldn't need a full-size canvas to extract a pixel. r=jib
https://hg.mozilla.org/integration/autoland/rev/7a17cc8d3069
Let CaptureStreamTestHelper2D.drawColor draw squares wherever you want. r=jib
https://hg.mozilla.org/integration/autoland/rev/5fd2e18c9007
Verify screensharing content in mochitest. r=jib
https://hg.mozilla.org/integration/autoland/rev/9498c22c35fe
Apply applyConstraints() properly. r=jib

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/455e69f2d765
https://hg.mozilla.org/mozilla-central/rev/7550c9bb04d7
https://hg.mozilla.org/mozilla-central/rev/7a17cc8d3069
https://hg.mozilla.org/mozilla-central/rev/5fd2e18c9007
https://hg.mozilla.org/mozilla-central/rev/9498c22c35fe