1450954 - getUserMedia reports incorrect track settings when requesting screen with single dimension constraint

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

+++ This bug was initially created as a clone of Bug #1449832 +++

When requesting screen media with a single dimension in the constraints:
> {video: {mediaSource: 'screen', height: 720}}

the resulting video track reports incorrect width and height from getSettings
> {width: 65535, height: 47251455}


When requesting screen media with no constraints:
> {video: {mediaSource: 'screen'}}

the resulting video track reports different width and height from getSettings
> {width: 65535, height: 65535}


In 58, we reported
> {width: 0, height: 0}
until the first frame had arrived and the screen resolution was known.

Then in 59, behavior changed when a resolution constraint was given. If the height was constrained to 720 like above we'd return
> {width: 0, height: 720}
until the first frame arrived.

Demo: https://codepen.io/brianpeiris/pen/NYydvb?editors=0010

Comment 1

[Andreas Pehrson [:pehrsons]]

I tracked this to a changeset [1] in bug 1434946 that added back some code to update the settings after having chosen capability. What it didn't do was add back some code to undo a nasty hack for screensharing constraints that was added in bug 1359662 and later removed.


[1] https://hg.mozilla.org/mozilla-central/rev/cf134b262f6a

Comment 2

[Andreas Pehrson [:pehrsons]]

pre-fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b2e084866dc516886b182c8c7a50c6803c2ebc8
post-fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f96168c06458ca41b1ed0d4a64ac5fe1ce47ddff

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1450954 - Test that a screenshare track's getSettings return sane dimensions.

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

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1450954 - Add back code to undo screenshare constraints hack.

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

Comment 5

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

Comment on attachment 8966572 [details]
Bug 1450954 - Test that a screenshare track's getSettings return sane dimensions.

https://reviewboard.mozilla.org/r/235302/#review241212

::: dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html:107
(Diff revision 1)
>      let stream = await getUserMedia({
>        video: {mediaSource: "screen"},
>        fake: false,
>      });
> +    is(stream.getTracks()[0].getSettings().width, 0,
> +       "Width setting should be 0 until first frame is known");
> +    is(stream.getTracks()[0].getSettings().height, 0,
> +       "Height setting should be 0 until first frame is known");

"Should" is questionable. The fact that we don't know the dimensions sooner seems like a limitation of our implementation.

Maybe add a comment that this isn't to spec and open a bug?

Also, do we actually guarantee frames won't arrive before the success callback runs, or could this be an intermittent?

::: dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html:107
(Diff revision 1)
>      let stream = await getUserMedia({
>        video: {mediaSource: "screen"},
>        fake: false,

fake: false

is redundant here now I believe.

::: dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html:157
(Diff revision 1)
> +    ok(settings.width >= 10 && settings.width <= 100,
> +       "Width setting should be within constraints");
> +    ok(settings.height >= 10 && settings.height <= 100,
> +       "Height setting should be within constraints");

Maybe break up && into separate ok()s, or emit actual value in log message?

::: dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html:161
(Diff revision 1)
> +    is(settings.width, testVideo.videoWidth,
> +       "Width setting should match video width");
> +    is(settings.height, testVideo.videoHeight,
> +       "Height setting should match video height");

Would it be worthwhile to test that the aspect matches that of the first unconstrained gUM call?

::: dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html:170
(Diff revision 1)
>      await Promise.all([
>        testVideo.srcObject.getVideoTracks()[0].applyConstraints({
>          mediaSource: 'screen',
>          width: 200,
>          height: 200,
>          frameRate: {
>            min: '5',
>            max: '10'
>          }
> +      })
> +      .then(() => {
> +        is(stream.getTracks()[0].getSettings().width, 200,
> +           "Width setting should be same as ideal constraint before first frame");
> +        is(stream.getTracks()[0].getSettings().height, 200,
> +           "Heightsetting should be same as ideal constraint before first frame");
>        }),
>        haveEvent(testVideo, "resize", wait(5000, new Error("Timeout"))),

Mix of await and .then(). How about:

    let p = haveEvent(testVideo, "resize", wait(5000, new Error("Timeout")));

    await testVideo.srcObject.getVideoTracks()[0].applyConstraints({...});
    is(stream.getTracks()[0].getSettings().width, 200, ...);
    is(stream.getTracks()[0].getSettings().height, 200, ...),

    await p;

?

Also, what does "first frame" mean with applyConstraints?

Again, this is probably [1] not spec compliant, and deserves a comment.

[1] https://github.com/w3c/mediacapture-main/issues/470#issuecomment-310921337

::: dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html:193
(Diff revision 1)
> +    is(newSettings.width, testVideo.videoWidth,
> +       "Width setting should match video width");
> +    is(newSettings.height, testVideo.videoHeight,
> +       "Height setting should match video height");

Maybe check that settings and newSettings have the same aspect?

Comment 6

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

Comment on attachment 8966573 [details]
Bug 1450954 - Add back code to undo screenshare constraints hack.

https://reviewboard.mozilla.org/r/235304/#review241216

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:318
(Diff revision 1)
> +        // Undo the hack where ideal and max constraints are crammed together
> +        // in mCapability for consumption by low-level code. We don't actually
> +        // know the real resolution yet, so report min(ideal, max) for now.

Maybe add a

    //TODO: bug #

here also, about someday fixing this to find out the source dimensions ahead of time?

Comment 7

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8966572 [details]
Bug 1450954 - Test that a screenshare track's getSettings return sane dimensions.

https://reviewboard.mozilla.org/r/235302/#review241212

> "Should" is questionable. The fact that we don't know the dimensions sooner seems like a limitation of our implementation.
> 
> Maybe add a comment that this isn't to spec and open a bug?
> 
> Also, do we actually guarantee frames won't arrive before the success callback runs, or could this be an intermittent?

It was an intermittent indeed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f96168c06458ca41b1ed0d4a64ac5fe1ce47ddff

I'm relaxing the check a bit.

Filed bug 1453247.

> Would it be worthwhile to test that the aspect matches that of the first unconstrained gUM call?

Seems reasonable. It's basically testing bug 1449832 though, so I'll make it a blocker.

> Mix of await and .then(). How about:
> 
>     let p = haveEvent(testVideo, "resize", wait(5000, new Error("Timeout")));
> 
>     await testVideo.srcObject.getVideoTracks()[0].applyConstraints({...});
>     is(stream.getTracks()[0].getSettings().width, 200, ...);
>     is(stream.getTracks()[0].getSettings().height, 200, ...),
> 
>     await p;
> 
> ?
> 
> Also, what does "first frame" mean with applyConstraints?
> 
> Again, this is probably [1] not spec compliant, and deserves a comment.
> 
> [1] https://github.com/w3c/mediacapture-main/issues/470#issuecomment-310921337

Fixed, and filed bug 1453259.

Comment 8

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8966572 [details]
Bug 1450954 - Test that a screenshare track's getSettings return sane dimensions.

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

Comment 9

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8966573 [details]
Bug 1450954 - Add back code to undo screenshare constraints hack.

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

Comment 10

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8966572 [details]
Bug 1450954 - Test that a screenshare track's getSettings return sane dimensions.

These changes were quite significant, please take another look.

Comment 11

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

Comment on attachment 8966572 [details]
Bug 1450954 - Test that a screenshare track's getSettings return sane dimensions.

https://reviewboard.mozilla.org/r/235302/#review241350

Comment 12

[Andreas Pehrson [:pehrsons]]

pre-fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0230526722d1505f37de462c21ec43619d9d784
post-fix (without the aspect ratio checks as they depend on bug 1449832): https://treeherder.mozilla.org/#/jobs?repo=try&revision=53ffa082c7d0ff0e794a4eef7c4961826a6d7294

I'll land the fix now so it can be uplifted early; and the test later as it's dependent on bug 1449832. ni? to myself as a reminder for this.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d03ca6d705b
Add back code to undo screenshare constraints hack. r=jib

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3d03ca6d705b

Comment 15

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8966573 [details]
Bug 1450954 - Add back code to undo screenshare constraints hack.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1434946
[User impact if declined]: Wrong settings reported to js for a screen capture track.
[Is this code covered by automated tests?]: It will be once bug 1449832 and the test on this bug lands.
[Has the fix been verified in Nightly?]: I verified it myself just now.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, see comment 0.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It adds back some old, stable, code. It is pretty trivial too.
[String changes made/needed]: None

Do note to not uplift the test patch at this time, it depends on bug 1449832.

Comment 16

[Andreas Pehrson [:pehrsons]]

Still need a reminder for the test.

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8966573 [details]
Bug 1450954 - Add back code to undo screenshare constraints hack.

webrtc regression fix, approved for 60.0b13

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0cf37de4a14
Test that a screenshare track's getSettings return sane dimensions. r=jib

Comment 19

[Andreas Pehrson [:pehrsons]]

The test is now ready for uplift here too. Since the fix has not been uplifted yet, they can go up together.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bf6f0553eaeb
https://hg.mozilla.org/releases/mozilla-beta/rev/9f8ac6bbbd47

Comment 21

[Noemi Erli[:noemi_erli]]

Backed out changeset d0cf37de4a14 (bug 1450954) for failing in dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html on a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d0cf37de4a14f99dd13b422ea752f565a8f91e68&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=173906018

Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=173906018&repo=mozilla-inbound&lineNumber=2352

Backout push: https://hg.mozilla.org/integration/mozilla-inbound/rev/970da641be1aa033bd3aa5f6b928de7bb75b6d22

Comment 22

[Ryan VanderMeulen [:RyanVM]]

I backed out the test change from Beta as well for the same reason. Really frequent failures :|
https://hg.mozilla.org/releases/mozilla-beta/rev/1c12e0428ff13c0b07a612d48f7043e4931e74c1

Comment 23

[Andreas Pehrson [:pehrsons]]

Sorry about this. It looks like a race between updating the settings and pushing a frame through to the video element. I have another patch in the works, and I'll make sure to run it through the test-verify job this time too.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db38d1dfdb81
Test that a screenshare track's getSettings return sane dimensions. r=jib

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f024ae9a5a4

Comment 26

[Andreas Pehrson [:pehrsons]]

Thanks for the quick uplift!

Comment 27

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/db38d1dfdb81