We need to adapt the video resolution to be appropriate for the bitrate available, and also for CPU, and where needed, adjust the frame rate as well. A corollary is that we should (where possible) match the getUserMedia capture resolution to the sending resolution (same or larger, actually). This may be handled by getUserMedia "constraints" applied to an open stream.

very preliminary code for handling resolution via current bandwidth. Does not compile.

The general idea is to create an array of resolutions/fps and bitrates. Crossing the minimum bitrate for an entry (perhaps doing so for "long enough") going up will cause a switch to that resolution. Going down in bitrate, we have to drop 20% below the current resolution's minimum to change resolution - this provides some hysteresis, along with a minimum time between switches (though we might let that become a small number if we add other heuristics to help control things). The minimum time is also there to help dampen oscillations and to minimize the user impact of bitrate spikes and dips - shifting resolutions can be distracting. This array is a tuning parameter, and I would consider having this be a config value (in parsed text, like a list of width:height:fps:bitrate entries. Also, the current WIP patch has no hooks yet for CPU overload.

this patch now works, though it's not hooked up to the current bitrate measurements, and CPU overload isn't coded. Note that it doesn't include rotated resolutions for Android, which should be added.

Comment on attachment 756806 [details] [diff] [review] adapt sending resolution using the current assumed bitrate f? to derf for general feedback. Obviously there a lot of tuning params to play with here, and approaches to take. This is patterned after (though not copied from) resolution adjustment code I've done in the past.

Comment on attachment 756806 [details] [diff] [review] adapt sending resolution using the current assumed bitrate Review of attachment 756806 [details] [diff] [review]: ----------------------------------------------------------------- I don't think this is the right place to do this. GIPS already has resolution adaptation code which is significantly more sophisticated than this (taking the average motion level, spatial detail, etc., into account), and I really think we should be using that instead of trying to write our own. The main driver is in VCMQmResolution in media/webrtc/trunk/webrtc/modules/video_coding/main/source/qm_select.cc. Its use is controlled by VCMMediaOptimization::RegisterVideoQMCallback in video_coding/main/source/media_optimization.cc. That currently appears to be called in ViEEncoder::Init() in video_engine/vie_encoder.cc, which passes the value to the video processing module, which creates a spatial resampler to resize the video to the target resolution. The scary thing is that, AFAICT, this _already_ appears to be hooked up and active. Maybe I'm just reading the code wrong.

I'm hoping gcp can work on this after Bug 902000. If not, I'll find someone else on the team. I'd also love to get this done for Gecko 27, but I don't need it until Gecko 28. So that's why I'm setting the milestone to Gecko 28.

I've been spending some time on this now to see how the code works and try to figure out why upstream disables it by default, with the hope of modifying it to handle CPU and not just bandwidth overload in the future. This was complicated a bit by the arcane ways the codebase handles determining available bitrate, and currently I'm hitting a 200kbps lower bound set somewhere which in turn is still easily achievable on the puny video my Android phone puts out. So it's actually tricky to get this code to act. That as an aside. Upstream has been adding APIs to handle feeding CPU overload info into the engine: http://code.google.com/p/webrtc/source/browse/trunk/webrtc/video_engine/vie_base_impl.h?spec=svn5178&r=5178 However, they seem to handle it by checking when the frame grabber starts to slow down instead of measuring encoding directly: http://code.google.com/p/webrtc/issues/detail?can=2&q=2325&colspec=ID%20Pri%20Mstone%20ReleaseBlock%20Area%20Status%20Owner%20Summary&id=2325

