Closed Bug 1060796 Opened 6 years ago Closed 6 years ago

Limit screen capture FPS

Categories

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

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
firefox32 --- unaffected
firefox33 --- verified
firefox34 --- verified
firefox35 --- verified

People

(Reporter: jesup, Assigned: gcp)

References

()

Details

Attachments

(1 file, 2 obsolete files)

gcp identified that the capture code was i-looping and capturing as fast as it could while investigating bug 1053264.

We should limit the capture rate to max 30fps.  In the future, we may want to limit it or make it controllable either at getUserMedia time, or via RTPSender.
Gian-Carlo, do you have the patches you did while investigating the flickering?
Flags: needinfo?(gpascutto)
Attached patch Limit screen capture FPS (obsolete) — Splinter Review
Gian-Carlo, does this fit with what you've been working?
Attached patch Patch 1. limit-capture-rate (obsolete) — Splinter Review
Not really.
Flags: needinfo?(gpascutto)
Attachment #8482752 - Flags: review?(linuxwolf)
Attachment #8481811 - Attachment is obsolete: true
Attachment #8481836 - Attachment is obsolete: true
Comment on attachment 8482752 [details] [diff] [review]
Limit screen capture FPS

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

Matthew is apparently working on something else, shifting reviewer.
Attachment #8482752 - Flags: review?(linuxwolf) → review?(rjesup)
Comment on attachment 8482752 [details] [diff] [review]
Limit screen capture FPS

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

Note, this will likely have some level of merge conflict with jimm's patches for windows, which will likely need to use a different process()/eventloop

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +801,5 @@
>    desktop_capturer_cursor_composer_->Capture(DesktopRegion());
> +  const uint32_t processTime =
> +      (uint32_t)(TickTime::Now() - startProcessTime).Milliseconds();
> +  // Use at most 50% CPU or limit framerate, 60 fps = 16ms
> +  time_event_.Wait(std::max<uint32_t>(16, processTime));

We create the channel with a max FPS of 30, so any frames beyond that will be thrown away anyways.

replace 16 with 1000/_requestedCapability.maxFPS, and consider hoisting the 30 there into a #define (for now); we can look at making it configurable in some manner by the app in the future.

We will also want to teach the content analysis stuff that screen/window captures should not be downrezed unless the frame rate gets really low, but that's a separate bug.
Attachment #8482752 - Flags: review?(rjesup) → review+
Depends on: 1060738
Now with more qref (on the try patch, sigh):
https://tbpl.mozilla.org/?tree=Try&rev=1eb1e5637d32
https://hg.mozilla.org/mozilla-central/rev/64f8210023a5
Assignee: nobody → gpascutto
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
This landed but I forgot to qref the patch with the latest changes, as a result it doesn't address all review comments. I will fix this here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Nevermind, the right thing did land, yay.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Comment on attachment 8482752 [details] [diff] [review]
Limit screen capture FPS

Approval Request Comment
[Feature/regressing bug #]: Bug 983504 - Basic WebRTC screensharing support
[User impact if declined]: High CPU usage during screensharing
[Describe test coverage new/current, TBPL]: Landed on m-c a week ago
[Risks and why]: At worst it does funny things to the frames per second.
[String/UUID change made/needed]: N/A
Attachment #8482752 - Flags: approval-mozilla-beta?
Attachment #8482752 - Flags: approval-mozilla-aurora?
Comment on attachment 8482752 [details] [diff] [review]
Limit screen capture FPS

Beta+ and Aurora+
Attachment #8482752 - Flags: approval-mozilla-beta?
Attachment #8482752 - Flags: approval-mozilla-beta+
Attachment #8482752 - Flags: approval-mozilla-aurora?
Attachment #8482752 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
Using Nightly 34.0a1 2014-08-30 under Win 7 64-bit I had about 200Mb memory and under 15% CPU usage while performing screen sharing on the test page.
The CPU rate seems lower with Firefox 33 beta 3.

Is there a tool I could use to measure FPS rate in order to verify this bug is fixed? Or how else should I proceed? Thanks
Flags: needinfo?(gpascutto)
I don't think you can externally observe the capture rate, given that IIRC the encoder would encode at 30fps anyway.

CPU usage in Windows is for all processors, so for example if you have 4 CPU cores, then "100% of 1 cpu" would show as "25%" (100/4) in Windows. We limit to 50% of the CPU that is capturing, so your number sound about right for a quad-core system. (100/4/2 = 12.5%)
Flags: needinfo?(gpascutto)
Thanks gcp!

Tested and observed the CPU rates using Firefox 33 beta 4, latest Aurora 34.0a2 (20140915004005) and latest Nightly 35.0a1 (20140915030204) under Win 7 64-bit (4 CPU cores), Ubuntu 12.10 32-bit (3 CPU) and Mac OSX 10.8.5 (4 CPU). 

On those versions the CPU rate is significantly reduced as against Nightly builds before the fix:
Win 7: 9-11% vs 15%
Ubuntu: 33-39% vs 51-60%
Mac OSX: 25-28% vs 80-121%
Status: RESOLVED → VERIFIED
QA Contact: petruta.rasa
Whiteboard: [screensharing-uplift]
You need to log in before you can comment on or make changes to this bug.