1376276 - firefox53 cannot limit framerate in getusermedia with screen

Status: RESOLVED FIXED

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

Description

[kongxuanace]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36

Steps to reproduce:

https://mozilla.github.io/webrtc-landing/gum_test.html

i want to limit the stream get form getusermedia in screen


Actual results:

when i set 10 fps,but in firefox53, the fps actually is 40-60


Expected results:

I think if i set 10fps, i shoud get the stream is limit in 10,not 40?

Comment 1

[Alex Chronopoulos [:achronop]]

In gum test page frame rate is provided as an ideal constraint. This is not a hard constraint. I copy from [0]:

```
An ideal value, when used, has gravity, which means that the browser will try to find the setting (and camera, if you have more than one), with the smallest fitness distance from the ideal values given.
```

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

Please let me know if that answers your query and I can close the bug.

Comment 2

[kongxuanace]

(In reply to Alex Chronopoulos [:achronop] from comment #1)
> In gum test page frame rate is provided as an ideal constraint. This is not
> a hard constraint. I copy from [0]:
> 
> ```
> An ideal value, when used, has gravity, which means that the browser will
> try to find the setting (and camera, if you have more than one), with the
> smallest fitness distance from the ideal values given.
> ```
> 
> [0]
> https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getUserMedia 
> 
> Please let me know if that answers your query and I can close the bug.




in firefox52 it is useful and i set mediaConstraints.video.frameRate={max:"10"},in firefox53 is also not limited?

Comment 3

[Alex Chronopoulos [:achronop]]

Did you try it on Nightly?

Comment 4

[kongxuanace]

(In reply to Alex Chronopoulos [:achronop] from comment #3)
> Did you try it on Nightly?

yes ,i try it on Nightly55 ,set frameRate={max:"10"} also is not useful.
start from 53,set max is not useful?

Comment 5

[Alex Chronopoulos [:achronop]]

:jib, can you tell if this is a regression or something changed intentionally?

Comment 6

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

Yes this is likely a regression. We don't offer downscaled resolution or decimated frame rates for camera (bug 1286945) but we do for screen sharing, to make it useful, due to the high resolutions and high frame rates.

If someone could use http://mozilla.github.io/mozregression/ to find a window that'd be great!

This also reminds me, I found a similar bug during the all hands that I forgot to file, where trying to resize screen with constraints gives me garbage, e.g. with https://jsfiddle.net/jib1/3xytpne5/ - It may have the same regression range.

Comment 7

[Alex Chronopoulos [:achronop]]

Can you please try to find a regression window? I try it here but my device uses only 30 fps, no more or less. If you install the tool you can execute: mozregression --good 52 --bad 53

Comment 8

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

Unless I misunderstand comment 0, this issue is about screensharing not cameras.

Comment 9

[Alex Chronopoulos [:achronop]]

Oops, my bad. I missed that it was just for the screen. I will try again.

Comment 10

[mmzhw51]

I also found the same problem and is this a feature?

why do this? the fps is too high, why we can not limit fps in newly version

Comment 11

[xpeng]

we use the max-fr to control the framerate before. but we find that the max-fr is also disabled from firefox 53
https://bugzilla.mozilla.org/show_bug.cgi?id=1393687

Comment 12

[Maire Reavy [:mreavy]]

jib - is this a dup?

Comment 13

[Bulk Bug Changes for mreavy's org]

Mass change P1->P2 to align with new Mozilla triage process

Comment 14

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

[Tracking Requested - why for this release]: Windows feature regression, albeit old one.

STR: https://jsfiddle.net/jib1/rnbcaao5

Works fine on OSX. Repros on Win10. frameRate constraint is ignored.

Regression window:

2017-10-25T16:25:47: INFO : Narrowed inbound regression window from [935e36fd, f647e2ea] (3 revisions) to [935e36fd, 126348e7] (2 revisions) (~1 steps left)
2017-10-25T16:25:47: DEBUG : Starting merge handling...
2017-10-25T16:25:47: DEBUG : Using url: https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=126348e718d03dec640b30b5def70fce8aa71527&full=1
2017-10-25T16:25:48: DEBUG : Found commit message:
Bug 1250356: rollup of changes for webrtc after applying webrtc.org v49 update r=pkerr,ng,pehrsons,etc
See ssh://hg.mozilla.org/users/paulrkerr_gmail.com/webrtc49_merge/ for the
patch development history.

Looks like another upstream break. Dan, could you take a look? Feel free to bring in others.

Comment 15

[Dan Minor [:dminor]]

I'll have a look.

Comment 16

[Dan Minor [:dminor]]

It's not surprising that this only affects Windows as the Windows code relies upon a timer to fire at the requested rate [1] and the other platforms wait on an event [2].

The desktop capture code itself doesn't seem to have changed since 52, but the PlatformUIThread code has. It looks like the timer code on Windows is firing too often, but I haven't been able to determine why yet. Removing the ifdefs so that Windows waits on the event like the other platforms seems to fix things, but it's concerning that the timer is not working as expected.

[1] http://searchfox.org/mozilla-central/rev/40e8eb46609dcb8780764774ec550afff1eed3a5/media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc#689

[2] http://searchfox.org/mozilla-central/rev/40e8eb46609dcb8780764774ec550afff1eed3a5/media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc#743

Comment 17

[Dan Minor [:dminor]]

For what it's worth, I just tested 52 and it did not work all that well then either. I seem to get around twice the requested framerate, at least for values between 1 and 10. (I'm testing small values because my Windows VM only captures at 10fps running without constraints, but I double checked the numbers on a Windows 10 laptop which runs at 30fps without constraints as well.)

Comment 18

[Dan Minor [:dminor]]

Attachment: bug-1376276-fix.patch

This fix reverts the changes made during the branch 49 update to the UIThread Run method. It looks like we restored video_engine/desktop_capture_impl.cc after it was removed upstream which is likely why it stopped working with the update to 49.

The other alternative is to remove the special case handling for Windows and wait on the event like we do for other platforms. This also seems to work, but I have not tested it extensively.

Comment 19

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

(In reply to Dan Minor [:dminor] from comment #17)
> I seem to get around twice the requested framerate,

Yes that matches my experience before the regression - e.g. {frameRate: 1} produced ~2 - at least it did something. :)

Comment 20

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

Comment on attachment 8922887 [details] [diff] [review]
bug-1376276-fix.patch

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

::: media/webrtc/trunk/webrtc/base/platform_thread.cc
@@ +310,5 @@
> +
> +    // Alertable sleep to permit RaiseFlag to run and update |stop_|.
> +    if (MsgWaitForMultipleObjectsEx(0, nullptr, INFINITE, QS_ALLINPUT,
> +                                    MWMO_ALERTABLE | MWMO_INPUTAVAILABLE) ==
> +        WAIT_OBJECT_0) {

so this does restore the pre-49 copy of that code.  The question is "is this code correct"?  it pre-dates 43, though the specific code 8might* have been redone to match ThreadWindows::Run() in 43.  Likely the UI variant was jimm's work to fix a bunch of evil sec issues with screen sharing because the thread wasn't a UI thread.

Of course, if it works, we're probably ok for now.

Comment 21

[Dan Minor [:dminor]]

:jib, if it is ok with you, I'd like to wait until 59 to land this change. Although I'm just restoring old code, it's not at all obvious to me why this works and the current code does not. Like Randell says, the UI thread code is only used on Windows for screen capture so the impact of this would be limited, but I'd still prefer to play it safe, given that this is an old regression anyway.

Comment 22

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

Makes sense. Let's get it in early in 59.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/009adb3d4e24
Fix limiting framerate in getusermedia with screen on Windows; r=jesup

Comment 24

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/009adb3d4e24

Comment 25

[Sylvestre Ledru [:Sylvestre]]

https://hg.mozilla.org/projects/oak/rev/009adb3d4e24b51e34ef8cb2f19444864bd7d14f
Bug 1376276 - Fix limiting framerate in getusermedia with screen on Windows; r=jesup

Comment 26

[Pradeep Kumar]

I'm still facing this issue on windows system that cannot limit the fps for screen sharing.
Browser : Firefox Quantum 63.
OS      : Windows 10
Processer : Intel I5 @ 2.60HGz
System Type : 64 bit operating system, x64-based processor.

Comment 27

[marcosdg3]

I'm seeing this issue as well on a macbook. I can reproduce the issue of the framerate being ignored by using this helpful snippet at the bottom of this page: https://developer.mozilla.org/en-US/docs/Web/API/Media_Streams_API/Constraints

Please let me know if you need more info or if I could contribute in anyway. 


MacBook Pro
  Model Identifier:	MacBookPro11,3
  Processor Name:	Intel Core i7
  Processor Speed:	2.6 GHz
  Number of Processors:	1
  Total Number of Cores:	4
  L2 Cache (per Core):	256 KB
  L3 Cache:	6 MB
  Memory:	16 GB
  Camera: Apple Camera VendorID_0x106B ProductID_0x1570

Best,
Marcos

Comment 28

[Pradeep Kumar]

Hi Marcos,

For screen sharing we need to include mediaSource as screen in the constraint as below. 
{
  "mediaSource" : "screen",
  "width": 320,
  "height": 240,
  "frameRate": 5
}

And for video, I observed that frameRate cannot be set anything below 30 in FF.

For my problem, setting value of frameRate for screen sharing to anything <= 10 is not the problem. Real problem is that FF does not adhere to that value. If I set value 5, I'm seeing frameRate to be ~10. That's the problem. If we set to 5, FF should emit 5 frames in a second.

Regarding video, and not able to even set the frameRate anything < 30 is irrespective of the platform (OS). That might be different issue and need to create another bug for it.

However, in this but we are targeting screen sharing issue only.