WebRTC maximum frame rate goes up to 20FPS only on v2.1

RESOLVED FIXED in Firefox 34

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaywang, Assigned: jesup)

Tracking

unspecified
2.1 S7 (24Oct)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.1+, firefox33 unaffected, firefox34 fixed, firefox35 fixed, firefox36 fixed, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [caf priority: p2][CR 737718])

Attachments

(5 attachments, 3 obsolete attachments)

Reporter

Description

5 years ago
Posted file fps_l1.txt
On v2.1, for some reason, the maximum encoder frame rate goes up to 20FPS only. v2.0 can have 30FPS frame rate.
From the log, it looks to me that the raw frame rate is already low. However, the frame rate from camera sensor goes up to 30FPS. So, something in the path that delivers the raw frame to H.264 encoder causes the lower frame rate.
I checked the camecorder app and it can encode the data @ 30FPS so the problem is specific to WebRTC use-case. 

Here is the test-case that I tried:

- Load the v2.1 build to 8x26 device
- Disable the loadmanager by adding "pref("media.navigator.load_adapt", false);" to  /system/b2g/defaults/pref/user.js on the target device and re-boot the device
- Make the H.264 WebRTC call (The same problem is there for VP8)
- Check the encoder statistics

Can you help to check the log and see if there is any clue on what the problem can be ?

I tried to enable these logs:
NSPR_LOG_MODULES=mediamanager:6,mediapipeline:5,signaling:6,WebrtcOMXH264VideoCodec:6,timestamp
Reporter

Updated

5 years ago
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(rjesup)
Flags: needinfo?(mvikram)
Flags: needinfo?(jolin)
Flags: needinfo?(dwilson)
Reporter

Comment 1

5 years ago
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.1?
Reporter

Updated

5 years ago
Flags: needinfo?(mvikram)
Flags: needinfo?(dwilson)
Reporter

Comment 2

5 years ago
John, Sotaro,
Could you help to check the log?
Whiteboard: [CR 737718]
Whiteboard: [CR 737718] → [caf priority: p2][CR 737718]

Comment 3

5 years ago
Please treat this as a blocking issue. Our FC milestone is already pastand this is a critical issue.
Randell, can you please reproduce this on a 8x26 device? As I mentioned earlier, Moz should be having a few devices.
I think this bug may be outside of the WebRTC code, but we'll take it for investigation.  If it is our bug, we'll fix it.  Assigning it to Randell for assessment.
Assignee: nobody → rjesup
Also, I'm told this is reproducible on the Flame.  We'll be testing and debugging this on a Flame.
Flags: needinfo?(sotaro.ikeda.g)
blocking-b2g: 2.1? → 2.1+

Updated

5 years ago
No longer blocks: CAF-v2.1-FC-metabug

Updated

5 years ago
:jesup, checking to see if you've investigated this and its a WebRTC issue, else we will have to find the culprit soon .
I'm trying to look at it.  Jay indicated that the front camera on the flame only goes to 16fps; we know the back camera chews about 10% cpu last I checked.

The logs here show ~20fps fed into webrtc; but it doesn't show anything about image capture.  We need logs with mediamanager:6 and getusermedia:5 to see all the logging for frame capture; there are separate logs for the B2G camera code.
Flags: needinfo?(rjesup)
mike - assuming the camera is being limited by something in 2.1 to 20fps, what might be the cause?  I'm assuming it's a camera issue, or a gralloc-buffer issue (recycling?)  I assume this would reproduce on a flame with the back camera in getUserMedia alone. (Jay, please verify using the log settings above.)

This doesn't seem to be webrtc given the logs posted by Jay, but appears to be camera or graphics related.  We need verification in a getUserMedia test.

At this point, I think we need the logs requested and/or steps to reproduce on a flame, and then to move it over to someone in b2g camera or gfx or both.


