Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: jesup, Assigned: gcp)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox32 unaffected, firefox33 verified, firefox34 verified, firefox35 verified)

Details

()

Attachments

(1 attachment, 2 obsolete attachments)

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)
Posted patch Limit screen capture FPS (obsolete) — Splinter Review
Gian-Carlo, does this fit with what you've been working?
Assignee

Comment 3

5 years ago
Posted patch Patch 1. limit-capture-rate (obsolete) — Splinter Review
Not really.
Flags: needinfo?(gpascutto)
Assignee

Updated

5 years ago
Attachment #8482752 - Flags: review?(linuxwolf)
Assignee

Updated

5 years ago
Attachment #8481811 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8481836 - Attachment is obsolete: true
Assignee

Comment 5

5 years ago
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+

Updated

5 years ago
Depends on: 1060738
Assignee

Comment 8

5 years ago
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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Assignee

Comment 11

5 years ago
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 → ---
Assignee

Comment 12

5 years ago
Nevermind, the right thing did land, yay.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Assignee

Comment 13

5 years ago
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)
Assignee

Comment 17

5 years ago
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.