So, the current code sortof works, but see the following. I'm emulating huge packet loss if > 75kbps, and lowered the minimum VP8 bitrate to 50kbps (The default minimum of 200kbps can easily be reached by the encoder without help for low-res low-fps Android images). Lowering bitrate as we discover our connection sucks: D/WEBRTC ( 9444): OnNetworkChanged(bitrate_bps: 73628, fraction_lost: 64, rtt_ms: 38 D/WEBRTC ( 9444): SetTargetRates: 73628 bps 64 loss 38 ms D/WEBRTC ( 9444): ProtectionRequest, deltaFECRate: 0, key_fecrate: 0, delta_use_uep_protection: 1, key_use_uep_protection: 0, delta_max_fec_frames: 1, key_max_fec_frames: 1, delta_mask_type: 0, key_mask_type: 0, D/WEBRTC ( 9444): SetTargetRates/enable_qm: 73.627998 bps 86.305000 kbps 10.111223 fps 64 loss D/WEBRTC ( 9444): SetTargetRates/selectquality D/WEBRTC ( 9444): SelectQuality D/WEBRTC ( 9444): SelectResolution D/WEBRTC ( 9444): SelectResolution::ComputeContentClass D/WEBRTC ( 9444): ComputeEncoderState==Stable D/WEBRTC ( 9444): avg_target_rate 128.046265 estimated_trans_rate_down 112.000000 max 280.000000 Stepping down resolution to make the target: D/WEBRTC ( 9444): OnNetworkChanged(bitrate_bps: 73848, fraction_lost: 5, rtt_ms: 1 D/WEBRTC ( 9444): SetTargetRates: 73848 bps 5 loss 1 ms D/WEBRTC ( 9444): ProtectionRequest, deltaFECRate: 0, key_fecrate: 0, delta_use_uep_protection: 1, key_use_uep_protection: 0, delta_max_fec_frames: 1, key_max_fec_frames: 1, delta_mask_type: 0, key_mask_type: 0, D/WEBRTC ( 9444): SetTargetRates/enable_qm: 73.848000 bps 63.695000 kbps 10.045203 fps 5 loss D/WEBRTC ( 9444): SetTargetRates/selectquality D/WEBRTC ( 9444): SelectQuality D/WEBRTC ( 9444): SelectResolution D/WEBRTC ( 9444): SelectResolution::ComputeContentClass D/WEBRTC ( 9444): ComputeEncoderState==Stressed D/WEBRTC ( 9444): avg_target_rate 73.834000 estimated_trans_rate_down 112.000000 max 280.000000 D/WEBRTC ( 9444): UpdateCodecResolution: [432 768] 432 768 => 324 576 D/WEBRTC ( 9444): Resolution change from QM select: W = 324, H = 576, FR = 30000.000000 So far so good, right. But then I put my phone down so it's all black, which is trivial to encode, and the following happens: D/WEBRTC ( 9444): OnNetworkChanged(bitrate_bps: 78325, fraction_lost: 5, rtt_ms: 42 D/WEBRTC ( 9444): SetTargetRates: 78325 bps 5 loss 42 ms D/WEBRTC ( 9444): ProtectionRequest, deltaFECRate: 0, key_fecrate: 0, delta_use_uep_protection: 1, key_use_uep_protection: 0, delta_max_fec_frames: 1, key_max_fec_frames: 1, delta_mask_type: 0, key_mask_type: 0, D/WEBRTC ( 9444): SetTargetRates/enable_qm: 39.162998 bps 9.154000 kbps 6.989247 fps 5 loss D/WEBRTC ( 9444): SetTargetRates/selectquality D/WEBRTC ( 9444): SelectQuality D/WEBRTC ( 9444): SelectResolution D/WEBRTC ( 9444): SelectResolution::ComputeContentClass D/WEBRTC ( 9444): ComputeEncoderState==Easy D/WEBRTC ( 9444): UpdateCodecResolution: [432 768] 324 576 => 432 768 D/WEBRTC ( 9444): Resolution change from QM select: W = 432, H = 768, FR = 30000.000000 Okay...it's easy and we're going back up. D/WEBRTC ( 9444): OnNetworkChanged(bitrate_bps: 78641, fraction_lost: 5, rtt_ms: 1 D/WEBRTC ( 9444): SetTargetRates: 78641 bps 5 loss 1 ms D/WEBRTC ( 9444): ProtectionRequest, deltaFECRate: 0, key_fecrate: 0, delta_use_uep_protection: 1, key_use_uep_protection: 0, delta_max_fec_frames: 1, key_max_fec_frames: 1, delta_mask_type: 0, key_mask_type: 0, D/WEBRTC ( 9444): SetTargetRates/enable_qm: 78.640999 bps 11.752000 kbps 6.889242 fps 5 loss D/WEBRTC ( 9444): SetTargetRates/selectquality D/WEBRTC ( 9444): SelectQuality D/WEBRTC ( 9444): SelectResolution D/WEBRTC ( 9444): SelectResolution::ComputeContentClass D/WEBRTC ( 9444): ComputeEncoderState==Easy D/WEBRTC ( 9444): avg_target_rate 68.426323 estimated_trans_rate_down 80.000000 max 200.000000 D/WEBRTC ( 9444): UpdateCodecResolution: [432 768] 432 768 => 324 576 D/WEBRTC ( 9444): Resolution change from QM select: W = 324, H = 576, FR = 30000.000000 It's easy and we reduce the resolution again. Oops! This is because our detected channel rate is below the minimum rate we require for this resolution, and it's doing this change regardless of encoder stress. But why did it raise the resolution in the first place then? I'll report upstream.

Upstream vie_base.h and vie_capturer.h/cc include code that registers media/webrtc/trunk/webrtc/video_engine/overuse_frame_detector.cc. This allows you to set a callback to be called in *your code* when the capturing code detects overload (see Comment 8). Interestingly, looking at their code, they move the logic to reduce the resolution to the application code, effectively duplicating much of the content analysis logic. I think I want to make a CPULoadAdaption class coupled with MediaEngineWebRTC, that receives callbacks both from LoadMonitor and the existing hooks in vie_base to detect an overload situation, and then add an api that gives back the current load situation to the media engines (i.e. so vie_capturer which is on the video side can give that info to the Opus encoder).

Apparently passing stuff from MediaEngine -> VideoConduit (or a place where we know the channel) is too much of a PITA in our current API. So let's get rid of that.

I'm now at the point where I'm introducing dependencies on LoadManager into the webrtc codebase, because ViEEncoder and friends need to register themselves to get callbacks when we go into stressed/relaxed states. 0:05.98 In file included from ../../../../../../dist/include/LoadMonitor.h:9:0, 0:05.98 from ../../../../../../dist/include/LoadManager.h:9, 0:05.98 from /home/morbo/hg/mozilla-central/media/webrtc/trunk/webrtc/video_engine/vie_base_impl.cc:32: 0:05.98 ../../../../../../dist/include/mozilla/Mutex.h:10:20: fatal error: prlock.h: No such file or directory 0:05.98 compilation terminated. Based on this, we've currently successfully avoided introducing any dependency on Mozilla stuff into much of the webrtc code. I'm wondering if I should I take that as a note that I probably want to split off even more interfaces so I can avoid that inclusion as well? Or is it really a non-issue? Food for thought.

Still WIP, but starting to get somewhere!

I think this is now in a reviewable state.

Relevant, because it breaks the current implementation of the patch. I'll see if I can make us reasonably robust to it. https://code.google.com/p/android/issues/detail?id=41630

There's some dead code in that logging, looks like damage from an uplift.

Comment on attachment 8384661 [details] [diff] [review] Patch 1.v2 Implement LoadManager. Update LoadMonitor. Add callbacks to ViEEncoder. Review of attachment 8384661 [details] [diff] [review]: ----------------------------------------------------------------- r+ with a few nits and requests for some discussion here and/or in the comments ::: content/media/webrtc/LoadManager.cpp @@ +61,5 @@ > + mLoadSumSeconds += seconds; > + mLoadSumMeasurements++; > + > + if (mLoadSumSeconds > kAveragingSeconds) { > + double averagedLoad = mLoadSum / (float)mLoadSumMeasurements; What happens when the system goes to sleep during measurement? Or is locked? Or hibernates? or NTP or setdate slews the clock? ::: content/media/webrtc/LoadManager.h @@ +35,5 @@ > + > +private: > + const float kHighLoadThreshold = 0.90f; > + const float kLowLoadThreshold = 0.40f; > + const int kAveragingSeconds = 3; Constants == evil. And worse we might find the best values vary by OS. I'd be very tempted to have these as configurable, at least for testing (perhaps a patch that makes them prefs, which we can run locally)? Or land that second patch and slate it for backout before beta. This will let us do much faster tuning ::: content/media/webrtc/LoadMonitor.h @@ +21,5 @@ > class LoadInfoUpdateRunner; > class LoadInfoCollectRunner; > > +// Update the system load every x milliseconds > +static const int kLoadUpdateInterval = 1000; perhaps later we should consider having this be configurable (or change the config var from a bool to an int (with 0 meaning off)) @@ +26,5 @@ > + > +class LoadNotificationCallback > +{ > +public: > + virtual void LoadChanged(float aSystemLoad, float aProcesLoad) = 0; typo - aProcessLoad ::: media/webrtc/trunk/webrtc/video_engine/vie_encoder.cc @@ +131,5 @@ > + : owner_(owner) { > + } > + virtual ~ViECPULoadStateObserver() {}; > + // Implements CPULoadStateObserver. > + virtual void onLoadStateChanged(CPULoadState state) { indentation ::: media/webrtc/trunk/webrtc/video_engine/vie_encoder.h @@ +200,4 @@ > scoped_ptr<ViEPacedSenderCallback> pacing_callback_; > > BitrateController* bitrate_controller_; > + CPULoadStateCallbackInvoker* load_manager_; See comment in vie_shared_data.h -- explain why this one can be a raw ptr ::: media/webrtc/trunk/webrtc/video_engine/vie_shared_data.cc @@ +53,2 @@ > last_error_ = 0; > return error; Side note (existing issue) - This SharedData seems like it's likely to be accessed on more than one thread. If not, great, In particular SetError would seem tsan-racey.... ::: media/webrtc/trunk/webrtc/video_engine/vie_shared_data.h @@ +54,5 @@ > scoped_ptr<ViEChannelManager> channel_manager_; > scoped_ptr<ViEInputManager> input_manager_; > scoped_ptr<ViERenderManager> render_manager_; > ProcessThread* module_process_thread_; > + CPULoadStateCallbackInvoker* load_manager_; Since this has a different lifetime than the other _manager_ pointers here, you should explain why a raw ptr is ok for this one.

Comment on attachment 8384662 [details] [diff] [review] Patch 2. Push load state to Media Optimization. Simple CPU adaption rules. Review of attachment 8384662 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/trunk/webrtc/modules/video_coding/main/source/qm_select.cc @@ +567,5 @@ > + && action_.temporal == kNoChangeTemporal > + && action_.spatial == kNoChangeSpatial) { > + // If FPS is high, allow dropping it to 20 fps. > + if (avg_incoming_framerate_ > 40) { > + action_.temporal = kOneHalfTemporal; } else if (avg_incoming_framerate_ >= 24) { action_.temporal = kTwoThirdsTemporal; an action of 2/3rds is not reduce by 2/3rds, but reduce by 1/3. Confusing... Also >= 40 please. I select 24 because a) some HD cams are 24fps, b) others are 25fps in europe, c) the measurement is likely noisy not theoretical, so I'm allowing a bit of slop below 30fps, especially if we're loaded). Worse case this will drop it to around 16fps. In most cases the camera will be giving 30fps or trying to, so we'll get around 20.

Comment on attachment 8384664 [details] [diff] [review] Patch 3. Enable QM/analsysis if load management is enabled. Review of attachment 8384664 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/trunk/webrtc/video_engine/vie_encoder.cc @@ +200,5 @@ > } > vpm_.EnableTemporalDecimation(true); > > + // Enable content analysis if load management enabled > + vpm_.EnableContentAnalysis(load_manager_ != nullptr); Normally I don't want ptr != nullptr, but it makes sense here

Comment on attachment 8384672 [details] [diff] [review] Patch 4. v2 Extra logging for adaption. Remove even more useless debug spam. Review of attachment 8384672 [details] [diff] [review]: ----------------------------------------------------------------- r+ with minor nits ::: media/webrtc/trunk/webrtc/modules/video_coding/main/source/media_optimization.cc @@ +85,5 @@ > uint32_t round_trip_time_ms) { > + WEBRTC_TRACE(webrtc::kTraceDebug, > + webrtc::kTraceVideoCoding, > + id_, > + "SetTargetRates: %d bps %d loss %d ms", %u bps %u%% lost %dms RTT ::: media/webrtc/trunk/webrtc/modules/video_coding/main/source/qm_select.cc @@ +215,5 @@ > int num_layers) { > + WEBRTC_TRACE(webrtc::kTraceDebug, > + webrtc::kTraceVideoCoding, > + -1, > + "qm_select.cc:initialize: %f %f %d %d", last are %u %u

>What happens when the system goes to sleep during measurement? Or is locked? Or hibernates? or NTP or setdate slews the clock? We're comparing deltas between between Linux kernel ticks, so that measurement is fine. The only thing that will happen is that we're base our measured load off of a single measurement instead of 3. Even that might be easy to protect with by measuring the number of measurements instead of the time. I'll put that in a separate patch.

Revised the first patch instead with some additional fixes. https://tbpl.mozilla.org/?tree=Try&rev=20046ec5054a

Comment on attachment 8389927 [details] [diff] [review] Patch 1. v3 Implement LoadManager. Update LoadMonitor. Add callbacks to ViEEncoder. Review of attachment 8389927 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h @@ +628,5 @@ > nsCOMPtr<nsIEventTarget> mSTSThread; > > + // CPU Load adaptation stuff. If only we had RefPtr with custom destructors > + // ala shared_ptr:-( > + mozilla::LoadManager* mLoadManager; What about using ScopedPtrs? Ala media/webrtc/signaling/src/common/MediaEngineWrapper.h (ScopedCustomReleasePtr()s) ::: media/webrtc/trunk/webrtc/video_engine/vie_shared_data.cc @@ +32,5 @@ > input_manager_->SetModuleProcessThread(module_process_thread_); > module_process_thread_->Start(); > } > > ViESharedData::~ViESharedData() { Since load_manager is owned by VIE encoder, can we assert it's null here when we tear this down? (and make sure when we kill it that we null it out by calling set_load_manager(nullptr).

>What about using ScopedPtrs? This doesn't work because it requires PeerConnection to know about the (details of the) destructor, which inevitably leads to having to know about the MOZILLA_INTERNAL_API stuff. padenot already suggested nsRefTraits which actually support doing what the comment laments about, but it has the same problem. Basically, PeerConnection absolutely can't know anything more about that field than that it's a pointer. I'll fix the comment. >Since load_manager is owned by VIE encoder It's owned by PeerConnection, so no. And I did actually document that in vie_shared_data :P

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/519ae59fa49e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d879d9643ed remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cb28fb0f9feb remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/41473d14f297

https://hg.mozilla.org/mozilla-central/rev/519ae59fa49e https://hg.mozilla.org/mozilla-central/rev/7d879d9643ed https://hg.mozilla.org/mozilla-central/rev/cb28fb0f9feb https://hg.mozilla.org/mozilla-central/rev/41473d14f297

Draft: Use OS X mach library calls to replace use of procs fs for system load information.