1449832 - getUserMedia crops video track when requesting screen with single dimension constraint

Status: VERIFIED FIXED

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

Description

[Brian Peiris]

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

Comment 1

[Andreas Pehrson [:pehrsons]]

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)

Comment 2

[Brian Peiris]

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.

Comment 3

[Andreas Pehrson [:pehrsons]]

(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.

Comment 4

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1449832 - P1 - screen share scale when a single dimension constraint is supplied

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

Comment 5

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1449832 - P2 - screen share scale mochitest

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

Comment 6

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8965800 [details]
Bug 1449832 - P1 - screen share scale when a single dimension constraint is supplied

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

Comment 7

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8965801 [details]
Bug 1449832 - P2 - screen share scale mochitest

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

Comment 8

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8965801 [details]
Bug 1449832 - P2 - screen share scale mochitest

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

Comment 9

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8965801 [details]
Bug 1449832 - P2 - screen share scale mochitest

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

Comment 10

[Andreas Pehrson [:pehrsons]]

This looks like the regressing commit: https://hg.mozilla.org/mozilla-central/rev/213c2c200c08

Comment 11

[Andreas Pehrson [:pehrsons]]

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?

Comment 12

[Andreas Pehrson [:pehrsons]]

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

Comment 13

[Andreas Pehrson [:pehrsons]]

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.

Comment 14

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

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()

Comment 15

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

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

Comment 16

[Andreas Pehrson [:pehrsons]]

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 17

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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.

Comment 18

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8965800 [details]
Bug 1449832 - P1 - screen share scale when a single dimension constraint is supplied

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

Comment 19

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8965801 [details]
Bug 1449832 - P2 - screen share scale mochitest

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234640/diff/4-5/

Comment 20

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8965800 [details]
Bug 1449832 - P1 - screen share scale when a single dimension constraint is supplied

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

Comment 21

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8965801 [details]
Bug 1449832 - P2 - screen share scale mochitest

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234640/diff/5-6/

Comment 22

[Andreas Pehrson [:pehrsons]]

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

Comment 23

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

Comment 24

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8965800 [details]
Bug 1449832 - P1 - screen share scale when a single dimension constraint is supplied

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234638/diff/4-5/

Comment 25

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8965801 [details]
Bug 1449832 - P2 - screen share scale mochitest

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234640/diff/6-7/

Comment 26

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1449832 - restore screen share scaling code to prevent cropping

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

Comment 27

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f67256cb45cf742e66dc81e05a04374d51faf6ac

Comment 28

[Andreas Pehrson [:pehrsons]]

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

Comment 29

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8967589 [details]
Bug 1449832 - restore screen share scaling code to prevent cropping

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

Comment 30

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af50acbd6c28
restore screen share scaling code to prevent cropping r=pehrsons

Comment 32

[Natalia Csoregi [:nataliaCs]]

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

Comment 33

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

Comment 34

[Julien Cristau [:jcristau]]

Comment on attachment 8967589 [details]
Bug 1449832 - restore screen share scaling code to prevent cropping

fix webrtc screen share regression, approved for 60.0b13

Comment 35

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f1a77df4047

Comment 36

[Oana Botisan, Desktop QA]

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.