Closed Bug 1450954 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Version:
60 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 + fixed
firefox61 + fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1450954 - Test that a screenshare track's getSettings return sane dimensions.
59 bytes, text/x-review-board-request
jib
: review+
pehrsons
: checkin+
Details
Bug 1450954 - Add back code to undo screenshare constraints hack.
59 bytes, text/x-review-board-request
jib
: review+
jcristau
: approval-mozilla-beta+
pehrsons
: checkin+
Details 
+++ 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
Rank: 9
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
Blocks: 1359662, 1434946
See Also: → 1436104
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?
Attachment #8966572 - Flags: review?(jib) → review+
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?
Attachment #8966573 - Flags: review?(jib) → review+
See Also: → 1453247
See Also: → 1453259
Depends on: 1449832
See Also: 1449832 → 
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.
Depends on: 1453269
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.
Attachment #8966572 - Flags: review+ → review?(jib)
Comment on attachment 8966572 [details]
Bug 1450954 - Test that a screenshare track's getSettings return sane dimensions.

https://reviewboard.mozilla.org/r/235302/#review241350
Attachment #8966572 - Flags: review?(jib) → review+
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.
Flags: needinfo?(apehrson)
Keywords: leave-open
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d03ca6d705b
Add back code to undo screenshare constraints hack. r=jib
Attachment #8966573 - Flags: checkin+
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.
Flags: needinfo?(apehrson)
Attachment #8966573 - Flags: approval-mozilla-beta?
Still need a reminder for the test.
Flags: needinfo?(apehrson)
Comment on attachment 8966573 [details]
Bug 1450954 - Add back code to undo screenshare constraints hack.

webrtc regression fix, approved for 60.0b13
Attachment #8966573 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0cf37de4a14
Test that a screenshare track's getSettings return sane dimensions. r=jib
Flags: needinfo?(apehrson)
Attachment #8966572 - Flags: checkin+
The test is now ready for uplift here too. Since the fix has not been uplifted yet, they can go up together.
Keywords: leave-open
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
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.
Flags: needinfo?(apehrson)
Attachment #8966572 - Flags: checkin+
Depends on: 1454625
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db38d1dfdb81
Test that a screenshare track's getSettings return sane dimensions. r=jib
Attachment #8966572 - Flags: checkin+
Thanks for the quick uplift!
https://hg.mozilla.org/mozilla-central/rev/db38d1dfdb81
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: