Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2234363002:
Removed old probe cluster logic and logic related to ssrcs from DelayBasedBwe. (Closed)
DescriptionRemoved old probe cluster logic and logic related to ssrcs from DelayBasedBwe.
-Removed the old probe cluster logic and use the new ProbeBitrateEstimator
instead.
-Removed all logic related to ssrcs from DelayBasedBwe as they have no function
on the sender side.
BUG=webrtc:5859
R=stefan@webrtc.org, terelius@webrtc.org
Committed: https://crrev.com/7522a2805118b561322a9ac7d37cec8e8eea06e3
Cr-Commit-Position: refs/heads/master@{#13771}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Feedback fixes. #Patch Set 3 : Added comment for kFixedSsrc. #
Total comments: 2
Patch Set 4 : Feedback fixes. #Patch Set 5 : Nit fix. #Patch Set 6 : Win fix. #Patch Set 7 : Windows WTF fix... #
Dependent Patchsets: Messages
Total messages: 28 (12 generated)
philipel@webrtc.org changed reviewers: + stefan@webrtc.org, terelius@webrtc.org
https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:47: estimator_(new OveruseEstimator(OverUseDetectorOptions())), For |inter_arrival_| and |estimator_| I used the same values as in the now removed function TimeOutStreams (line 327) https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:89: if (now_ms - last_seen_packet_ms_ > kStreamTimeOutMs) { Checking and resetting was done in the now removed function TimeOutStreams (line 327) https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:106: uint32_t send_time_24bits = Earlier this was calculated using the ConvertMsTo24Bits function, but is now inlined here instead. https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (left): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:70: TEST_F(DelayBasedBweTest, ProbeDetectionTooHighBitrate) { All removed tests have an equivalent test in probe_bitrate_estimator_unittest.cc instead, except the ProbingIgnoresSmallPackets since we don't ignore small packets anymore.
https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:57: LOG(LS_INFO) << "RemoteBitrateEstimatorAbsSendTime: Instantiating."; Can we get rid of this? https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:89: if (now_ms - last_seen_packet_ms_ > kStreamTimeOutMs) { I think I'd prefer doing if (last_seen_packet_ms_ == -1 || now_ms - last_seen_packet_ms_ > kStreamTimeOutMs) { and then remove line 79. https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe.h (right): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.h:61: rtc::CriticalSection crit_; I'd prefer to have this right before the members it protects. https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (left): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:70: TEST_F(DelayBasedBweTest, ProbeDetectionTooHighBitrate) { On 2016/08/11 11:30:06, philipel wrote: > All removed tests have an equivalent test in probe_bitrate_estimator_unittest.cc > instead, except the ProbingIgnoresSmallPackets since we don't ignore small > packets anymore. Have you verified that small packets aren't included in probes so you know this is safe? I think it is. Btw, should we have at least one test in this file which tests that probes can fail? https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc (right): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:171: PacketInfo::kNotAProbe); Why do we classify these as not being probes now? https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:278: int cluster_id = i < 5 ? 0 : PacketInfo::kNotAProbe; What does 5 mean?
https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:57: LOG(LS_INFO) << "RemoteBitrateEstimatorAbsSendTime: Instantiating."; On 2016/08/11 11:43:18, stefan-webrtc (holmer) wrote: > Can we get rid of this? I have to investigate further, but when I removed it the test still fails. But I guess it has something to do with a DelayBasedBwe being instantiated instead of the RemoteBitrateEstimatorAbsSendTime. https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:89: if (now_ms - last_seen_packet_ms_ > kStreamTimeOutMs) { On 2016/08/11 11:43:18, stefan-webrtc (holmer) wrote: > I think I'd prefer doing > if (last_seen_packet_ms_ == -1 || now_ms - last_seen_packet_ms_ > > kStreamTimeOutMs) { > > and then remove line 79. Done. https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe.h (right): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.h:61: rtc::CriticalSection crit_; On 2016/08/11 11:43:18, stefan-webrtc (holmer) wrote: > I'd prefer to have this right before the members it protects. Done. https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (left): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:70: TEST_F(DelayBasedBweTest, ProbeDetectionTooHighBitrate) { On 2016/08/11 11:43:18, stefan-webrtc (holmer) wrote: > On 2016/08/11 11:30:06, philipel wrote: > > All removed tests have an equivalent test in > probe_bitrate_estimator_unittest.cc > > instead, except the ProbingIgnoresSmallPackets since we don't ignore small > > packets anymore. > > Have you verified that small packets aren't included in probes so you know this > is safe? I think it is. > > Btw, should we have at least one test in this file which tests that probes can > fail? I haven't verified it, but I guess I could simply check the packet size like we did earlier. There is a unittest where the probing fails (ProbeDetectionFasterArrival) on line 68. https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc (right): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:171: PacketInfo::kNotAProbe); On 2016/08/11 11:43:18, stefan-webrtc (holmer) wrote: > Why do we classify these as not being probes now? It didn't make sense to let this function always insert a packet info saying this is a probe as all other functions (*TestHelper) using this function in a loop would result in huge clusters being inserted. https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:278: int cluster_id = i < 5 ? 0 : PacketInfo::kNotAProbe; On 2016/08/11 11:43:18, stefan-webrtc (holmer) wrote: > What does 5 mean? Since I changed the behavior of IncomingFeeback/4 to not always insert a probe I now let the first 5 packets be probing packets.
https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:33: constexpr uint32_t kFixedSsrc = 0; Is this only here for legacy reasons? Will it be removed once the API of surrounding code has been updated? If so, I'd prefer to call it kDummySsrc (or something similar) and add a TODO explaining that it will be removed.
https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:33: constexpr uint32_t kFixedSsrc = 0; On 2016/08/11 13:37:17, terelius wrote: > Is this only here for legacy reasons? Will it be removed once the API of > surrounding code has been updated? > > If so, I'd prefer to call it kDummySsrc (or something similar) and add a TODO > explaining that it will be removed. Done.
lgtm, but I don't know the probing code well enough to accurately assess it.
https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:89: if (now_ms - last_seen_packet_ms_ > kStreamTimeOutMs) { On 2016/08/11 13:30:51, philipel wrote: > On 2016/08/11 11:43:18, stefan-webrtc (holmer) wrote: > > I think I'd prefer doing > > if (last_seen_packet_ms_ == -1 || now_ms - last_seen_packet_ms_ > > > kStreamTimeOutMs) { > > > > and then remove line 79. > > Done. You should probably check for -1 still at line 85 in latest patchset https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc (right): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:278: int cluster_id = i < 5 ? 0 : PacketInfo::kNotAProbe; On 2016/08/11 13:30:51, philipel wrote: > On 2016/08/11 11:43:18, stefan-webrtc (holmer) wrote: > > What does 5 mean? > > Since I changed the behavior of IncomingFeeback/4 to not always insert a probe I > now let the first 5 packets be probing packets. Does it work I'd you dont pass in probe packets? If not, at least make 5 a constant. https://codereview.webrtc.org/2234363002/diff/40001/webrtc/call/bitrate_estim... File webrtc/call/bitrate_estimator_tests.cc (left): https://codereview.webrtc.org/2234363002/diff/40001/webrtc/call/bitrate_estim... webrtc/call/bitrate_estimator_tests.cc:285: receiver_log_.PushExpectedLogLine("Switching to absolute send time RBE."); I'm surprised that this doesn't require an abs send time log line too?
https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:89: if (now_ms - last_seen_packet_ms_ > kStreamTimeOutMs) { On 2016/08/11 15:38:59, stefan-webrtc (holmer) wrote: > On 2016/08/11 13:30:51, philipel wrote: > > On 2016/08/11 11:43:18, stefan-webrtc (holmer) wrote: > > > I think I'd prefer doing > > > if (last_seen_packet_ms_ == -1 || now_ms - last_seen_packet_ms_ > > > > kStreamTimeOutMs) { > > > > > > and then remove line 79. > > > > Done. > > You should probably check for -1 still at line 85 in latest patchset No need since "now_ms - (-1) = large value", and since "large_value" > kStreamTimeOutMs it will be initialized. https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc (right): https://codereview.webrtc.org/2234363002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:278: int cluster_id = i < 5 ? 0 : PacketInfo::kNotAProbe; On 2016/08/11 15:38:59, stefan-webrtc (holmer) wrote: > On 2016/08/11 13:30:51, philipel wrote: > > On 2016/08/11 11:43:18, stefan-webrtc (holmer) wrote: > > > What does 5 mean? > > > > Since I changed the behavior of IncomingFeeback/4 to not always insert a probe > I > > now let the first 5 packets be probing packets. > > Does it work I'd you dont pass in probe packets? If not, at least make 5 a > constant. This test check that the expected bitrate converges after a certain amount of time, and if we don't start off by probing then it wont have enough time to converge. Earlier the first packets were considered to be probing packets, now we have to specify that explicitly. Changed to a constant. https://codereview.webrtc.org/2234363002/diff/40001/webrtc/call/bitrate_estim... File webrtc/call/bitrate_estimator_tests.cc (left): https://codereview.webrtc.org/2234363002/diff/40001/webrtc/call/bitrate_estim... webrtc/call/bitrate_estimator_tests.cc:285: receiver_log_.PushExpectedLogLine("Switching to absolute send time RBE."); On 2016/08/11 15:38:59, stefan-webrtc (holmer) wrote: > I'm surprised that this doesn't require an abs send time log line too? Line 282 and 284 has now been replaced by the DelayBasedBwe, the AST is still instantiated after line 285, added expected log line for that.
lgtm
% the nits we talked about offline...
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2234363002/#ps80001 (title: "Nit 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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/9506)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2234363002/#ps100001 (title: "Win 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 philipel@webrtc.org
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2234363002/#ps120001 (title: "Windows WTF 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_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Removed old probe cluster logic and logic related to ssrcs from DelayBasedBwe. -Removed the old probe cluster logic and use the new ProbeBitrateEstimator instead. -Removed all logic related to ssrcs from DelayBasedBwe as they have no function on the sender side. BUG=webrtc:5859 ========== to ========== Removed old probe cluster logic and logic related to ssrcs from DelayBasedBwe. -Removed the old probe cluster logic and use the new ProbeBitrateEstimator instead. -Removed all logic related to ssrcs from DelayBasedBwe as they have no function on the sender side. BUG=webrtc:5859 R=stefan@webrtc.org, terelius@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/7522a2805118b561322a9ac7d... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) manually as 7522a2805118b561322a9ac7d37cec8e8eea06e3 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Removed old probe cluster logic and logic related to ssrcs from DelayBasedBwe. -Removed the old probe cluster logic and use the new ProbeBitrateEstimator instead. -Removed all logic related to ssrcs from DelayBasedBwe as they have no function on the sender side. BUG=webrtc:5859 R=stefan@webrtc.org, terelius@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/7522a2805118b561322a9ac7d... ========== to ========== Removed old probe cluster logic and logic related to ssrcs from DelayBasedBwe. -Removed the old probe cluster logic and use the new ProbeBitrateEstimator instead. -Removed all logic related to ssrcs from DelayBasedBwe as they have no function on the sender side. BUG=webrtc:5859 R=stefan@webrtc.org, terelius@webrtc.org Committed: https://crrev.com/7522a2805118b561322a9ac7d37cec8e8eea06e3 Cr-Commit-Position: refs/heads/master@{#13771} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
