Jay Reporter
	Description • 10 years ago

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

	Jay Reporter
	Comment 1 • 10 years ago

[Blocking Requested - why for this release]:

	Jay Reporter
	Comment 2 • 10 years ago

John, Sotaro,
Could you help to check the log?

	Mandyam Vikram
	Comment 3 • 10 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.

	Maire Reavy [:mreavy]
	Comment 4 • 10 years ago

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.

	Maire Reavy [:mreavy]
	Comment 5 • 10 years ago

Also, I'm told this is reproducible on the Flame.  We'll be testing and debugging this on a Flame.

	bhavana bajaj [:bajaj]
	Comment 6 • 10 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 .

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 7 • 10 years ago

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.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 8 • 10 years ago

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.

	Jay Reporter
	Comment 9 • 10 years ago

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.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 12 • 10 years ago

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.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 13 • 10 years ago

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.

	Paul Adenot (:padenot)
	Comment 14 • 10 years ago

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?

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 15 • 10 years ago

cleaned up to remove duplication

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 16 • 10 years ago

minor cleanups

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 18 • 10 years ago

add smidge of audio logging cleanup as well

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 19 • 10 years ago

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).

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 20 • 10 years ago

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)

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 21 • 10 years ago

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.

	Jay Reporter
	Comment 22 • 10 years ago

Just tried the patch on 8x26 setup and fps goes up to 30, now.

	Paul Adenot (:padenot)
	Comment 23 • 10 years ago

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 24 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4270e5224a9

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 25 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 26 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 10 years ago

This is wanted for Aurora35 but not Beta34?

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 28 • 10 years ago

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.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 29 • 10 years ago

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 30 • 10 years ago

Comment on attachment 8510393 [details] [diff] [review]
Push video frames into MediaStreamGraph instead of waiting for pulls

Beta+

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 10 years ago

I'm assuming the Beta approval covers Aurora as well.

https://hg.mozilla.org/releases/mozilla-aurora/rev/746e357374f2
https://hg.mozilla.org/releases/mozilla-beta/rev/22cfde2bf1ce

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/22cfde2bf1ce

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 33 • 10 years ago

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.