+++ 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

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

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

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 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 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 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 on attachment 8966572 [details] Bug 1450954 - Test that a screenshare track's getSettings return sane dimensions. https://reviewboard.mozilla.org/r/235302/#review241350

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.

Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d03ca6d705b Add back code to undo screenshare constraints hack. r=jib

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.

Still need a reminder for the test.

Comment on attachment 8966573 [details] Bug 1450954 - Add back code to undo screenshare constraints hack. webrtc regression fix, approved for 60.0b13

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

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

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

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

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.

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

Thanks for the quick uplift!