Closed Bug 1449832 Opened 6 years ago Closed 6 years ago

getUserMedia crops video track when requesting screen with single dimension constraint

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 + verified
firefox61 + verified

People

(Reporter: bpeiris, Assigned: ng)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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}

and the video is cropped, e.g. 1920x720 on a 1920x1080 screen.

I would expect it to scale the other dimension down and return a 1280x720 track. 

This is especially problematic since, AFAIK, there is no way to determine the aspect ratio of the provided screen before you prompt the user.

Demo: https://codepen.io/brianpeiris/pen/NYydvb?editors=0010
Thanks for the report, excellent codepen for repro!

Luckily we haven't shipped this yet, it's a 60 regression.

I'll try to narrow the regression range further.


tracking for: screensharing is more or less useless with this bug (height is cropped)
Assignee: nobody → apehrson
Rank: 9
Keywords: regression
Priority: -- → P1
Thanks for the quick response!

I think you're mistaken about this being a 60 regression. It looks like 59 reports getSettings correctly, but I _can_ reproduce the cropping issue in Firefox 59, on the stable channel, on Windows.
(In reply to Brian Peiris from comment #2)
> Thanks for the quick response!
> 
> I think you're mistaken about this being a 60 regression. It looks like 59
> reports getSettings correctly, but I _can_ reproduce the cropping issue in
> Firefox 59, on the stable channel, on Windows.

Sorry, you're right. I was looking at getSettings when making that assessment. Then let's split this out to two distinct bugs. I'm keeping this one for the scaling issue and moving out getSettings.

58 does have more sensible behavior on scaling.
Summary: getUserMedia reports incorrect track settings and crops video track when requesting screen with single dimension constraint → getUserMedia crops video track when requesting screen with single dimension constraint
See Also: → 1450954
Attachment #8965800 - Flags: review?(jib)
Attachment #8965801 - Flags: review?(jib)
Assignee: apehrson → na-g
Status: NEW → ASSIGNED
Comment on attachment 8965800 [details]
Bug 1449832 - P1 - screen share scale when a single dimension constraint is supplied

https://reviewboard.mozilla.org/r/234638/#review240564

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:511
(Diff revision 2)
>      req_max_width = mCapability.width & 0xffff;
>      req_max_height = mCapability.height & 0xffff;
>      req_ideal_width = (mCapability.width >> 16) & 0xffff;
>      req_ideal_height = (mCapability.height >> 16) & 0xffff;

Relying on the constraint to be reflected in mCapability is no longer needed as we now handle this here instead of in desktop_capture_impl.cc.

We should reflect the constraint in an mConstraint member instead. One that can properly store the intended constraint. Then when mCapability says 0x0 we can rely on mConstraint for scaling without losing aspect ratio.

For upliftability I'm OK with you filing a followup instead of fixing this now, but OTOH the resulting code will be much clearer with it fixed.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:516
(Diff revision 2)
> -
> +  // Preserve aspect ratio if only a single dimension is supplied
> +  preserveFrameAspectRatio(req_max_width, req_max_height, aProps);
> +  preserveFrameAspectRatio(req_ideal_width, req_ideal_height, aProps);

Please only do this when mMediaSource is screen, window or application, or use asserts.

If a camera doesn't have capabilities we use hardcoded ones, so the `req_max_` vars should never be 0 for cameras.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:534
(Diff revision 2)
> +  LOGFRAME(("%s req_max_width=%li, req_max_height=%li, " \
> +            "req_ideal_width=%li, req_ideal_height=%li" \
> +            "dst_width=%li, dst_height=%li",
> +            __PRETTY_FUNCTION__,
> +            static_cast<long>(req_max_width), static_cast<long>(req_max_height),
> +            static_cast<long>(req_ideal_width), static_cast<long>(req_ideal_height),
> +            static_cast<long>(dst_width), static_cast<long>(dst_height)
> +  ));

Did you mean to include this?

If so, please put them in the other LOGFRAME at the end of the method.

And you should be able to log these values with `%d` as they are, no?
Attachment #8965800 - Flags: review+
Comment on attachment 8965800 [details]
Bug 1449832 - P1 - screen share scale when a single dimension constraint is supplied

https://reviewboard.mozilla.org/r/234638/#review240584

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:490
(Diff revision 2)
> +  if (!aConstraintWidth && aConstraintHeight && aProps.height()) {
> +    aConstraintWidth = aProps.width() / (aProps.height() / aConstraintHeight);
> +  }
> +  if (!aConstraintHeight && aConstraintWidth && aProps.width()) {
> +    aConstraintHeight = aProps.height() / (aProps.width() / aConstraintWidth);
> +  }

This does integer division no?

E.g., with constraints height:720 and resolution 1920x1080 you end up with 1920/(1080/720)=1920/1=1920
Attachment #8965800 - Flags: review+ → review-
Comment on attachment 8965801 [details]
Bug 1449832 - P2 - screen share scale mochitest

https://reviewboard.mozilla.org/r/234640/#review240578

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:8
(Diff revision 4)
> +<head>
> +  <script src="mediaStreamPlayback.js"></script>
> +  <script src="constraints.js"></script>
> +</head>
> +<body>
> +<video id="view" autoplay=""></video>

If you use `createMediaElement` [1] we'll get a media element in the "content" div that's only visible when `visible: true` is passed to `createHTML`.

This is minor but how most of our tests control visibility.

[1] https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/dom/media/tests/mochitest/head.js#312

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:11
(Diff revision 4)
> +</head>
> +<body>
> +<video id="view" autoplay=""></video>
> +<pre id="test">
> +<script type="application/javascript">
> +createHTML({ title: "Test getUserMedia scales not crops when given a single dimension constraint", bug: "1449832" });

Shoud mention screen/window/application capture explicitly.

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:13
(Diff revision 4)
> +let eventToPromise = (target, event, predicate = () => true) => new Promise((resolve) =>
> +    target.addEventListener(event, () => {
> +      if(predicate()) {
> +        resolve();
> +      }
> +    }));

If something as generic as this is truly needed it should be added to head.js. Chances are head.js already has what you need.

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:26
(Diff revision 4)
> +  await pushPrefs(
> +    ["full-screen-api.enabled", true],
> +    ["full-screen-api.unprefix.enabled", true],
> +    ["full-screen-api.allow-trusted-requests-only", false],
> +    ["full-screen-api.transition-duration.enter", "0 0"],
> +    ["full-screen-api.transition-duration.leave", "0 0"],
> +  );

We only need these if you're gonna go fullscreen and (i.e., analyse pixels in the capture), and I don't see the test doing that.

For now I don't think we need to, though corruption during scaling is certainly a risk.

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:40
(Diff revision 4)
> +    let vidReady = eventToPromise(view, 'loadedmetadata');
> +    view.srcObject = stream;
> +    await vidReady.then();
> +    let [width, height] = [view.videoWidth, view.videoHeight];
> +    if (width == 0 || height == 0) {
> +      ok(false,
> +         `Screen share dimensions are not 0. ${width}x${height}`);
> +      return;
> +    }

We should be able to wait for the "resize" event on the media element instead of all this.

It probably also makes sense for the test logic to handle the starting resolution so you don't end up calling applyConstraints with the same resolution your screen already has.

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:56
(Diff revision 4)
> +  let testHeightScaling = async (width) => {
> +    await wrapTest({mediaSource: "screen", width: width}, async (stream) => {

I think I'd rather see `wrapTest` merged into `testHeightScaling` and `testWidthScaling`. It's simple enough, and it gets rid of a layer of indirection.

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:60
(Diff revision 4)
> +      resChanged = eventToPromise(view, 'timeupdate',
> +        () => (view.videoWidth != width && view.videoWidth));

Please use `haveEvent` from head.js, and the "resize" event on HTMLMediaElement.

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:62
(Diff revision 4)
> +    await wrapTest({mediaSource: "screen", width: width}, async (stream) => {
> +      let height = view.videoHeight;
> +      is(view.videoWidth, width, "Starting video width is correct.");
> +      resChanged = eventToPromise(view, 'timeupdate',
> +        () => (view.videoWidth != width && view.videoWidth));
> +      await stream.getVideoTracks()[0].applyConstraints({width: width / 2});

Please log the constraints that get applied with `info`. The initial dimensions would also be good to know when reading the logs.

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:63
(Diff revision 4)
> +      let height = view.videoHeight;
> +      is(view.videoWidth, width, "Starting video width is correct.");
> +      resChanged = eventToPromise(view, 'timeupdate',
> +        () => (view.videoWidth != width && view.videoWidth));
> +      await stream.getVideoTracks()[0].applyConstraints({width: width / 2});
> +      await resChanged.then();

Hmm, what's the `.then()` for?

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:64
(Diff revision 4)
> +      is(view.videoWidth, width / 2,
> +         `Width scaled down to ${width / 2}`);
> +      is(view.videoHeight, height / 2,
> +         `Height scaled down to ${height / 2}`);

How do you know the initial width/height was scaled correctly when it was passed in with a constraint?

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:64
(Diff revision 4)
> +      is(view.videoWidth, width / 2,
> +         `Width scaled down to ${width / 2}`);
> +      is(view.videoHeight, height / 2,
> +         `Height scaled down to ${height / 2}`);

Don't you risk test failures here due to an odd `width` or `height`?

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:85
(Diff revision 4)
> +      is(view.videoHeight, height / 2,
> +         `Height scaled down to ${height / 2}`);
> +    });
> +  };
> +
> +  for (dimension of [320, 480, 640, 720]) {

Could you do a single unconstrained gUM first, just to figure out the real screen resolutions?

Use that to calculate expected scaling values and explicitly pass the expectations in to the test case.

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:85
(Diff revision 4)
> +      is(view.videoHeight, height / 2,
> +         `Height scaled down to ${height / 2}`);
> +    });
> +  };
> +
> +  for (dimension of [320, 480, 640, 720]) {

`for (let dimension of`
      ^^^^ ?

I guess this is why the test logs show only one iteration of this loop?

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:85
(Diff revision 4)
> +      is(view.videoHeight, height / 2,
> +         `Height scaled down to ${height / 2}`);
> +    });
> +  };
> +
> +  for (dimension of [320, 480, 640, 720]) {

I'd like to see some tests of extremes as well.

For instance that a larger ideal constraint than the source resolution doesn't create pixels.

And that odd dimensions end up choosing the closest resolution with exactly the same aspect ratio as the source's, or rounds the right way (so we end up with a slight crop, again not creating pixels).
We should probably use the same behavior we had before this regressed, or the specced one if it is in the spec. I doubt that however.
Attachment #8965801 - Flags: review-
Comment on attachment 8965801 [details]
Bug 1449832 - P2 - screen share scale mochitest

https://reviewboard.mozilla.org/r/234640/#review241220

Andreas' review is good. Just some drive-by nits.

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:13
(Diff revision 4)
> +let eventToPromise = (target, event, predicate = () => true) => new Promise((resolve) =>
> +    target.addEventListener(event, () => {
> +      if(predicate()) {
> +        resolve();
> +      }
> +    }));

We have this already:

listenUntil https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/dom/media/tests/mochitest/head.js#616

and

haveEvent https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/dom/media/tests/mochitest/head.js#781

A benefit is they use removeListener() to clean up. Please reuse.

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:15
(Diff revision 4)
> +<script type="application/javascript">
> +createHTML({ title: "Test getUserMedia scales not crops when given a single dimension constraint", bug: "1449832" });
> +
> +let eventToPromise = (target, event, predicate = () => true) => new Promise((resolve) =>
> +    target.addEventListener(event, () => {
> +      if(predicate()) {

if (

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:38
(Diff revision 4)
> +
> +  // Setup and tear down the stream for each test
> +  let wrapTest = async (constraints, test) => {
> +    // Setup the stream
> +    let stream = await getUserMedia({
> +      video: constraints, fake: false

Nit:

    fake: false

should be redundant now for screensharing.

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:40
(Diff revision 4)
> +    let vidReady = eventToPromise(view, 'loadedmetadata');
> +    view.srcObject = stream;
> +    await vidReady.then();

Unnecessary construct. It's fine to register a listener on the following line. E.g.:

    view.srcObject = await getUserMedia({video: constraints});
    await haveEvent(view, 'loadedmetadata');

Specs and the JS event loop guarantee there's no race here.

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:56
(Diff revision 4)
> +    // Tear down the stream
> +    stream.getTracks().forEach(track => track.stop());
> +    view.srcObject = null;
> +  }
> +
> +  let testHeightScaling = async (width) => {

Nit: () are redundant

There's also the old fashioned

    async function testHeightScaling(width) {

::: dom/media/tests/mochitest/test_getUserMedia_bug1449832.html:77
(Diff revision 4)
> +      let width = view.videoWidth;
> +      is(view.videoHeight, height, "Starting video height is correct.");
> +      resChanged = eventToPromise(view, 'timeupdate',
> +        () => (view.videoHeight != height && view.videoHeight));
> +      await stream.getVideoTracks()[0].applyConstraints({height: height / 2});
> +      await resChanged.then();

Remove

    .then()
Attachment #8965801 - Flags: review?(jib)
Comment on attachment 8965800 [details]
Bug 1449832 - P1 - screen share scale when a single dimension constraint is supplied

https://reviewboard.mozilla.org/r/234638/#review241226
Attachment #8965800 - Flags: review?(jib)
Blocks: 1450954
See Also: 1450954
See Also: → 1453269
Comment on attachment 8965800 [details]
Bug 1449832 - P1 - screen share scale when a single dimension constraint is supplied

https://reviewboard.mozilla.org/r/234638/#review240564

> Relying on the constraint to be reflected in mCapability is no longer needed as we now handle this here instead of in desktop_capture_impl.cc.
> 
> We should reflect the constraint in an mConstraint member instead. One that can properly store the intended constraint. Then when mCapability says 0x0 we can rely on mConstraint for scaling without losing aspect ratio.
> 
> For upliftability I'm OK with you filing a followup instead of fixing this now, but OTOH the resulting code will be much clearer with it fixed.

I've filed bug 1453269 as a followup to this.
Comment on attachment 8965801 [details]
Bug 1449832 - P2 - screen share scale mochitest

https://reviewboard.mozilla.org/r/234640/#review240578

> I'd like to see some tests of extremes as well.
> 
> For instance that a larger ideal constraint than the source resolution doesn't create pixels.
> 
> And that odd dimensions end up choosing the closest resolution with exactly the same aspect ratio as the source's, or rounds the right way (so we end up with a slight crop, again not creating pixels).
> We should probably use the same behavior we had before this regressed, or the specced one if it is in the spec. I doubt that however.

I am not sure that I have fully quantified the behavior in 58. My native resolution is 3840 × 2160. When I request a width of 2560 x 720, I get twice that. When I request a width of 719 I get 2559 x 1439. My tactic is to round down to the nearest perfect resoltion in the unit dimenisions.
Attachment #8965800 - Flags: review?(jib)
Attachment #8965801 - Flags: review?(jib)
Since this is for uplift, would it be easier to copy the 58 code [1]? It should be a pretty smooth fit in our MediaEngineRemoteVideoSource::DeliverFrame scaling code.

The essential parts look like this:
>     // scale to average of portrait and landscape
>     float scale_width = (float)dst_width / (float)target_width;
>     float scale_height = (float)dst_height / (float)target_height;
>     float scale = (scale_width + scale_height) / 2;
>     dst_width = (int)(scale * target_width);
>     dst_height = (int)(scale * target_height);
> 
>     // if scaled rectangle exceeds max rectangle, scale to minimum of portrait and landscape
>     if (dst_width > dest_max_width || dst_height > dest_max_height) {
>       scale_width = (float)dest_max_width / (float)dst_width;
>       scale_height = (float)dest_max_height / (float)dst_height;
>       scale = std::min(scale_width, scale_height);
>       dst_width = (int)(scale * dst_width);
>       dst_height = (int)(scale * dst_height);
>     }

[1] https://hg.mozilla.org/mozilla-central/file/1f91961bb79a/media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc#l585
For uplift, I am fine with transplanting the pre-59 code and opening another bug to address any of the issues 58 may have with imperfect aspect ratio, and doubling.

(In reply to Andreas Pehrson [:pehrsons] from comment #22)
> Since this is for uplift, would it be easier to copy the 58 code [1]? It
> should be a pretty smooth fit in our
> MediaEngineRemoteVideoSource::DeliverFrame scaling code.
...
> [1]
> https://hg.mozilla.org/mozilla-central/file/1f91961bb79a/media/webrtc/trunk/
> webrtc/video_engine/desktop_capture_impl.cc#l585
Attachment #8965800 - Flags: review?(apehrson)
Attachment #8965801 - Flags: review?(apehrson)
Attachment #8965800 - Attachment is obsolete: true
Attachment #8965801 - Attachment is obsolete: true
Attachment #8967589 - Flags: review?(apehrson)
Comment on attachment 8967589 [details]
Bug 1449832 - restore screen share scaling code to prevent cropping

https://reviewboard.mozilla.org/r/236282/#review242188

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:505
(Diff revision 1)
>      req_ideal_height = (mCapability.height >> 16) & 0xffff;
>    }
>  
> +  // This is only used in the case of screen sharing, see bug 1453269.
> +  int32_t target_width = aProps.width();
> +  int32_t target_height = abs(aProps.height());

Can `aProps.height()` really be negative?
If not we don't need these two.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:512
(Diff revision 1)
>    if (aProps.rotation() == 90 || aProps.rotation() == 270) {
>      // This frame is rotated, so what was negotiated as width is now height,
>      // and vice versa.
>      std::swap(req_max_width, req_max_height);
>      std::swap(req_ideal_width, req_ideal_height);
> +    std::swap(target_width, target_height);

If the frame is rotated, `aProps.width()/height()` is already correctly set, and opposite dimensions of the `req_` ones.

Should be able to just skip this.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:533
(Diff revision 1)
> +      // scale to average of portrait and landscape
> +      float scale_width = (float)dst_width / (float)aProps.width();
> +      float scale_height = (float)dst_height / (float)aProps.height();
> +      float scale = (scale_width + scale_height) / 2;
> +      dst_width = (int)(scale * target_width);
> +      dst_height = (int)(scale * target_height);

Cast to int32_t

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:541
(Diff revision 1)
> +      if (dst_width > dst_max_width || dst_height > dst_max_height) {
> +        scale_width = (float)dst_max_width / (float)dst_width;
> +        scale_height = (float)dst_max_height / (float)dst_height;
> +        scale = std::min(scale_width, scale_height);
> +        dst_width = (int)(scale * dst_width);
> +        dst_height = (int)(scale * dst_height);

Cast to int32_t
Attachment #8967589 - Flags: review?(apehrson) → review+
See Also: → 1211656
Comment on attachment 8967589 [details]
Bug 1449832 - restore screen share scaling code to prevent cropping

https://reviewboard.mozilla.org/r/236282/#review242188

> Can `aProps.height()` really be negative?
> If not we don't need these two.

I can't find evidence that it can. If it could be negative, surely it would already be a problem elsewehre in DeliverFrame.
Pushed by na-g@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/af50acbd6c28
restore screen share scaling code to prevent cropping r=pehrsons
https://hg.mozilla.org/mozilla-central/rev/af50acbd6c28
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8967589 [details]
Bug 1449832 - restore screen share scaling code to prevent cropping

Approval Request Comment
[Feature/Bug causing the regression]: 1388219
[User impact if declined]: Scaling of screen sharing when a single dimension constraint is supplied will not work at all as expected.
[Is this code covered by automated tests?]: Screen sharing does have mochitests, though the use case in this bug is not covered. I manually tested this patch with a number of single dimension constraints against the last revision that originally contained the code (58).
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: The filer provided a codepen.io, that can be used to verify.
1) Load the codepen.io: https://codepen.io/brianpeiris/pen/NYydvb?editors=0010
2) click the share screen button
3) select your screen from the prompt that appears
3) verify that a scaled down version of your screen appears below the "share screen" button. Without this fix only a cropped version of your screen appears.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: It is low risk
[Why is the change risky/not risky?]: This is code that was in the tree for several releases before being accidentally removed in 59, though it was in a different file at the time.
[String changes made/needed]: None
Attachment #8967589 - Flags: approval-mozilla-beta?
Comment on attachment 8967589 [details]
Bug 1449832 - restore screen share scaling code to prevent cropping

fix webrtc screen share regression, approved for 60.0b13
Attachment #8967589 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I managed to reproduce the issue using an older version of Nightly (2018-03-28) on Windows 10 x64.

I retested everything using latest Nightly 61.0a1 and beta 60.0b13 on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13 and the bug is not reproducing anymore. The video of the screen scaled in order to fit entirely.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.