Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 4 months ago by Irfan Modified:
4 years, 3 months ago Reviewers:
philipel, danilchap, stefan-webrtc, terelius CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@fix_probing2 Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionProbing: Add support for exponential startup probing
Adds support for exponentially probing the bandwidth at start-up to allow
ramp-up to real capacity of the network.
BUG=webrtc:6332
R=philipel@webrtc.org, stefan@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/b2540bb99fa974bc8cb66eeff9c6b6d7fd0c36eb
Patch Set 1 #Patch Set 2 : Updates #
Total comments: 29
Patch Set 3 : Addressed comments #
Total comments: 19
Patch Set 4 : Addressed comments #
Total comments: 12
Patch Set 5 : Refactored probe_controller #
Total comments: 9
Patch Set 6 : Address comments and add unit tests #
Total comments: 6
Patch Set 7 : Addressed comments #
Total comments: 4
Patch Set 8 : Fixed nit #Patch Set 9 : lint fix #
Created: 4 years, 3 months ago
Messages
Total messages: 43 (12 generated)
Description was changed from ========== Probing: Add support for exponential startup probing Adds support for exponentially probing the bandwidth at start-up to allow ramp-up to real capacity of the network. BUG= ========== to ========== Probing: Add support for exponential startup probing Adds support for exponentially probing the bandwidth at start-up to allow ramp-up to real capacity of the network. BUG= ==========
isheriff@chromium.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org, terelius@webrtc.org
PTAL. I have rebased this on top of philip's CL. The tests will need fixes, but this is ready for review as an independent change. Note that I have not added support for multiple packets per probe yet. Planning to do that as a separate CL.
PS#2 adds some updates based on changes in https://codereview.chromium.org/2239143002/ I think I need to rebase on at least one CL that went in today. Looking forward to feedback before I go ahead and clean up tests and add some tests.
https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti... webrtc/modules/congestion_controller/delay_based_bwe.cc:128: // Overuse End the comments with "." https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti... webrtc/modules/congestion_controller/delay_based_bwe.cc:140: controller_->OnProbingBitrateMeasured(0); This looks a bit hacky. It's not really that we measured a zero bitrate on a probe. Should we have a separate method to stop probing, or a parameter to say so explicitly? https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti... webrtc/modules/congestion_controller/delay_based_bwe.cc:148: // Been a while since we updated Move this inside the else if below instead to make it clear that it doesn't belong to this block. https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:148: RTC_DCHECK(probing_state_ == State::kActive); Isn't it good to notify the prober about recent packets even though it's not active? That way it will know what's been sent before and can better recommend a packet size on line 142. https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b... File webrtc/modules/pacing/bitrate_prober_unittest.cc (right): https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober_unittest.cc:20: constexpr int probe_multipliers[2] = {3, 6}; kProbeMultipliers https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:340: min_bitrate_to_probe_further_ = 4 * estimated_bitrate_bps_; What is different between being here and in kStartupProbing? Here we have a factor of 4 while in startup we have a factor of 1.25? https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:369: } Isn't this adding a lot of complexity to paced_sender.cc when it could as well be in birtate_prober.cc? https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:436: estimated_bitrate_bps_ = bitrate_bps; I'm not sure this is the right way to set this. It will also be set via PacedSender::SetEstimatedBitrate(int bitrate_bps), and that estimate will possibly be different as it takes lost packets into account. Can we have a single way of setting the estimated bitrate to PacedSender? https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:438: LOG(LS_WARNING) << "OnProbingBitrateMeasured while not waiting on results"; Should we simply DCHECK on this? https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:440: } To keep the pacer simpler, I think it would be nice to move this into bitrate_prober.cc. WDYT? https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:528: if (probe_cluster_id != PacketInfo::kNotAProbe) { Why only notify about sent probe packets? https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... File webrtc/modules/pacing/paced_sender.h (right): https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.h:79: static const size_t kMinProbePacketSize = 1000; I'm not sure about introducing this change. It will break the behavior at low bitrates, e.g., if the bitrate is set to 300 kbps we may never have 1000 bytes packets. https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.h:188: int bitrate_below_expected_count_ GUARDED_BY(critsect_) = 0; Initialize in initializer list instead. https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/remote_b... File webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc (left): https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/remote_b... webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc:139: void TransportFeedbackAdapter::OnReceiveBitrateChanged( Nice to move this out.
https://codereview.webrtc.org/2235373004/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2235373004/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe.cc:121: probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info, 2); Seems like probe_bitrate_estimator.h/.cc haven't been uploaded, can you upload a patch with them included?
https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti... webrtc/modules/congestion_controller/delay_based_bwe.cc:121: probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info, 2); On 2016/08/16 14:44:43, philipel wrote: > Seems like probe_bitrate_estimator.h/.cc haven't been uploaded, can you upload a > patch with them included? https://codereview.chromium.org/2239143002/ is the CL which has changes in probe_bitrate_estimator.
https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti... webrtc/modules/congestion_controller/delay_based_bwe.cc:128: // Overuse On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote: > End the comments with "." Will make these full sentence comments and add full stop. https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti... webrtc/modules/congestion_controller/delay_based_bwe.cc:140: controller_->OnProbingBitrateMeasured(0); On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote: > This looks a bit hacky. It's not really that we measured a zero bitrate on a > probe. Should we have a separate method to stop probing, or a parameter to say > so explicitly? Will make it a well-defined name instead. Works for you ? https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti... webrtc/modules/congestion_controller/delay_based_bwe.cc:148: // Been a while since we updated On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote: > Move this inside the else if below instead to make it clear that it doesn't > belong to this block. Done. https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:148: RTC_DCHECK(probing_state_ == State::kActive); On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote: > Isn't it good to notify the prober about recent packets even though it's not > active? That way it will know what's been sent before and can better recommend a > packet size on line 142. We initiate probing only when there is an incoming packet > kMinProbePacketSize and is added to queue. The RecommendedPacketSize() is used for padding and should be triggered at the earliest on the second probe packet (since we have atleast one in queue). Once we initiate probing, we keep sending probes until done. The last_packet_size_ is used to decide on delta between probes (except on first probe when it is immediately sent). All this seems to indicate we really only need to track last_packet_size_ on the probes themselves. By doing so, we will be able to have stricter constraints on the method (add DCHECK on state we expect to be called) and calling only on probes keeps it simpler. Let me know if I have missed something here. I can revert this behavior if there is a problem. https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b... File webrtc/modules/pacing/bitrate_prober_unittest.cc (right): https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober_unittest.cc:20: constexpr int probe_multipliers[2] = {3, 6}; On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote: > kProbeMultipliers Done. https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:340: min_bitrate_to_probe_further_ = 4 * estimated_bitrate_bps_; On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote: > What is different between being here and in kStartupProbing? Here we have a > factor of 4 while in startup we have a factor of 1.25? The multiplers are 3 and 6 on the very first time (if you recall, we kept this to avoid slowing down ramp up). so we place a requirement of 4. Afterwards, the multiplier is 2 and we keep a requirement of 1.25. https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:369: } On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote: > Isn't this adding a lot of complexity to paced_sender.cc when it could as well > be in birtate_prober.cc? I will evaluate a way to simplify paced_sender and see if it works withm moving to bitrate_prober. https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:436: estimated_bitrate_bps_ = bitrate_bps; On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote: > I'm not sure this is the right way to set this. It will also be set via > PacedSender::SetEstimatedBitrate(int bitrate_bps), and that estimate will > possibly be different as it takes lost packets into account. Can we have a > single way of setting the estimated bitrate to PacedSender? This is a good point. The reason its happening here now is to sync up on bitrate the moment pacing state is updated. I will see if we can get rid of duplicate setting of bitrate. https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:438: LOG(LS_WARNING) << "OnProbingBitrateMeasured while not waiting on results"; On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote: > Should we simply DCHECK on this? Done. https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:440: } On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote: > To keep the pacer simpler, I think it would be nice to move this into > bitrate_prober.cc. WDYT? Right, will evaluate simplifiying pacer https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:528: if (probe_cluster_id != PacketInfo::kNotAProbe) { On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote: > Why only notify about sent probe packets? I clarified this in an earlier reply. Let me know if I have missed something here. https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... File webrtc/modules/pacing/paced_sender.h (right): https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.h:79: static const size_t kMinProbePacketSize = 1000; On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote: > I'm not sure about introducing this change. It will break the behavior at low > bitrates, e.g., if the bitrate is set to 300 kbps we may never have 1000 bytes > packets. I will go ahead and revert this. Since I plan to add support for multiple packets per probe, this should not be needed. https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.h:188: int bitrate_below_expected_count_ GUARDED_BY(critsect_) = 0; On 2016/08/16 11:27:05, stefan-webrtc (holmer) wrote: > Initialize in initializer list instead. Will do, but I seem to recall seeing in style this being encouraged (I cant find it now though)
PTAL. The big changes are moving most of the logic to bitrate_prober (thanks for the suggestion, it actually looks a lot cleaner) and removing redundant probing callback. I have also rebased on top of Philip's latest CL.
isheriff@chromium.org changed reviewers: + danilchap@webrtc.org
+ Danil in case he has time to look as well This is the design doc: https://docs.google.com/document/d/1CQHLFZhknzEGV9PoD0TD0tKZonnXAf8UzJyUcZ-dI...
I'm not deep enough in BWE part, so mostly style comments. Is it reasonable to ask to split this CL into refactoring for the change (like uint32_t->int for bitrate and routing UpdateDelayBasedEstimate singnal via CongestionController instead of TransportFeedbackAdapter) and the actual functional change? https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bwe.h (right): https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe.h:25: #include "webrtc/modules/congestion_controller/include/congestion_controller.h" prefer forward declaration over including header with full one. https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/include/congestion_controller.h (left): https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/include/congestion_controller.h:31: class BitrateController; Why this class need full instead of forward definition of BitrateController? https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:78: case State::kWaitForResult: shouldn't you start probing from State::kComplete too? https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:124: default: remove default. Missing state would be noticed at compile time, instead of runtime then. https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:130: RTC_DCHECK(probing_state_ != State::kDisabled); RTC_DCHECK_NE https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:176: if (packet_size_last_sent_ != 0 && probing_state_ == State::kSending) { probing_state == State::kSending seems no longer needed this line. https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:198: RTC_DCHECK(probing_state_ == State::kSending); RTC_DCHECK_EQ https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:208: RTC_DCHECK(probing_state_ == State::kSending); RTC_DCHECK_EQ https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.h (right): https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.h:27: enum class State { what move this enum type into public part? No public functions return or accept it as a parameter.
I moved the uint32-> int cleanup into a separate CL. Philip/Stefan, This is ready for a review before I can fix up the tests. PTAL. https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/congesti... File webrtc/modules/congestion_controller/delay_based_bwe.h (right): https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/congesti... webrtc/modules/congestion_controller/delay_based_bwe.h:25: #include "webrtc/modules/congestion_controller/include/congestion_controller.h" On 2016/08/19 17:19:34, danilchap wrote: > prefer forward declaration over including header with full one. Done https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/congesti... File webrtc/modules/congestion_controller/include/congestion_controller.h (left): https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/congesti... webrtc/modules/congestion_controller/include/congestion_controller.h:31: class BitrateController; On 2016/08/19 17:19:34, danilchap wrote: > Why this class need full instead of forward definition of BitrateController? due to use of std::unique_ptr<> I see complaints from compiler on my system. https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:78: case State::kWaitForResult: On 2016/08/19 17:19:34, danilchap wrote: > shouldn't you start probing from State::kComplete too? kComplete indicates we do not expect any results from probing and we are done with probing. kSending and kWaitForResults are basically states where the prober expects feedback to decide if it should ramp up again. so it makes sense to look at feedback only in these states. Let me know if that is not clear. https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:124: default: On 2016/08/19 17:19:34, danilchap wrote: > remove default. > Missing state would be noticed at compile time, instead of runtime then. Done. https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:130: RTC_DCHECK(probing_state_ != State::kDisabled); On 2016/08/19 17:19:34, danilchap wrote: > RTC_DCHECK_NE RTC_DCHECK_NE does not handle scoped enums properly. Something to look at outside this CL. https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:176: if (packet_size_last_sent_ != 0 && probing_state_ == State::kSending) { On 2016/08/19 17:19:34, danilchap wrote: > probing_state == State::kSending seems no longer needed this line. Done. https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:198: RTC_DCHECK(probing_state_ == State::kSending); On 2016/08/19 17:19:34, danilchap wrote: > RTC_DCHECK_EQ see above https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:208: RTC_DCHECK(probing_state_ == State::kSending); On 2016/08/19 17:19:34, danilchap wrote: > RTC_DCHECK_EQ see above https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b... File webrtc/modules/pacing/bitrate_prober.h (right): https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.h:27: enum class State { On 2016/08/19 17:19:34, danilchap wrote: > what move this enum type into public part? > No public functions return or accept it as a parameter. Done. It used to return in an earlier PS.
https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:78: case State::kWaitForResult: On 2016/08/23 05:46:57, Irfan wrote: > On 2016/08/19 17:19:34, danilchap wrote: > > shouldn't you start probing from State::kComplete too? > > kComplete indicates we do not expect any results from probing and we are done > with probing. > > kSending and kWaitForResults are basically states where the prober expects > feedback to decide if it should ramp up again. so it makes sense to look at > feedback only in these states. > > Let me know if that is not clear. So it is not a coincidence it is same states as in functions just above. May be then replace the switch with if (IsExpectingProbingResult()) { .... } https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe.cc:151: rtc::CritScope lock(&crit_); UpdateEsimate is called when this lock is hold already. Add EXCLUSIVE_LOCKS_REQUIRED(crit_) to it's definition in .h instead of retaking the lock here. https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:116: min_bitrate_to_probe_further_ = 4 * estimated_bitrate_bps_; it probably make more sense to derive this value from just added probe, not estimated_bitrate_bps_, Or derive probe bitrate from estimated_bitrate_bps_ too. e.g. const int kStartProbeBps = 1800000; CreateProbeCluster(kStartProbeBps / 2,...); CreateProbeCluster(kStartProbeBps,...); min_bitrate_to_probe_further_ = kStartProbeBps * 2 / 3; https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:122: if ((now_ms - time_last_probe_sent_ms_) > this check has nothing to do with IncomingPacket (a new media packet created to be paced). The kInit block uses only packet_size, The kWaitForResult block uses only now_ms. i.e. this two blocks of code feel too unrelated to be in same function. May be move this block into own function and call it from PacedSender::Process?
https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:78: void BitrateProber::SetEstimatedBitrate(int bitrate_bps) { In order to make the bitrate prober more general I would suggest we move the logic to perform exponential probing to the congestion controller (now that the delay based bwe takes a congestion controller instead of a remote bitrate observer). https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:85: if (min_bitrate_to_probe_further_ && min_bitrate_to_probe_further != kDisableExponentialProbing https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:112: CreateProbeCluster(900000, kProbeDeltasPerCluster + 1); Why move the creation of initial probe clusters here? https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:122: if ((now_ms - time_last_probe_sent_ms_) > I don't think this will ever occur. If we don't have packets to send we send padding instead and PacketSent is still called.
https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe.cc:131: remote_rate_.SetEstimate(probing_bps, info.arrival_time_ms); Should we DCHECK here somewhere that probing_bps isn't smaller than the current estimate? https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/include/congestion_controller.h (right): https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/include/congestion_controller.h:18: #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h" Why is this change needed? https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/include/congestion_controller.h:104: bool IsExpectingProbingResults(); I'm not super happy about making these public. These should be internal to the congestion controller and not be exposed. https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:78: void BitrateProber::SetEstimatedBitrate(int bitrate_bps) { On 2016/08/25 10:39:56, philipel wrote: > In order to make the bitrate prober more general I would suggest we move the > logic to perform exponential probing to the congestion controller (now that the > delay based bwe takes a congestion controller instead of a remote bitrate > observer). Do you mean that the CongestionController should be calling CreateProbeCluster() with exponentially increasing bitrates? I don't want to spread out the logic too much, but at the same time it would be good to don't make too many assumptions in this class, so if the amount of probing code needed in CongestionController is small I think that might be a good idea.
https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:78: void BitrateProber::SetEstimatedBitrate(int bitrate_bps) { On 2016/09/02 13:10:22, stefan-webrtc (holmer) wrote: > On 2016/08/25 10:39:56, philipel wrote: > > In order to make the bitrate prober more general I would suggest we move the > > logic to perform exponential probing to the congestion controller (now that > the > > delay based bwe takes a congestion controller instead of a remote bitrate > > observer). > > Do you mean that the CongestionController should be calling CreateProbeCluster() > with exponentially increasing bitrates? > > I don't want to spread out the logic too much, but at the same time it would be > good to don't make too many assumptions in this class, so if the amount of > probing code needed in CongestionController is small I think that might be a > good idea. To me it looks like this could be implemented either in CongestionController::Process or in CongestionController::MaybeTriggerOnNetworkChanged. I think it's important to keep the "lego blocks" clean so they are both easy to understand but also to avoid having to refactor even the most basic components in the case of anyone wanting to change the probing behavior.
PTAL. A couple of follow up fixes needed to make the capacity ramp up: 1. https://codereview.chromium.org/2269873002/ or something along those lines is needed to make the start up exponential probing work 2. Multiple packets per probe to go > 6 Mbps
lg, just a few comments. https://codereview.webrtc.org/2235373004/diff/80001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2235373004/diff/80001/webrtc/modules/congestion... webrtc/modules/congestion_controller/congestion_controller.cc:318: void CongestionController::OnDelayBasedBweChanged(int bitrate_bps) { Where can I find the changes to the .h file? https://codereview.webrtc.org/2235373004/diff/80001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2235373004/diff/80001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:32: constexpr int kDisableExponentialProbing = 0; kExponentialProbingDisabled https://codereview.webrtc.org/2235373004/diff/80001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:109: ((clock_->TimeInMilliseconds() - time_last_probing_initiated_ms_) > Remove Process() and add the timeout check to SetEstimatedBitrate. https://codereview.webrtc.org/2235373004/diff/80001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller.h (right): https://codereview.webrtc.org/2235373004/diff/80001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.h:45: kWaitForProbingResult, kWaitingForProbingResult
Added a few unit tests and reverted using observer on delay_based_bwe to allow tests to be same https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti... webrtc/modules/congestion_controller/congestion_controller.cc:318: void CongestionController::OnDelayBasedBweChanged(int bitrate_bps) { On 2016/09/08 12:52:46, philipel wrote: > Where can I find the changes to the .h file? The .h file is part of this patchset, its in include/ https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti... webrtc/modules/congestion_controller/probe_controller.cc:32: constexpr int kDisableExponentialProbing = 0; On 2016/09/08 12:52:47, philipel wrote: > kExponentialProbingDisabled Done. https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti... webrtc/modules/congestion_controller/probe_controller.cc:109: ((clock_->TimeInMilliseconds() - time_last_probing_initiated_ms_) > On 2016/09/08 12:52:47, philipel wrote: > Remove Process() and add the timeout check to SetEstimatedBitrate. Done. https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti... File webrtc/modules/congestion_controller/probe_controller.h (right): https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti... webrtc/modules/congestion_controller/probe_controller.h:45: kWaitForProbingResult, On 2016/09/08 12:52:47, philipel wrote: > kWaitingForProbingResult Done.
lgtm with the thread annotation fix. https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti... webrtc/modules/congestion_controller/congestion_controller.cc:318: void CongestionController::OnDelayBasedBweChanged(int bitrate_bps) { On 2016/09/09 08:12:52, Irfan wrote: > On 2016/09/08 12:52:46, philipel wrote: > > Where can I find the changes to the .h file? > > The .h file is part of this patchset, its in include/ Oops https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest... File webrtc/modules/congestion_controller/delay_based_bwe.h (right): https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest... webrtc/modules/congestion_controller/delay_based_bwe.h:64: uint32_t* target_bitrate_bps); Add EXCLUSIVE_LOCKS_REQUIRED(crit_) annotation and remove the CritScope. https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest... File webrtc/modules/congestion_controller/probe_controller.h (right): https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest... webrtc/modules/congestion_controller/probe_controller.h:37: void InitiateProbing(std::initializer_list<int> bitrates_to_probe, What is the reason we use an std::initializer_list instead of a std::vector?
One more thing. Irfan, can you create a bug so we can track all CLs related to this feature?
Description was changed from ========== Probing: Add support for exponential startup probing Adds support for exponentially probing the bandwidth at start-up to allow ramp-up to real capacity of the network. BUG= ========== to ========== Probing: Add support for exponential startup probing Adds support for exponentially probing the bandwidth at start-up to allow ramp-up to real capacity of the network. BUG=webrtc:6332 ==========
On 2016/09/09 09:05:18, philipel wrote: > One more thing. Irfan, can you create a bug so we can track all CLs related to > this feature? Tracking bug here: https://bugs.chromium.org/p/webrtc/issues/detail?id=6332 Feel free to create blocking bugs for sub tasks as you see fit.
https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest... webrtc/modules/congestion_controller/congestion_controller.cc:259: RemoteBitrateEstimator* rbe = new DelayBasedBwe(this, clock_); It's not clear to me why we pass in a CongestionController to DelayBasedBwe instead of a BitrateController. Seems to just pollute the CongestionController interface. I think it'd be better to have BitrateController the interface needed.
https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest... webrtc/modules/congestion_controller/congestion_controller.cc:259: RemoteBitrateEstimator* rbe = new DelayBasedBwe(this, clock_); On 2016/09/09 12:03:52, stefan-webrtc (holmer) wrote: > It's not clear to me why we pass in a CongestionController to DelayBasedBwe > instead of a BitrateController. Seems to just pollute the CongestionController > interface. > > I think it'd be better to have BitrateController the interface needed. I have gone and ahead and taken your suggestion. There were a couple of things abstracting this out of BitrateController did: adding a clearer API about it being a delay based estimate and removing the redundant ssrc list. Nevertheless, I have added comments to clear that up. https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest... File webrtc/modules/congestion_controller/delay_based_bwe.h (right): https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest... webrtc/modules/congestion_controller/delay_based_bwe.h:64: uint32_t* target_bitrate_bps); On 2016/09/09 08:59:09, philipel wrote: > Add EXCLUSIVE_LOCKS_REQUIRED(crit_) annotation and remove the CritScope. Done. https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest... File webrtc/modules/congestion_controller/probe_controller.h (right): https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest... webrtc/modules/congestion_controller/probe_controller.h:37: void InitiateProbing(std::initializer_list<int> bitrates_to_probe, On 2016/09/09 08:59:09, philipel wrote: > What is the reason we use an std::initializer_list instead of a std::vector? It is a light weight proxy for {} list. std::vector actually takes an std::initializer_list and constructs a vector. Since we just need an unmodifiable list, this suffices.
lgtm % nit https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate... File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right): https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate... webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:194: // This is called for BW estimate from a send-side estimator. Perhaps make this a TODO and mention that we should introduce a new observer for DelayBasedBwe which is independent of RemoteBitrateEstimator.
https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate... File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right): https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate... webrtc/modules/bitrate_controller/include/bitrate_controller.h:23: #include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" Include order.
https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate... File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right): https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate... webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:194: // This is called for BW estimate from a send-side estimator. On 2016/09/12 13:58:29, stefan-webrtc (holmer) wrote: > Perhaps make this a TODO and mention that we should introduce a new observer for > DelayBasedBwe which is independent of RemoteBitrateEstimator. Done. https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate... File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right): https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate... webrtc/modules/bitrate_controller/include/bitrate_controller.h:23: #include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" On 2016/09/12 13:59:28, stefan-webrtc (holmer) wrote: > Include order. Done.
The CQ bit was checked by isheriff@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.chromium.org/2235373004/#ps140001 (title: "Fixed nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8284)
The CQ bit was checked by isheriff@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.chromium.org/2235373004/#ps160001 (title: "lint fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Message was sent while issue was closed.
Description was changed from ========== Probing: Add support for exponential startup probing Adds support for exponentially probing the bandwidth at start-up to allow ramp-up to real capacity of the network. BUG=webrtc:6332 ========== to ========== Probing: Add support for exponential startup probing Adds support for exponentially probing the bandwidth at start-up to allow ramp-up to real capacity of the network. BUG=webrtc:6332 R=philipel@webrtc.org, stefan@webrtc.org Committed: https://crrev.com/b2540bb99fa974bc8cb66eeff9c6b6d7fd0c36eb Cr-Commit-Position: refs/heads/master@{#14189} ==========
Message was sent while issue was closed.
Description was changed from ========== Probing: Add support for exponential startup probing Adds support for exponentially probing the bandwidth at start-up to allow ramp-up to real capacity of the network. BUG=webrtc:6332 R=philipel@webrtc.org, stefan@webrtc.org Committed: https://crrev.com/b2540bb99fa974bc8cb66eeff9c6b6d7fd0c36eb Cr-Commit-Position: refs/heads/master@{#14189} ========== to ========== Probing: Add support for exponential startup probing Adds support for exponentially probing the bandwidth at start-up to allow ramp-up to real capacity of the network. BUG=webrtc:6332 R=philipel@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/b2540bb99fa974bc8cb66eeff... ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b2540bb99fa974bc8cb66eeff9c6b6d7fd0c36eb Cr-Commit-Position: refs/heads/master@{#14189}
Message was sent while issue was closed.
Committed patchset #9 (id:160001) manually as b2540bb99fa974bc8cb66eeff9c6b6d7fd0c36eb (presubmit successful). |
