1211658 - GUM constraints for screen sharing don't affect framerate

Status: RESOLVED FIXED

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 framerate is set 3fps instead of the framerate passed in with the GUM constraints.


Expected results:

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

Comment 1

[steinbrecher.johann]

Attachment: Proposed fix.

Comment 2

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

Comment on attachment 8669973 [details] [diff] [review]
Proposed fix.

We should probably go through the motions of review here even though I've looked at previous versions of this patch.

Comment 3

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

Comment on attachment 8669973 [details] [diff] [review]
Proposed fix.

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

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp
@@ +391,5 @@
> +  /**
> +   * TASKS todo:
> +   * - check the mediasource screen|application|tab|window.
> +   *   i.e. MediaEngineRemoteVideoSource->MediaSource and MediaEngineRemoteVideoSource->CaptureEngine
> +   * - parse contraints and set capabilities

This comment can be removed now I think.

@@ +407,5 @@
> +    case dom::MediaSourceEnum::Application: {
> +      FlattenedConstraints c(aConstraints);
> +      mCapability.width = c.mWidth.Clamp(c.mWidth.mIdeal.WasPassed() ? c.mWidth.mIdeal.Value() : aPrefs.mWidth);
> +      mCapability.height = c.mHeight.Clamp(c.mHeight.mIdeal.WasPassed() ? c.mHeight.mIdeal.Value() : aPrefs.mHeight);
> +      mCapability.maxFPS = c.mFrameRate.Clamp(c.mFrameRate.mIdeal.WasPassed() ? c.mFrameRate.mIdeal.Value() : aPrefs.mFPS);

Long lines should be word-wrapped to 80 characters (not a hard rule, but these are too long for sure).

Comment 4

[steinbrecher.johann]

Attachment: Bug fix

Incorporated comments from jib's review.

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8670040 [details] [diff] [review]
Bug fix

Carrying forward r+=jib

Comment 6

[Nils Ohlmeier [:drno]]

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

Hans you should check the results above some time tomorrow and see if you find any problems related to your change.

Comment 7

[steinbrecher.johann]

dry run results look good.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a915b7fdf7ac

Comment 9

[Wes Kocher (:KWierso)]

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