1211656 - GUM Constraints for screen sharing don't affect stream resolution

Status: RESOLVED FIXED

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

Description

[steinbrecher.johann]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36

Steps to reproduce:

call navigator.mozGetUserMedia with the constraints object. For example
{
    video: {
        mediaSource: 'screen',
        require: [
           'width',
           'height',
           'frameRate'
        ],
        width: {
          min: '160',
          max: '400'
        },
        height: {
          min: '90',
          max: '400'
        },
        frameRate: {
          min: '5',
          max: '10'
        }
      }
}



Actual results:

The video stream starts capturing from the mediasource screen. The resolution is set to the screen resolution instead of the resolution passed in with the GUM constraints.


Expected results:

The video stream starts capturing from the mediasource screen. The resolution should be set according to the passed in GUM constraints.

Comment 1

[steinbrecher.johann]

Attachment: 0001-Fixes-1211656-GUM-Constraints-for-screen-sharing-don.patch

- set frame capture resolution based on constraints (capabilities)
- maintain aspect ratio of actual screen resolution

I should initialize the capabilities object with the actual screen resolution. In case no constraints get passed in, the full screen gets captured (same behavior as so far). Challenge:

the actual screen resolution is contained in the 'frameInfo' object. http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc#580.

The constraints are initialized way before in the flow in the GUM code:
http://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#402

From what I see, there is no way to know the actual screen resolution in the GUM code?

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

(In reply to steinbrecher.johann from comment #1)

> From what I see, there is no way to know the actual screen resolution in the
> GUM code?

You can grab a screencapture stream, and then assign it to a hidden <video> element and ask it the size.  That's about it (this is from the JS level).  There's no way to query the screensize at the c++ level without capturing at least one frame, I think.

Comment 3

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

Exactly where do the values in the frameInfo object come from? If it's the size of the desktop, then that seems knowable at some level (though maybe it's more complicated in multi-monitor setups etc.)

If we can't get this information out, then an alternative I think would be to bring the constraints down to this code (the flattened version should do) and apply the constraints once we know the native dimension.

Comment 4

[steinbrecher.johann]

The frameInfo's origin is here:
http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer_x11.cc#316

Comment 5

[steinbrecher.johann]

If we could mark the constraints object in the GUM code it would be easy to populate it in the webrtc.org section with the actual screen resolution.

Comment 6

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

That's taken from at frame it seems, which I guess is too late, so it looks like jesup is right in comment 2.

How about the alternative (pass in minWidth, maxWidth, minHeight, maxHeight)?

Comment 7

[steinbrecher.johann]

Attachment: 0001-Fixes-1211656-GUM-Constraints-for-screen-sharing-don.patch

To use the screen resolution as default capability, I detect if the capability got set to a default value (http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/video_engine/vie_capturer.cc#216). If so I set it to the actual screen size in DesktopCaptureImpl::OnCaptureCompleted.

Comment 8

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

Comment on attachment 8676587 [details] [diff] [review]
0001-Fixes-1211656-GUM-Constraints-for-screen-sharing-don.patch

Review of attachment 8676587 [details] [diff] [review]:
-----------------------------------------------------------------

I gather from your TODO in the patch that this is a WIP, but I decided to record some comments here anyway.

Unfortunately, I see a tension here between getting something working, and implementing the spec correctly, because getting the spec right might be a bit tricky.

For instance, we probably need to know the unconstrained value (or some good approximation of it) ahead of time to implement:

 1. The max possible value exposed to JS in (inputDeviceInfo || track).getCapabilities() (Bug 1179084).
 2. We're supposed to throw OverconstrainedError on mandatory constraints [1], like width: { min: 2881 }.

The latter is a problem for cameras too for frameRate and lighting conditions (see bug 1213524 comment 1).

If we pass constraints in and apply them as frames come in, as I suggest in comment 6, then it's too late to fail getUserMedia or applyConstraints.

If we can't know the exact values ahead of time, then perhaps we can get away with a hybrid where constraints are resolved higher up based on some approximation (so at least width: { min: 9999 } will fail), and still pass constraints in, to make sure we don't scale the output by mistake).

