Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2038023002:
Use |probe_cluster_id| to cluster packets. (Closed)
DescriptionUse |probe_cluster_id| to cluster packets.
Introduced new class DelayBasedProbingEstimator which is a copy of
RemoteBitrateEstimatorAbsSendTime with only minor changes. This makes probing
more reliable but is still not usable for mid-call probing.
BUG=
Committed: https://crrev.com/863a8264cc0d8499fb7b666cb8be868ab95b1bff
Cr-Commit-Position: refs/heads/master@{#13195}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Moved Probe/Cluster class into DelayBasedBwe. #
Total comments: 12
Patch Set 3 : Feedback fixes. #Patch Set 4 : Added probing unittests to DelayBasedBwe and feedback fixes. #
Total comments: 15
Patch Set 5 : Feedback fixes. #Patch Set 6 : BUILD file... #Patch Set 7 : BUILD file fix. #
Total comments: 3
Patch Set 8 : Updated comments. #Patch Set 9 : rebase #Patch Set 10 : Optional<T> fix #
Messages
Total messages: 44 (17 generated)
philipel@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
We should probably bring along unittests and update webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.h / .cc to also use this class. https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/congestion_controller.cc:23: #include "webrtc/modules/congestion_controller/delay_based_bitrate_probing.h" I'd prefer if we didn't have probing in the name. It's main purpose is to do delay-based bwe. delay_based_bandwidth_estimator.h maybe? https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc (right): https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc:105: if (it->probe_cluster_id == last_probe_cluster_id) { Could you perhaps introduce these changes in a separate CL, so that the refactoring is easier to review?
https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc (right): https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc:48: namespace webrtc { Why keep this constant and function in webrtc instead of unnamed namespace? They would likely collide with same-named from RemoteBitrateEstimatorAbsSendTime.cc https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc:84: // XXX(philipel): The BitrateEstimatorTest relies on this EXACT log line. XXX=TODO? ( fix the test) https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc:215: // XXX(philipel): The BitrateEstimatorTest relies on this EXACT log line. ditto https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bitrate_probing.h (right): https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bitrate_probing.h:33: struct Probe { that make two webrtc::Probe structures with different size/constructors. (one here, another one - in RemoteBitrateEstimatorAbsSen.h) Source for unnatural, hard to find bugs. you can fix it by making Probe/Cluster sturtures member classes of DelayBasedProbingEstimator. Might be even better to forward-declare them inside the class and define in .cc file.
https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc (right): https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc:84: // XXX(philipel): The BitrateEstimatorTest relies on this EXACT log line. On 2016/06/03 14:11:45, danilchap wrote: > XXX=TODO? ( fix the test) The test is only useful for the receive-side BWE, so we should be able to remove this on the send-side.
https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/congestion_controller.cc:23: #include "webrtc/modules/congestion_controller/delay_based_bitrate_probing.h" On 2016/06/03 13:46:23, stefan-webrtc (holmer) wrote: > I'd prefer if we didn't have probing in the name. It's main purpose is to do > delay-based bwe. delay_based_bandwidth_estimator.h maybe? Renamed files to delay_based_bitrate_probing.h/cc and the class to DelayBasedBwe. https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc (right): https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc:48: namespace webrtc { On 2016/06/03 14:11:45, danilchap wrote: > Why keep this constant and function in webrtc instead of unnamed namespace? > They would likely collide with same-named from > RemoteBitrateEstimatorAbsSendTime.cc Wanted to do as few changes as possible, but I guess it makes sense to move them to the anonymous namespace as well. https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc:84: // XXX(philipel): The BitrateEstimatorTest relies on this EXACT log line. On 2016/06/03 14:11:45, danilchap wrote: > XXX=TODO? ( fix the test) XXX = dirty code. I think it is best to not change the existing unit tests for the sake of this class, but to refactor and write new tests for this class. Send side and receive side BWE is starting to diverge. Stefan WDYT? https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc:105: if (it->probe_cluster_id == last_probe_cluster_id) { On 2016/06/03 13:46:23, stefan-webrtc (holmer) wrote: > Could you perhaps introduce these changes in a separate CL, so that the > refactoring is easier to review? Not 100% sure what you mean. This logic must be changed in order to actually use cluster ids instead of guessing. https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc:215: // XXX(philipel): The BitrateEstimatorTest relies on this EXACT log line. On 2016/06/03 14:11:45, danilchap wrote: > ditto Acknowledged. https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bitrate_probing.h (right): https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bitrate_probing.h:33: struct Probe { On 2016/06/03 14:11:45, danilchap wrote: > that make two webrtc::Probe structures with different size/constructors. > (one here, another one - in RemoteBitrateEstimatorAbsSen.h) > Source for unnatural, hard to find bugs. > you can fix it by making Probe/Cluster sturtures member classes of > DelayBasedProbingEstimator. > Might be even better to forward-declare them inside the class and define in .cc > file. Good point, moved Probe and Cluster into DelayBasedBwe. Can't be forward declared though since we need the complete type in the .h file.
https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.cc (right): https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.cc:47: static const double kTimestampToMs = to make review easier, try keep the order same as in original file: (in this case kTimestampToMs, template<>Keys, ConvertMsTo24Bits) unless you have reasons to change the order. https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.h (right): https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.h:33: class DelayBasedBwe : public RemoteBitrateEstimator { it is easiser to follow code when class name match filename, i.e. rename class to DelayBasedBandwidthEstimator or file to delay_based_bwe.h (or both of them to something like LocalBandwidthEstimatorDelayBased) https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.h:70: int probe_cluster_id; this code (specially this struct) already knows it is about probing. May be name member cluster_id to make it shorter https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.h:73: struct Cluster { Probe can't be easily forward-declared, but Cluster can. Not sure if you want to start to move classes around in this or next CL thought.
https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc (right): https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc:84: // XXX(philipel): The BitrateEstimatorTest relies on this EXACT log line. On 2016/06/03 14:54:55, philipel wrote: > On 2016/06/03 14:11:45, danilchap wrote: > > XXX=TODO? ( fix the test) > > XXX = dirty code. I think it is best to not change the existing unit tests for > the sake of this class, but to refactor and write new tests for this class. Send > side and receive side BWE is starting to diverge. Stefan WDYT? I think that the particular test that relies on this log line provides no value on the send-side, so we shouldn't need to run it there. Can we adapt it so that it only runs with receive-side BWE? https://codereview.webrtc.org/2038023002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc:105: if (it->probe_cluster_id == last_probe_cluster_id) { On 2016/06/03 14:54:55, philipel wrote: > On 2016/06/03 13:46:23, stefan-webrtc (holmer) wrote: > > Could you perhaps introduce these changes in a separate CL, so that the > > refactoring is easier to review? > > Not 100% sure what you mean. This logic must be changed in order to actually use > cluster ids instead of guessing. What I meant was that you could make a separate CL for simply copying the old class / renaming, but without any functionality changes. That would be easier to review. https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.cc (left): https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.cc:256: // larger than 200 bytes are paced by the sender. Should we keep this comment at line 255? https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.cc (right): https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.cc:102: if (it->probe_cluster_id == last_probe_cluster_id) { Is this correct? Seems like this is equal to IsWithinClusterBounds(), rather than !IsWithinClusterBounds()? This also makes me think we need tests for this new class. We should bring some along from the old class.
https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.cc (left): https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.cc:256: // larger than 200 bytes are paced by the sender. On 2016/06/08 08:57:09, stefan-webrtc (holmer) wrote: > Should we keep this comment at line 255? Done. https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.cc (right): https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.cc:47: static const double kTimestampToMs = On 2016/06/03 16:25:28, danilchap wrote: > to make review easier, try keep the order same as in original file: (in this > case kTimestampToMs, template<>Keys, ConvertMsTo24Bits) > unless you have reasons to change the order. Order fixed. https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.cc:102: if (it->probe_cluster_id == last_probe_cluster_id) { On 2016/06/08 08:57:09, stefan-webrtc (holmer) wrote: > Is this correct? Seems like this is equal to IsWithinClusterBounds(), rather > than !IsWithinClusterBounds()? > > This also makes me think we need tests for this new class. We should bring some > along from the old class. This is not correct, fixed. https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.h (right): https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.h:33: class DelayBasedBwe : public RemoteBitrateEstimator { On 2016/06/03 16:25:29, danilchap wrote: > it is easiser to follow code when class name match filename, i.e. > rename class to DelayBasedBandwidthEstimator or file to delay_based_bwe.h > (or both of them to something like LocalBandwidthEstimatorDelayBased) Renamed files to delay_based_bwe.cc/h. https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.h:70: int probe_cluster_id; On 2016/06/03 16:25:29, danilchap wrote: > this code (specially this struct) already knows it is about probing. May be name > member cluster_id to make it shorter Done. https://codereview.webrtc.org/2038023002/diff/10005/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bandwidth_estimator.h:73: struct Cluster { On 2016/06/03 16:25:28, danilchap wrote: > Probe can't be easily forward-declared, but Cluster can. > Not sure if you want to start to move classes around in this or next CL thought. I would keep them grouped rather than to have one in the .h file and the other in the .cc file. If we want to move it lets do it in another CL.
https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/congestion_controller.cc (left): https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/congestion_controller.cc:118: rbe_.reset(new RemoteBitrateEstimatorAbsSendTime(observer_)); isn't WrappingBitrateEstimator a receive-side bwe? Likely should keep using RemoteBitrateEstimatorAbsSendTime here then. It seems send-side bwe uses transport_feedback_adapter. https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (right): https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:18: class TestDelayBasedBwe : public ::testing::Test, public RemoteBitrateObserver { DelayBasedBweTest https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:69: const int kProbeLength = 5; constexpr may be name it kNumberOfProbes (ProbeLength gives false impression it's a length of something, e.g. may be understood as size of a probe in bytes) this constant appear in every test, may be move it into the test class https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:105: IncomingPacket(0, 100, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), may be use regular size packets for this test. (since there is a dedicated test that small-size packets are not used for probing) https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:195: TEST_F(TestDelayBasedBwe, TestProbeDetectionSlowerArrivalHighBitrate) { Either all tests should start with word 'Test' (like this test) or none (like next test), preferably the latter. https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:221: IncomingPacket(0, 200, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), true, may be better use PacedSender::kMinProbePacketSize, instead of magical number 200 https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:232: IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), May be use (kMinProbePacketSize+1) instead of 1000 (and adjust expected bitrate accordingly)
https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/congestion_controller.cc (left): https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/congestion_controller.cc:118: rbe_.reset(new RemoteBitrateEstimatorAbsSendTime(observer_)); On 2016/06/08 13:20:07, danilchap wrote: > isn't WrappingBitrateEstimator a receive-side bwe? Likely should keep using > RemoteBitrateEstimatorAbsSendTime here then. > It seems send-side bwe uses transport_feedback_adapter. Very true, the RemoteBitrateEstimatorAbsSendTime should be used here. https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (right): https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:18: class TestDelayBasedBwe : public ::testing::Test, public RemoteBitrateObserver { On 2016/06/08 13:20:07, danilchap wrote: > DelayBasedBweTest Hmm... I usually use this format. Doing 'git grep "TEST_.(Test" | wc' gives 345 lines and 'git grep -E "TEST_.\(.+Test\)" | wc' gives 90 lines, so I don't think it is uncommon to use Test as the prefix. https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:69: const int kProbeLength = 5; On 2016/06/08 13:20:07, danilchap wrote: > constexpr > may be name it kNumberOfProbes (ProbeLength gives false impression it's a length > of something, e.g. may be understood as size of a probe in bytes) > this constant appear in every test, may be move it into the test class Done. https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:105: IncomingPacket(0, 100, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), On 2016/06/08 13:20:07, danilchap wrote: > may be use regular size packets for this test. (since there is a dedicated test > that small-size packets are not used for probing) Done. https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:195: TEST_F(TestDelayBasedBwe, TestProbeDetectionSlowerArrivalHighBitrate) { On 2016/06/08 13:20:08, danilchap wrote: > Either all tests should start with word 'Test' (like this test) or none (like > next test), preferably the latter. Done. https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:221: IncomingPacket(0, 200, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), true, On 2016/06/08 13:20:08, danilchap wrote: > may be better use PacedSender::kMinProbePacketSize, instead of magical number > 200 Done. https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:232: IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), On 2016/06/08 13:20:07, danilchap wrote: > May be use (kMinProbePacketSize+1) instead of 1000 (and adjust expected bitrate > accordingly) I agree that is a good idea, but these tests are kind of temporary. The DelayBasedBwe class will change more and more and I think at that point it is a good idea to clean/implement more tests.
lgtm https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (right): https://codereview.webrtc.org/2038023002/diff/50001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:18: class TestDelayBasedBwe : public ::testing::Test, public RemoteBitrateObserver { On 2016/06/08 14:34:40, philipel wrote: > On 2016/06/08 13:20:07, danilchap wrote: > > DelayBasedBweTest > > Hmm... I usually use this format. Doing 'git grep "TEST_.(Test" | wc' gives 345 > lines and 'git grep -E "TEST_.\(.+Test\)" | wc' gives 90 lines, so I don't think > it is uncommon to use Test as the prefix. it also good idea to do a local style search: both RemoteBitrateEstimatorAbsSendTime you copy tests from and neighbor CongestionController unittests use Test as suffix, not prefix.
The CQ bit was checked by philipel@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038023002/70001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/369) linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
The CQ bit was checked by philipel@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038023002/90001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/370) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/3814) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/10032)
The CQ bit was checked by philipel@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038023002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
philipel@webrtc.org changed reviewers: + mflodman@webrtc.org
Magnus, PTAL
One real comment, then LGTM. https://codereview.webrtc.org/2038023002/diff/110001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2038023002/diff/110001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:82: // XXX(philipel): The BitrateEstimatorTest relies on this EXACT log line. I"d prefer to change this comment to not have XXX(philipel), but something else instead. https://codereview.webrtc.org/2038023002/diff/110001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:212: // XXX(philipel): The BitrateEstimatorTest relies on this EXACT log line. Same here. https://codereview.webrtc.org/2038023002/diff/110001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:229: // XXX(philipel): The BitrateEstimatorTest relies on this EXACT log line. And here.
Comments fixed.
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2038023002/#ps130001 (title: "Updated comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038023002/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_x64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) android_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/1...) android_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/1...) ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/8357) ios64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/612) ios64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/615) ios_api_framework on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/10674) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/10597) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/15903) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/14549) linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/15566) linux_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...) linux_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...) linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) linux_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/16014) linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/2905) mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/12192) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/4062) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/10278) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/16000) presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/6380) win_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_compile_dbg/builds/...) win_x64_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...) win_x64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_rel/builds/1...)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2038023002/#ps150001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038023002/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/8358) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/8357) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/10675) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/15904)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2038023002/#ps170001 (title: "Optional<T> fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038023002/170001
Message was sent while issue was closed.
Committed patchset #10 (id:170001)
Message was sent while issue was closed.
Description was changed from ========== Use |probe_cluster_id| to cluster packets. Introduced new class DelayBasedProbingEstimator which is a copy of RemoteBitrateEstimatorAbsSendTime with only minor changes. This makes probing more reliable but is still not usable for mid-call probing. BUG= ========== to ========== Use |probe_cluster_id| to cluster packets. Introduced new class DelayBasedProbingEstimator which is a copy of RemoteBitrateEstimatorAbsSendTime with only minor changes. This makes probing more reliable but is still not usable for mid-call probing. BUG= Committed: https://crrev.com/863a8264cc0d8499fb7b666cb8be868ab95b1bff Cr-Commit-Position: refs/heads/master@{#13195} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/863a8264cc0d8499fb7b666cb8be868ab95b1bff Cr-Commit-Position: refs/heads/master@{#13195} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
