Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2918323002:
Add functionality which limits the number of bytes on the network. (Closed)
|
Created:
3 years, 6 months ago by stefan-webrtc Modified:
3 years, 4 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhuangzesen_agora.io, zhengzhonghou_agora.io, tterriberry_mozilla.com, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd functionality which limits the number of bytes on the network.
The limit is based on the bandwidth delay product, but also adds some additional slack to compensate for the sawtooth-like BWE pattern and the slowness of the encoder rate control. The delay is estimated based on the time from sending a packet until an ack is received. Since acks are received in bursts (feedback is only sent periodically), a min filter is used to estimate the rtt.
Whenever the in flight bytes reaches the congestion window, the pacer is paused, which in turn will result in send-side queues growing. Eventually the encoders will be paused as the pacer queue grows large (currently 2 seconds).
BUG=webrtc:7926
Review-Url: https://codereview.webrtc.org/2918323002
Cr-Commit-Position: refs/heads/master@{#19289}
Committed: https://chromium.googlesource.com/external/webrtc/+/8497fdde43d920ab1f0cc90362534e5493d23abe
Patch Set 1 #Patch Set 2 : Fix tests. #Patch Set 3 : Improved cwnd calculation. #Patch Set 4 : Fix tests. #Patch Set 5 : Fix lock order inversion. #Patch Set 6 : Initialize the pause pacer members. #Patch Set 7 : Fix typo #Patch Set 8 : Always send some packets. #Patch Set 9 : Fix padding during pause. #Patch Set 10 : Rebase. #Patch Set 11 : Rebase. #Patch Set 12 : Fix flaky test. #Patch Set 13 : Fix race. #Patch Set 14 : Fix test... #Patch Set 15 : Add test. #Patch Set 16 : Add field trial. #Patch Set 17 : Remove BWE filtering. #Patch Set 18 : . #
Total comments: 25
Patch Set 19 : Rebase #Patch Set 20 : Comments addressed. #
Total comments: 5
Patch Set 21 : Comments addressed. #
Total comments: 4
Patch Set 22 : Fix test. #Patch Set 23 : Comments addressed. #Patch Set 24 : Switch to max-rtt within a feedback message and fix race. #
Total comments: 4
Patch Set 25 : Fix race in test. #Patch Set 26 : Comment addressed. #Messages
Total messages: 89 (60 generated)
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17879)
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17970)
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18044)
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18559)
Initialize the pause pacer members.
Fix typo
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18579)
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18588)
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18775)
Rebase.
Rebase.
Fix race.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18839)
Fix test...
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18843)
Description was changed from ========== Add functionality which limits the number of outstanding bytes. BUG= ========== to ========== Add functionality which limits the number of outstanding bytes. BUG=webrtc:7926 ==========
Description was changed from ========== Add functionality which limits the number of outstanding bytes. BUG=webrtc:7926 ========== to ========== Add functionality which limits the number of bytes on the network. The limit is based on the bandwidth delay product, but also adds some additional slack. The delay is estimated based on the time from sending a packet until an ack is received. Since acks are received in bursts (feedback is only sent periodically), a min filter is used to estimate the rtt. A max filter is used to compute the bandwidth based on the BWE, to avoid being affected by the sawtooth-like pattern of the BWE. BUG=webrtc:7926 ==========
Description was changed from ========== Add functionality which limits the number of bytes on the network. The limit is based on the bandwidth delay product, but also adds some additional slack. The delay is estimated based on the time from sending a packet until an ack is received. Since acks are received in bursts (feedback is only sent periodically), a min filter is used to estimate the rtt. A max filter is used to compute the bandwidth based on the BWE, to avoid being affected by the sawtooth-like pattern of the BWE. BUG=webrtc:7926 ========== to ========== Add functionality which limits the number of bytes on the network. The limit is based on the bandwidth delay product, but also adds some additional slack to compensate for the sawtooth-like BWE pattern and the slowness of the encoder rate control. The delay is estimated based on the time from sending a packet until an ack is received. Since acks are received in bursts (feedback is only sent periodically), a min filter is used to estimate the rtt. BUG=webrtc:7926 ==========
stefan@webrtc.org changed reviewers: + philipel@webrtc.org, terelius@webrtc.org, tschumim@webrtc.org
This is a first attempt at adding a cwnd to the congestion controller. A different approach that could be considered is to use a budget in the pacer instead. The budget would then be reduced for every byte sent and increased for every byte acked. Could perhaps be a cleaner solution as it doesn't require calling Pause() etc. Let me know what you think.
Any comments on this?
https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/send_side_congestion_controller.cc (right): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:111: pause_pacer_(false), I assume you duplicate the paced sender pause state to avoid grabbing its lock, please add a comment about it. https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:328: const int64_t kAcceptedQueueMs = 250; Just curios, why do we use a hard coded additional queue length instead of a factor of the BDP? https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:331: 2 * 1500); kMinCwndBytes = 2*1500 https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:40: min_feedback_rtt_(-1) {} Maybe use optional instead of magic -1? https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (left): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:494: return false; Make it into an RTC_DCHECK https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:423: size_t bytes_sent = SendPadding(1, pacing_info); Is there a way to completely stop the pacer, not just pause it and still have it send padding. https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:481: if (packets_->Empty()) { Is this correct? I assume you want to be able to send padding while paused, even if there are packets in the queue? https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:530: if (packet_counter_ == 0) I think it's somewhat clearer to keep this check where it was, moving it here just makes this function behave in an unexpected way. Maybe even add a DCHECK_GT for the |packet_counter_|. https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender_unittest.cc (right): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender_unittest.cc:659: EXPECT_CALL(callback_, TimeToSendPadding(1, _)).Times(1); Maybe create a new unittests called SendPaddingWhilePaused that only test that functionality? This test is pretty big already. https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/include/send_time_history.h (right): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:49: int64_t latest_acked_seq_num_; Use Optional. https://codereview.webrtc.org/2918323002/diff/340001/webrtc/video/end_to_end_... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/video/end_to_end_... webrtc/video/end_to_end_tests.cc:2481: EXPECT_TRUE(success) << "Failed to perform mid call probing (" << kMaxAttempts revert
Also, this congestion window will pretty much force the pacer to pace at the estimated BWE (instead of 2.5x), how do we avoid building queues in the pacer?
On 2017/08/02 15:39:28, philipel wrote: > Also, this congestion window will pretty much force the pacer to pace at the > estimated BWE (instead of 2.5x), how do we avoid building queues in the pacer? This is a good point. Perhaps we are forced to use 2.5x also for the cwnd?
Comments addressed.
https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/send_side_congestion_controller.cc (right): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:111: pause_pacer_(false), On 2017/08/02 15:35:15, philipel wrote: > I assume you duplicate the paced sender pause state to avoid grabbing its lock, > please add a comment about it. Done. https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:328: const int64_t kAcceptedQueueMs = 250; On 2017/08/02 15:35:15, philipel wrote: > Just curios, why do we use a hard coded additional queue length instead of a > factor of the BDP? That'd be an option as well. My reasoning is that it's easier to understand what this constant means. https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:331: 2 * 1500); On 2017/08/02 15:35:15, philipel wrote: > kMinCwndBytes = 2*1500 Done. https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:40: min_feedback_rtt_(-1) {} On 2017/08/02 15:35:15, philipel wrote: > Maybe use optional instead of magic -1? Done. https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (left): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:494: return false; On 2017/08/02 15:35:16, philipel wrote: > Make it into an RTC_DCHECK Done. https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:423: size_t bytes_sent = SendPadding(1, pacing_info); On 2017/08/02 15:35:16, philipel wrote: > Is there a way to completely stop the pacer, not just pause it and still have it > send padding. We could, but the problem is that if we lose some feedback packets, or if the packets in flight are lost, we may never resume. We need to send something to ensure that we sooner or later get feedback and can resume... https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:481: if (packets_->Empty()) { On 2017/08/02 15:35:16, philipel wrote: > Is this correct? > > I assume you want to be able to send padding while paused, even if there are > packets in the queue? Why would we do that if there are packets in the queue? I don't think this is a change in behavior? https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:530: if (packet_counter_ == 0) On 2017/08/02 15:35:16, philipel wrote: > I think it's somewhat clearer to keep this check where it was, moving it here > just makes this function behave in an unexpected way. > > Maybe even add a DCHECK_GT for the |packet_counter_|. Then the code will have to be duplicated since we're calling SendPadding from two places now. Is that preferable? https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender_unittest.cc (right): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender_unittest.cc:659: EXPECT_CALL(callback_, TimeToSendPadding(1, _)).Times(1); On 2017/08/02 15:35:16, philipel wrote: > Maybe create a new unittests called SendPaddingWhilePaused that only test that > functionality? This test is pretty big already. I don't know. What would then be the point of the Paused test? It can't really verify that no packets are being sent. :) https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/include/send_time_history.h (right): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:49: int64_t latest_acked_seq_num_; On 2017/08/02 15:35:16, philipel wrote: > Use Optional. Done. https://codereview.webrtc.org/2918323002/diff/340001/webrtc/video/end_to_end_... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/video/end_to_end_... webrtc/video/end_to_end_tests.cc:2481: EXPECT_TRUE(success) << "Failed to perform mid call probing (" << kMaxAttempts On 2017/08/02 15:35:16, philipel wrote: > revert I didn't get the reason why we use RTC_DCHECK over EXPECT_TRUE? It's in a test after all, and we don't want the full test executable to die just because of this test failing?
stefan@webrtc.org changed reviewers: + gnish@webrtc.org
Giorgi may be interested in reviewing this too.
Description was changed from ========== Add functionality which limits the number of bytes on the network. The limit is based on the bandwidth delay product, but also adds some additional slack to compensate for the sawtooth-like BWE pattern and the slowness of the encoder rate control. The delay is estimated based on the time from sending a packet until an ack is received. Since acks are received in bursts (feedback is only sent periodically), a min filter is used to estimate the rtt. BUG=webrtc:7926 ========== to ========== Add functionality which limits the number of bytes on the network. The limit is based on the bandwidth delay product, but also adds some additional slack to compensate for the sawtooth-like BWE pattern and the slowness of the encoder rate control. The delay is estimated based on the time from sending a packet until an ack is received. Since acks are received in bursts (feedback is only sent periodically), a min filter is used to estimate the rtt. Whenever the in flight bytes reaches the congestion window, the pacer is paused, which in turn will result in send-side queues growing. Eventually the encoders will be paused as the pacer queue grows large (currently 2 seconds). BUG=webrtc:7926 ==========
https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:481: if (packets_->Empty()) { On 2017/08/04 09:50:00, stefan-webrtc wrote: > On 2017/08/02 15:35:16, philipel wrote: > > Is this correct? > > > > I assume you want to be able to send padding while paused, even if there are > > packets in the queue? > > Why would we do that if there are packets in the queue? I don't think this is a > change in behavior? Ah, you're right. https://codereview.webrtc.org/2918323002/diff/340001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:530: if (packet_counter_ == 0) On 2017/08/04 09:50:00, stefan-webrtc wrote: > On 2017/08/02 15:35:16, philipel wrote: > > I think it's somewhat clearer to keep this check where it was, moving it here > > just makes this function behave in an unexpected way. > > > > Maybe even add a DCHECK_GT for the |packet_counter_|. > > Then the code will have to be duplicated since we're calling SendPadding from > two places now. Is that preferable? I still think that would be preferable. https://codereview.webrtc.org/2918323002/diff/340001/webrtc/video/end_to_end_... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/2918323002/diff/340001/webrtc/video/end_to_end_... webrtc/video/end_to_end_tests.cc:2481: EXPECT_TRUE(success) << "Failed to perform mid call probing (" << kMaxAttempts On 2017/08/04 09:50:00, stefan-webrtc wrote: > On 2017/08/02 15:35:16, philipel wrote: > > revert > > I didn't get the reason why we use RTC_DCHECK over EXPECT_TRUE? It's in a test > after all, and we don't want the full test executable to die just because of > this test failing? Thought this was an unintentional change, but yeah it should be EXPECT_TRUE. https://codereview.webrtc.org/2918323002/diff/380001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/send_side_congestion_controller.cc (right): https://codereview.webrtc.org/2918323002/diff/380001/webrtc/modules/congestio... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:335: void SendSideCongestionController::LimitOutstandingBytes( More of a style thing, but WDYT about making this function return a bool instead, and then pause the pacer directly in the OnSentPacket function (moving the logic from process to OnSentPacket that is)? https://codereview.webrtc.org/2918323002/diff/380001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2918323002/diff/380001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:163: if (feedback_rtt == -1 && packet_feedback.send_time_ms >= 0) { Why don't we calculate the min rtt over all packets?
Comments addressed.
https://codereview.webrtc.org/2918323002/diff/380001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/send_side_congestion_controller.cc (right): https://codereview.webrtc.org/2918323002/diff/380001/webrtc/modules/congestio... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:335: void SendSideCongestionController::LimitOutstandingBytes( On 2017/08/04 14:13:04, philipel wrote: > More of a style thing, but WDYT about making this function return a bool > instead, and then pause the pacer directly in the OnSentPacket function (moving > the logic from process to OnSentPacket that is)? I agree that'd be nicer in general. However, this method is called in two places (on feedback and on sending), so the code would have to be duplicated. In addition, we'd have to move the experiment check outside as it otherwise would have to return true, false and no-op. So I think the methods calling LimitOutstandingBytes would become harder to read at the expense of this. https://codereview.webrtc.org/2918323002/diff/380001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2918323002/diff/380001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:163: if (feedback_rtt == -1 && packet_feedback.send_time_ms >= 0) { On 2017/08/04 14:13:04, philipel wrote: > Why don't we calculate the min rtt over all packets? Do you mean we should remove feedback_rtt == -1 from the if statement? I think the reason for doing it like this is that the packets are ordered in the feedback message. I could probably remove it and add a min below. Perhaps better for readability.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
lgtm % comments. https://codereview.webrtc.org/2918323002/diff/380001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2918323002/diff/380001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:163: if (feedback_rtt == -1 && packet_feedback.send_time_ms >= 0) { On 2017/08/07 07:37:11, stefan-webrtc wrote: > On 2017/08/04 14:13:04, philipel wrote: > > Why don't we calculate the min rtt over all packets? > > Do you mean we should remove feedback_rtt == -1 from the if statement? I think > the reason for doing it like this is that the packets are ordered in the > feedback message. I could probably remove it and add a min below. Perhaps better > for readability. Yes, but also calculating the RTT on the first packet in the feedback vector will result in a higher RTT since it has been waiting on the receiving side the longest. https://codereview.webrtc.org/2918323002/diff/400001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2918323002/diff/400001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:165: clock_->TimeInMilliseconds() - packet_feedback.send_time_ms; Replace with a now_ms defined outside the loop. https://codereview.webrtc.org/2918323002/diff/400001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/2918323002/diff/400001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:498: critsect_.Leave(); Maybe add a DCHECK_LT(0, packet_counter_)
Fix test.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Comments addressed.
https://codereview.webrtc.org/2918323002/diff/400001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2918323002/diff/400001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:165: clock_->TimeInMilliseconds() - packet_feedback.send_time_ms; On 2017/08/07 11:39:48, philipel wrote: > Replace with a now_ms defined outside the loop. Done. https://codereview.webrtc.org/2918323002/diff/400001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/2918323002/diff/400001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:498: critsect_.Leave(); On 2017/08/07 11:39:48, philipel wrote: > Maybe add a DCHECK_LT(0, packet_counter_) Done.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/24698)
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/24736)
Philip, after talking to Goergi I decided to update the min rtt estimation to take the max rtt within a feedback message, but the min among all the feedback messages. This makes sense since it's similar to the ack compression problem.
On 2017/08/09 11:43:44, stefan-webrtc wrote: > Philip, after talking to Goergi I decided to update the min rtt estimation to > take the max rtt within a feedback message, but the min among all the feedback > messages. This makes sense since it's similar to the ack compression problem. And now all tests are passing. If you don't have any more comments I'll land this.
https://codereview.webrtc.org/2918323002/diff/460001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2918323002/diff/460001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:166: if (feedback_rtt == -1) { Nit: remove if/else statement. https://codereview.webrtc.org/2918323002/diff/460001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:169: feedback_rtt = std::max(rtt, feedback_rtt); Nit: add a comment that you use max to account for feedback being delayed by the receiver.
https://codereview.webrtc.org/2918323002/diff/460001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2918323002/diff/460001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:166: if (feedback_rtt == -1) { On 2017/08/09 11:58:43, philipel wrote: > Nit: remove if/else statement. Done. https://codereview.webrtc.org/2918323002/diff/460001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:169: feedback_rtt = std::max(rtt, feedback_rtt); On 2017/08/09 11:58:43, philipel wrote: > Nit: add a comment that you use max to account for feedback being delayed by the > receiver. Done.
lgtm
The CQ bit was checked by stefan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org Link to the patchset: https://codereview.webrtc.org/2918323002/#ps500001 (title: "Comment addressed.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 500001, "attempt_start_ts": 1502283188130530,
"parent_rev": "cac3ccc2dec1e018a6d9db14425c6a4f6e0750ab", "commit_rev":
"8497fdde43d920ab1f0cc90362534e5493d23abe"}
Message was sent while issue was closed.
Description was changed from ========== Add functionality which limits the number of bytes on the network. The limit is based on the bandwidth delay product, but also adds some additional slack to compensate for the sawtooth-like BWE pattern and the slowness of the encoder rate control. The delay is estimated based on the time from sending a packet until an ack is received. Since acks are received in bursts (feedback is only sent periodically), a min filter is used to estimate the rtt. Whenever the in flight bytes reaches the congestion window, the pacer is paused, which in turn will result in send-side queues growing. Eventually the encoders will be paused as the pacer queue grows large (currently 2 seconds). BUG=webrtc:7926 ========== to ========== Add functionality which limits the number of bytes on the network. The limit is based on the bandwidth delay product, but also adds some additional slack to compensate for the sawtooth-like BWE pattern and the slowness of the encoder rate control. The delay is estimated based on the time from sending a packet until an ack is received. Since acks are received in bursts (feedback is only sent periodically), a min filter is used to estimate the rtt. Whenever the in flight bytes reaches the congestion window, the pacer is paused, which in turn will result in send-side queues growing. Eventually the encoders will be paused as the pacer queue grows large (currently 2 seconds). BUG=webrtc:7926 Review-Url: https://codereview.webrtc.org/2918323002 Cr-Commit-Position: refs/heads/master@{#19289} Committed: https://chromium.googlesource.com/external/webrtc/+/8497fdde43d920ab1f0cc9036... ==========
Message was sent while issue was closed.
Committed patchset #26 (id:500001) as https://chromium.googlesource.com/external/webrtc/+/8497fdde43d920ab1f0cc9036...
Message was sent while issue was closed.
A revert of this CL (patchset #26 id:500001) has been created in https://codereview.webrtc.org/3001653002/ by stefan@webrtc.org. The reason for reverting is: Speculative revert to see if this caused regressions in android perf tests.. |