[1] https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getUserMedia#Parameters

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +648,5 @@
> +
> +
> +    if (_requestedCapability.height < frameInfo.height) {
> +      // aspect ratio is maintained
> +      const float frameAspectRatio =  ((const float) width) / ((const float) height);

I think we generally avoid casting expressions to const (seems redundant and might interfere with move semantics in general). const on the auto variable should suffice.

@@ +650,5 @@
> +    if (_requestedCapability.height < frameInfo.height) {
> +      // aspect ratio is maintained
> +      const float frameAspectRatio =  ((const float) width) / ((const float) height);
> +
> +      int dst_width = (const int32_t) (_requestedCapability.height * frameAspectRatio);

should probably use round

@@ +833,5 @@
> +    _requestedCapability.width = frameInfo.width;
> +  }
> +  if (_requestedCapability.height == kViECaptureDefaultHeight) {
> +    _requestedCapability.height = frameInfo.height;
> +  }

With this hack, it's not possible to constrain to 352x288.

Furthermore, the constraints algorithm must take the unconstrained value as input, or the algorithm will produce wrong results, and not be spec compliant.

Comment 9

[steinbrecher.johann]

(In reply to Randell Jesup [:jesup] from comment #2)
> (In reply to steinbrecher.johann from comment #1)
> 
> > From what I see, there is no way to know the actual screen resolution in the
> > GUM code?
> 
> You can grab a screencapture stream, and then assign it to a hidden <video>
> element and ask it the size.  That's about it (this is from the JS level). 
> There's no way to query the screensize at the c++ level without capturing at
> least one frame, I think.

We want to maintain the current behavior of defaulting to the actual screen resolution in case no constraints are passed in. The actual screen resolution is accessible in http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc#786. Thats in the webrtc.org capturer code. How can I detect if the requested constraints got passed in from the application or if its a default. Maybe we should populate the constraints with unrealistic values as suggested by jib in comment 8.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #8)
>  1. The max possible value exposed to JS in (inputDeviceInfo ||
> track).getCapabilities() (Bug 1179084).
>  2. We're supposed to throw OverconstrainedError on mandatory constraints
> [1], like width: { min: 2881 }.
> 
> The latter is a problem for cameras too for frameRate and lighting
> conditions (see bug 1213524 comment 1).
> 
> If we pass constraints in and apply them as frames come in, as I suggest in
> comment 6, then it's too late to fail getUserMedia or applyConstraints.
> 
> If we can't know the exact values ahead of time, then perhaps we can get
> away with a hybrid where constraints are resolved higher up based on some
> approximation (so at least width: { min: 9999 } will fail), and still pass
> constraints in, to make sure we don't scale the output by mistake).

Note: there is no way to know what size a capture is in the future.  You know can know what size is currently captured in a frame but not what the next frame will bring, since the underlying source can change (i.e. window resize, desktop resolution change, rotate a tablet/monitor/etc).  This is fundamentally kinda different than a camera, which generally are fixed return size once you open them and configure.

Comment 11

[steinbrecher.johann]

These appear to be the options here:

1) Roll with a default value. In the moment thats 352x288. This value gets correct by the actual screen aspect ratio before delivering the frame. If the application wants a different resolution, it has to pass it in through the constraints. The application can use the method from comment 2.

2) Find a criteria to detect if the constraints/requestedCapabilities are populated with default values and apply the dimensions of the actually captured frame on each capture.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

(In reply to steinbrecher.johann from comment #11)
> These appear to be the options here:
> 
> 1) Roll with a default value. In the moment thats 352x288. This value gets
> correct by the actual screen aspect ratio before delivering the frame. If
> the application wants a different resolution, it has to pass it in through
> the constraints. The application can use the method from comment 2.

This is not a viable option, in my opinion - it's not supposed to work like this when unconstrained, and no application should (or does) expect to have to do this.

> 2) Find a criteria to detect if the constraints/requestedCapabilities are
> populated with default values and apply the dimensions of the actually
> captured frame on each capture.

Can't we detect if constraints weren't passed, and if so set things to "accept anything unchanged"?

Comment 13

[steinbrecher.johann]

On option 2:

The actual screen resolution is contained in the 'frameInfo' object. http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc#580.

The constraints are initialized way before in the flow in the gUM code:
http://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#402

In between those two ends is IPC. The place setting default values is http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc#786.
We could:
2.1) instead of using 352x288 for the default use say -INF x -INF and correct this value and by applying the dimensions of the actually captured frame on each capture.
2.2) add a flag to the capabilities object indicating its no set by the application from the gUM code to the webrtc.org code.
2.3) ?

Comment 14

[steinbrecher.johann]

Attachment: 0001-Fixes-1211656-GUM-Constraints-for-screen-sharing-don.patch

Going with option 2 as in comment #11. Detect that no constraints got passed in and set capture resolution to the actual screen resolution on each capture.

Comment 15

[steinbrecher.johann]

Attachment: 0001-Fixes-1211656-GUM-Constraints-for-screen-sharing-don.patch

Comment 16

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

All the "detect if constraints are passed in" solutions are nonviable, because:

Users who pass in { min: 0, max: 99999 } should get the same as passing in nothing.
Users who pass in width: { min: 320, max: 1280 } for a 2880 wide desktop should get 1280, not 352 or 320.

As I said in comment 8, the algorithm takes the unconstrained input.

> 2.1) instead of using 352x288 for the default use say -INF x -INF and
> correct this value and by applying the dimensions of the actually captured
> frame on each capture.

As I said in comment 6, I think we need to pass in a rectangle: (-INF x -INF, +INF x +INF) which happens to be the default value of a FlattenConstraints structure. [1]

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaTrackConstraints.cpp?rev=0a8484884b6b&mark=35-35#33