separate question for Mike: Is the camera hardware for Flame front camera limited to 16fps?  That would explain why I haven't seen anything over maybe 20fps; I assumed it was mostly because my office tends to be dark, especially at the hours I'm testing this.
Flags: needinfo?(mhabicher)
Flags: needinfo?(jaywang)
Reporter

Comment 9

5 years ago
Posted file 20fps_l2.zip
Attached file has the log with NSPR_LOG_MODULES=Camera:5,mediapipeline:6,signaling:6,WebrtcOMXH264VideoCodec:6,mediamanager:6,getusermedia:5

The log shows that 
"OnNewPreviewFrame: we have 1 preview frame listener(s)" gets called every 30-40mses, but somehow the frame is lost before reaching encoder.
Flags: needinfo?(jaywang)
Forces frames into MSG on reception with basically nil duration.  This means we'll still get called via NotifyPull, but NotifyPull will always just extend the frame's duration as needed.  Direct Listeners (like PeerConnection) will now get every frame input (and the duplicate AppendToTrack's caused by NotifyPull, but they get ignored).  

Before, the frames sat in a buffer until NotifyPull.  This was ok (though not great) when MSG ran off a 10ms timer.  However, with MSG being clocked by Audio callbacks, while it should be fairly frequent and regular (and is on desktop), the logs show that NotifyPull calls were highly jittered - some callbacks 2ms apart, others 125ms apart (which will drop ~2 frames).  The net result was 16-20fps from a 30fps camera stream.  

Interestingly, on 2.0 the FPS into MSG without this patch appeared to be 30fps (rear camera used alone), and 22fps when used with audio (gum_test.html, Video + Audio).   On 2.1, without this patch I see 30fps camera, ~16-17 fps into MSG (with audio).  With this patch, I see ~30fps into PeerConnection, though note that non-Direct listeners will get called back off NotifyQueuedTrackChanges, and I'm not sure if they'll see ~16fps or 30fps.  Switching to being a Direct Listener when possible would guarantee 30fps.

This is actually much closer to the design I wanted to use from the start for inputting video frames. :-)  It should help (slightly) desktop as well, and perhaps help Android in the same way as B2G, at least on lower-end phones.

I'd love to get rid of the extra callbacks, but given the duration-based design of MSG this is tough.

Note for testing: testing on a flame MUST be done using the back camera, and pointed at a bright object (bright screen will do).  Also note that PeerConnection testing should be done with a low-overhead testcase like webrtc-landing/pc_test.html with one-way-call and require h264 checked.  This is because the rear camera on the flame will eat ~10% of the CPU.
Flags: needinfo?(mhabicher)
Flags: needinfo?(jolin)
Comment on attachment 8510051 [details] [diff] [review]
Push video frames into MediaStreamGraph instead of waiting for pulls

Note that for testing the logging patch can be added, with 
NSPR_LOG_MODULES=timestamp,mediapipeline:5,mediamanager:5,camera:5
in /system/bin/b2g.sh

I'll look to clean up the logging so we can check it in as well

This patch may not be optimal; it tends to duplicate code instead of adding new functions, as we're looking to uplift to 2.1/beta.  We can do a cleanup pass as well for m-c.
Attachment #8510051 - Flags: review?(padenot)
Comment on attachment 8510051 [details] [diff] [review]
Push video frames into MediaStreamGraph instead of waiting for pulls

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

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +141,5 @@
> +      // This is safe from any thread, and is safe if the track is Finished
> +      // or Destroyed.
> +      mSources[i]->AppendToTrack(mTrackID, &(segment));
> +    }
> +  }

Can't you just put this block in a function so we avoid the duplication?
Attachment #8510051 - Flags: review?(padenot) → review+
cleaned up to remove duplication
Attachment #8510051 - Attachment is obsolete: true
Attachment #8510388 - Attachment is obsolete: true
Attachment #8510410 - Attachment is obsolete: true
Comment on attachment 8510412 [details] [diff] [review]
Push video frames into MediaStreamGraph instead of waiting for pulls (m-c/aurora version)

Mechanical merge to aurora/m-c - a refactor of MediaEngineVideo had landed, so the patch has to be split up into 3 cpp files.  Also turn down incessant logging of Audio NotifyPull unless we log at :6 (like video).
Attachment #8510412 - Flags: review?(padenot)
Comment on attachment 8510393 [details] [diff] [review]
Push video frames into MediaStreamGraph instead of waiting for pulls

carry forward r+ (note: patch applies against beta)
Attachment #8510393 - Flags: review+
Comment on attachment 8510393 [details] [diff] [review]
Push video frames into MediaStreamGraph instead of waiting for pulls

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 848954 (though it wasn't great to start)

User impact if declined: Limit on frames-per-second when capturing audio and video to 16-20fps

Testing completed: Manually tested on flame with back camera.  Patch (merged to aurora/m-c) ready to land there as well.

Risk to taking this patch (and alternatives if risky): very low risk; clones existing code so it can be called at another point.  Function is already thread-safe. Also avoids some minor video processing in the MSG thread (the input to the webrtc.org code queues it for some modifications/encode, but there's some processing that happens first).  Anything NOT done on MSG thread is a Good Thing.

String or UUID changes made by this patch: none.
Attachment #8510393 - Flags: approval-mozilla-b2g34?
Reporter

Comment 22

5 years ago
Just tried the patch on 8x26 setup and fps goes up to 30, now.
Comment on attachment 8510412 [details] [diff] [review]
Push video frames into MediaStreamGraph instead of waiting for pulls (m-c/aurora version)

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

rs=padenot
Attachment #8510412 - Flags: review?(padenot) → review+
Comment on attachment 8510412 [details] [diff] [review]
Push video frames into MediaStreamGraph instead of waiting for pulls (m-c/aurora version)

Approval Request Comment
[Feature/regressing bug #]: Bug 848954

[User impact if declined]: Lower framerate if audio callbacks aren't evenly spaced or frequent enough

[Describe test coverage new/current, TBPL]: Requires a manual test with real hardware.

[Risks and why]: Low risk; just adds another instance of a threadsafe call to push data into the graph.  Also probably also reduces the overhead on MSG some (moves it to the capture thread), which is always a good thing since MSG is hard-realtime.

[String/UUID change made/needed]: none
Attachment #8510412 - Flags: approval-mozilla-aurora?
Attachment #8510393 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
https://hg.mozilla.org/mozilla-central/rev/d4270e5224a9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (24Oct)
This is wanted for Aurora35 but not Beta34?
Flags: needinfo?(rjesup)
I would want it for Beta as well, as it will help there as well in some cases.  I figured 34 approval during beta would land on beta as well, sorry.
Flags: needinfo?(rjesup)
Comment on attachment 8510393 [details] [diff] [review]
Push video frames into MediaStreamGraph instead of waiting for pulls

Approval Request Comment
[Feature/regressing bug #]: bug 848954

[User impact if declined]: likely to help significantly on some Android devices; will help some on all desktops (frames get through earlier), might help more on some desktops (if their audio callbacks are >>10ms apart ever).

[Describe test coverage new/current, TBPL]: on m-c for some time, verified on b2g, path is hit by all webrtc tests that involve real or os-faked camera (fake:true streams bypass this currently; I may make them work the same way for better testing).

[Risks and why]: quite low risk at this point; see aurora notes

[String/UUID change made/needed]: None
Attachment #8510393 - Flags: approval-mozilla-beta?
Comment on attachment 8510393 [details] [diff] [review]
Push video frames into MediaStreamGraph instead of waiting for pulls

Beta+
Attachment #8510393 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8510412 [details] [diff] [review]
Push video frames into MediaStreamGraph instead of waiting for pulls (m-c/aurora version)

it's already landed so just getting the flags aligned correctly.
Attachment #8510412 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1122387
You need to log in before you can comment on or make changes to this bug.