Comment 17

[steinbrecher.johann]

Followed up with jib. The suggested solution is to pass down height/width min/max values to the capturer and scale accordingly.

Comment 18

[steinbrecher.johann]

Attachment: 0001-Fixes-1211656-GUM-Constraints-for-screen-sharing-don.patch

Comment 19

[steinbrecher.johann]

Attachment: 0001-Fixes-1211656-GUM-Constraints-for-screen-sharing-don.patch

Comment 20

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

Comment on attachment 8682339 [details] [diff] [review]
0001-Fixes-1211656-GUM-Constraints-for-screen-sharing-don.patch

Review of attachment 8682339 [details] [diff] [review]:
-----------------------------------------------------------------

Need to fix the width typo, and I also have concerns about the test.

::: dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html
@@ +54,5 @@
> +        playback.startMedia(false);
> +        return playback.verifyPlaying()
> +          .then(() => Promise.all([
> +            () => testVideo.srcObject.getVideoTracks()[0].applyConstraints(videoConstraints),
> +            () => listenUntil(testVideo, "resize", () => true)

Hmm, you're passing in the same videoConstraints as initially. Does "resize" really fire? It shouldn't I don't think, since we're not changing resolution from what existed when the stream was created by getUserMedia.

We should also do a test with ideal values. e.g. width: 200, height: 200.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp
@@ +410,5 @@
>      case dom::MediaSourceEnum::Window:
>      case dom::MediaSourceEnum::Application: {
>        FlattenedConstraints c(aConstraints);
> +      mCapability.width = ((c.mWidth.mIdeal.WasPassed() ? c.mWidth.mIdeal.Value() : 0) & 0xffff) << 16 | (c.mWidth.mMax & 0xffff);
> +      mCapability.height = ((c.mHeight.mIdeal.WasPassed() ? c.mHeight.mIdeal.Value() : 0) & 0xffff) << 16 | (c.mHeight.mMax & 0xffff);

Please wordwrap to 80ish characters, and add a comment about why we're doing this.

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +645,5 @@
>        return -1;
>      }
> +
> +    int32_t reqMaxHeight = _requestedCapability.height & 0xffff;
> +    int32_t reqMaxWidth = _requestedCapability.width & 0xffff;

Nit: existing code does width then height throughout, while this does height then width. We should be consistent (width then height being most common here).

@@ +650,5 @@
> +    int32_t reqIdealHeight = (_requestedCapability.height >> 16) & 0xffff;
> +    int32_t reqIdealWidth = (_requestedCapability.width >> 16) & 0xffff;
> +
> +    int32_t maxHeight = std::min(reqMaxHeight, frameInfo.height);
> +    int32_t maxWidth = std::min(reqMaxWidth, frameInfo.width);

Looking at the code directly above this whole patch, which optionally rotates the capture, I think we must use target_width and target_height here instead of frameInfo.width and frameInfo.height, from here on out.

@@ +670,5 @@
> +      dst_width = (int)(scale * dst_width);
> +      dst_height = (int)(scale * dst_height);
> +    }
> +
> +    if (width == frameInfo.width && dst_height == frameInfo.height) {

This should be dst_width, not width I think (width is already an alias for frameInfo.width)!

I think all the similarly named auto variables in this function just became a maintenance liability. Could we clean it up? I think we should either:

A) Remove the width and height autos that bit us here, or

B) stick with width and height, but overwrite them as we go along to keep up with the transformations.

@@ +678,5 @@
> +      I420VideoFrame scaledFrame;
> +      ret = scaledFrame.CreateEmptyFrame(dst_width,
> +                                               dst_height,
> +                                               stride_y,
> +                                               stride_uv, stride_uv);

Fix indentation.

Comment 21

[steinbrecher.johann]

Attachment: 0001-Fixes-1211656-GUM-Constraints-for-screen-sharing-don.patch

Comment 22

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

Comment on attachment 8683346 [details] [diff] [review]
0001-Fixes-1211656-GUM-Constraints-for-screen-sharing-don.patch

Review of attachment 8683346 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +686,5 @@
> +        return -1;
> +      }
> +
> +      webrtc::Scaler s;
> +      s.Set(width, height, dst_width, dst_height, kI420, kI420, kScaleBox);

First two args should be target_width and target_height, I think.

I'm for removing width and height.

Comment 23

[steinbrecher.johann]

Attachment: 0001-Fixes-1211656-GUM-Constraints-for-screen-sharing-don.patch

Comment 24

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

Comment on attachment 8683444 [details] [diff] [review]
0001-Fixes-1211656-GUM-Constraints-for-screen-sharing-don.patch

Review of attachment 8683444 [details] [diff] [review]:
-----------------------------------------------------------------

would still like to see const width and height go, but lgtm.

Comment 25

[Nils Ohlmeier [:drno]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95e3ae7b7592

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6828e82e1b7

Comment 27

[Carsten Book [:Tomcat]]

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