Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2904183002:
Structure of BBR's implementation,some main classes and functions which are going to be used (Closed)
DescriptionThis is an initial cl, which contains small amount of implemented functions, and large amount of unimplemented ones.
Code should implement BBR which is the congestion controlling algorithm. BBR tries to estimate two values bottle-neck bandwidth(bw) and round trip time(rtt),then use these two values to set two control parameters pacing rate(pacing_rate),the rate at which data should be sent and congestion window size (cwnd), cwnd is the upper bound for data in flight,data_in_flight <= cwnd at all time.
BBR has four modes:
1)Startup-ramping up throughput discovering estimated bw.
2)Drain-after Startup decrease throughput to drain queues.
3)Probe Bandwidth-most of the time BBR should be in this mode,
sending data at the rate of estimated bw, while sometimes trying to discover new bandwidth.
4)Probe Rtt-in this mode BBR tries to discover new rtt for the connection.
The key moment in BBR is when we receive feedback from the receiver,as this is the only moment which should effect our two estimators. At this moment all the switches between modes should happen, except switch to ProbeRtt mode (switching to ProbeRtt mode should happen when current min_rtt value expires).
This cl serves to emphasize the structure of Bbr, when switches happen and what key classes/functions should be implemented for proper functionality.
BUG=webrtc:7713
NOTRY=True
Review-Url: https://codereview.webrtc.org/2904183002
Cr-Commit-Position: refs/heads/master@{#18444}
Committed: https://chromium.googlesource.com/external/webrtc/+/6dcdf10c76d68459758a6503d9874d76ed1473e5
Patch Set 1 #
Total comments: 24
Patch Set 2 : nothing #Patch Set 3 : nothing #
Total comments: 1
Patch Set 4 : Moved include on top #
Total comments: 60
Patch Set 5 : Updated code according to reviews. #Patch Set 6 : This is a very basic representation of what BBR implementation should look like and what classes it… #
Total comments: 10
Patch Set 7 : some minor stylistic fixes. #
Total comments: 10
Patch Set 8 : minor changes #Patch Set 9 : Added header files to build file #Messages
Total messages: 46 (25 generated)
gnish@chromium.org changed reviewers: + gnish@chromium.org, holmer@google.com, philipel@google.com, terelius@google.com
gnish@chromium.org changed reviewers: + philipel@webrtc.org - philipel@google.com
Some initial comments. Please update the description of the bug, both the subject and what you aim to implement in this CL. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right): https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:159: const std::vector<std::pair<uint64_t, int64_t> >& packet_feedback_vector) Change from std::vector<std::pair<uint64_t, int64_t> >& to std::vector<std::pair<uint64_t, int64_t>>& https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:18: Please fix includes according to the style guide: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:29: const int kFeedbackIntervals = 100; Move into anonymous namespace. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:29: const int kFeedbackIntervals = 100; Add empty line between 29-30 https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:59: const std::vector<std::pair<uint64_t, int64_t> >& feedback_vector = remove whitespace https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:19: #include <utility> Please add an empty line between 19 and 20. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:27: namespace bwe { newline 27-28 https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:45: enum RecoveryState { If this is not used yet, remove it. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:61: const std::vector<std::pair<uint64_t, int64_t> >& feedback_vector, "> >" to ">>" https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:86: }; newline :) https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/stats.h (right): https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/stats.h:18: class Data { Please find a better name for this class. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py (right): https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py:152: figures = [receiver] Undo this change
gnish@chromium.org changed reviewers: + gnish@webrtc.org, stefan@webrtc.org, terelius@webrtc.org - holmer@google.com, terelius@google.com
https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right): https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:159: const std::vector<std::pair<uint64_t, int64_t> >& packet_feedback_vector) On 2017/05/29 09:38:49, philipel wrote: > Change from > std::vector<std::pair<uint64_t, int64_t> >& to > std::vector<std::pair<uint64_t, int64_t>>& Done. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:18: On 2017/05/29 09:38:50, philipel wrote: > Please fix includes according to the style guide: > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:29: const int kFeedbackIntervals = 100; On 2017/05/29 09:38:49, philipel wrote: > Move into anonymous namespace. Done. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:29: const int kFeedbackIntervals = 100; On 2017/05/29 09:38:49, philipel wrote: > Add empty line between 29-30 Done. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:59: const std::vector<std::pair<uint64_t, int64_t> >& feedback_vector = On 2017/05/29 09:38:49, philipel wrote: > remove whitespace Done. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:19: #include <utility> On 2017/05/29 09:38:50, philipel wrote: > Please add an empty line between 19 and 20. Done. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:27: namespace bwe { On 2017/05/29 09:38:50, philipel wrote: > newline 27-28 Done. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:45: enum RecoveryState { On 2017/05/29 09:38:50, philipel wrote: > If this is not used yet, remove it. Done. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:61: const std::vector<std::pair<uint64_t, int64_t> >& feedback_vector, On 2017/05/29 09:38:50, philipel wrote: > "> >" to ">>" Done. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:86: }; On 2017/05/29 09:38:50, philipel wrote: > newline :) Done. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/stats.h (right): https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/stats.h:18: class Data { On 2017/05/29 09:38:50, philipel wrote: > Please find a better name for this class. Done. https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py (right): https://codereview.webrtc.org/2904183002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py:152: figures = [receiver] On 2017/05/29 09:38:50, philipel wrote: > Undo this change Done.
Please update the description https://codereview.webrtc.org/2904183002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2904183002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:17: #include "webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h" This should be on top
Description was changed from ========== initial commit initial commit initial commit aa BUG=webrtc:7713 ========== to ========== This is an initial cl,which contains small amount of implemented functions,and large amount of unimplemented ones.The final result should look like initial cl,but with implemented classes/functions. BUG=webrtc:7713 ==========
Description was changed from ========== This is an initial cl,which contains small amount of implemented functions,and large amount of unimplemented ones.The final result should look like initial cl,but with implemented classes/functions. BUG=webrtc:7713 ========== to ========== This is an initial cl,which contains small amount of implemented functions,and large amount of unimplemented ones. The key moment in Bbr is when we receive feedback from the sender.At this moment all the switches between modes should happen,except switch to ProbeRtt mode(Switching to ProbeRtt mode should happen when current min_rtt value expires). This cl serves to emphasize the structure of Bbr,when switches happen and what key classes/functions should be implemented for proper functionality. BUG=webrtc:7713 ==========
Description was changed from ========== This is an initial cl,which contains small amount of implemented functions,and large amount of unimplemented ones. The key moment in Bbr is when we receive feedback from the sender.At this moment all the switches between modes should happen,except switch to ProbeRtt mode(Switching to ProbeRtt mode should happen when current min_rtt value expires). This cl serves to emphasize the structure of Bbr,when switches happen and what key classes/functions should be implemented for proper functionality. BUG=webrtc:7713 ========== to ========== This is an initial cl, which contains small amount of implemented functions, and large amount of unimplemented ones. The key moment in Bbr is when we receive feedback from the sender. At this moment all the switches between modes should happen,except switch to ProbeRtt mode(Switching to ProbeRtt mode should happen when current min_rtt value expires). This cl serves to emphasize the structure of Bbr,when switches happen and what key classes/functions should be implemented for proper functionality. BUG=webrtc:7713 ==========
Description was changed from ========== This is an initial cl, which contains small amount of implemented functions, and large amount of unimplemented ones. The key moment in Bbr is when we receive feedback from the sender. At this moment all the switches between modes should happen,except switch to ProbeRtt mode(Switching to ProbeRtt mode should happen when current min_rtt value expires). This cl serves to emphasize the structure of Bbr,when switches happen and what key classes/functions should be implemented for proper functionality. BUG=webrtc:7713 ========== to ========== This is an initial cl, which contains small amount of implemented functions, and large amount of unimplemented ones. The key moment in BBR is when we receive feedback from the sender. At this moment all the switches between modes should happen, except switch to ProbeRtt mode (switching to ProbeRtt mode should happen when current min_rtt value expires). This cl serves to emphasize the structure of Bbr, when switches happen and what key classes/functions should be implemented for proper functionality. BUG=webrtc:7713 ==========
Starting to look good :) I still think you need to update the description (and subject) of this CL since it is not clear that you have a plan on landing more followup CLs. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:51: int BbrBweSender::GetPacketSize(uint64_t sequence_number) { Is this function necessary, wont it be trivial when implemented? Why is it public? https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:62: // Calculate number of bytes acknowledged after the feedback. For trivial pieces of code comments are not useful, remove this comment. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:63: for (const auto& feedback : feedback_vector) { remove {} https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:67: // check if current min_rtt estimate expired or not. Since you choose descriptive names (good! :)) this comment is not necessary either. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:69: // If it did enter ProbeRtt mode. Remove comment https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:70: if (min_rtt_expired) { remove {} https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:147: packet_feedbacks_.push_back( Instead of packet_feedbacks_.push_back( {media_packet.sequence_number(), arrival_time_ms}); use packet_feedbacks_.emplace_back( media_packet.sequence_number(), arrival_time_ms); https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:152: FeedbackPacket* BbrBweReceiver::GetFeedback(int64_t now_ms) { I think this function should be as bare bone as possible since it is not relevant for this CL. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:154: return NULL; NULL --> nullptr https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:48: void EnterStartup(); All functions that you expect to only be used by this class should be private. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc (right): https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc:72: // fprintf(stderr, "%ld\n", rtt_ms); Undo this change https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/packet.h (right): https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/packet.h:127: const std::vector<std::pair<uint64_t, int64_t> > packet_feedback_vector_; > >
https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:26: const float kHighGain = 2.885f; Could you point to a reference which recommends this value, or describe how we came to conclude it was a good value? https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:28: const int kFeedbackIntervals = 100; Unit? https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:56: bool min_rtt_expired = false; Move declaration down to where it's used the first time. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:58: int64_t bytes_acked = 0; Same with these two https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:60: const std::vector<std::pair<uint64_t, int64_t>>& feedback_vector = Here I'd probably use const auto& https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:66: // Update bandwidth and min_rtt estimates,also space after , https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:69: // If it did enter ProbeRtt mode. I don't understand this comment https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:106: stats_->set_pacing_gain(kHighGain); What is the purpose of stats_? Seems odd that it's responsible for pacing? https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:111: // switch to Drain mode. Merge these lines. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:158: PacketIdentifierNode* latest = *(received_packets_.begin()); Remove parentheses. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:24: #include "webrtc/modules/remote_bitrate_estimator/test/estimators/stats.h" Do we have to include this here, or can we forward declare instead? https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:47: void OnPacketsSent(const Packets& packets) override; Please group overrides and non-overrides separately. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:62: int GetPacketSize(const uint64_t sequence_number); No need for const for pass-by-value parameters. Should this return size_t? https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:63: int GetFeedbackIntervalMs() const override; Return int64_t which is typically used for time. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:72: int64_t last_feedback_ms_ = 0; Please initialize in initializer list. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:73: std::map<uint16_t, int64_t> send_time_; Map from what to what? Add a comment above. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:77: int64_t rt_count_; rtt_count_? https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:89: int64_t last_feedback_ms_; private?
Description was changed from ========== This is an initial cl, which contains small amount of implemented functions, and large amount of unimplemented ones. The key moment in BBR is when we receive feedback from the sender. At this moment all the switches between modes should happen, except switch to ProbeRtt mode (switching to ProbeRtt mode should happen when current min_rtt value expires). This cl serves to emphasize the structure of Bbr, when switches happen and what key classes/functions should be implemented for proper functionality. BUG=webrtc:7713 ========== to ========== This is an initial cl, which contains small amount of implemented functions, and large amount of unimplemented ones. Code should implement BBR which is the congestion controlling algorithm. BBR tries to estimate two values bottle-neck bandwidth(bw) and round trip time(rtt),then use these two values to set two control parameters pacing rate(pacing_rate),the rate at which data should be sent and congestion window size (cwnd), cwnd is the upper bound for data in flight,data_in_flight <= cwnd at all time. BBR has four modes: 1)Startup-ramping up throughput discovering estimated bw. 2)Drain-after Startup decrease throughput to drain queues. 3)Probe Bandwidth-most of the time BBR should be in this mode, sending data at the rate of estimated bw, while sometimes trying to discover new bandwidth. 4)Probe Rtt-in this mode BBR tries to discover new rtt for the connection. The key moment in BBR is when we receive feedback from the receiver,as this is the only moment which should effect our two estimators. At this moment all the switches between modes should happen, except switch to ProbeRtt mode (switching to ProbeRtt mode should happen when current min_rtt value expires). This cl serves to emphasize the structure of Bbr, when switches happen and what key classes/functions should be implemented for proper functionality. BUG=webrtc:7713 ==========
https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:26: const float kHighGain = 2.885f; On 2017/05/30 09:45:13, stefan-webrtc wrote: > Could you point to a reference which recommends this value, or describe how we > came to conclude it was a good value? Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:28: const int kFeedbackIntervals = 100; On 2017/05/30 09:45:13, stefan-webrtc wrote: > Unit? Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:51: int BbrBweSender::GetPacketSize(uint64_t sequence_number) { On 2017/05/30 09:43:38, philipel wrote: > Is this function necessary, wont it be trivial when implemented? Why is it > public? Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:56: bool min_rtt_expired = false; On 2017/05/30 09:45:13, stefan-webrtc wrote: > Move declaration down to where it's used the first time. Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:58: int64_t bytes_acked = 0; On 2017/05/30 09:45:13, stefan-webrtc wrote: > Same with these two Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:60: const std::vector<std::pair<uint64_t, int64_t>>& feedback_vector = On 2017/05/30 09:45:12, stefan-webrtc wrote: > Here I'd probably use const auto& Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:62: // Calculate number of bytes acknowledged after the feedback. On 2017/05/30 09:43:38, philipel wrote: > For trivial pieces of code comments are not useful, remove this comment. Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:63: for (const auto& feedback : feedback_vector) { On 2017/05/30 09:43:38, philipel wrote: > remove {} Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:66: // Update bandwidth and min_rtt estimates,also On 2017/05/30 09:45:12, stefan-webrtc wrote: > space after , Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:67: // check if current min_rtt estimate expired or not. On 2017/05/30 09:43:38, philipel wrote: > Since you choose descriptive names (good! :)) this comment is not necessary > either. Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:69: // If it did enter ProbeRtt mode. On 2017/05/30 09:45:13, stefan-webrtc wrote: > I don't understand this comment Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:69: // If it did enter ProbeRtt mode. On 2017/05/30 09:43:38, philipel wrote: > Remove comment Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:70: if (min_rtt_expired) { On 2017/05/30 09:43:38, philipel wrote: > remove {} Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:106: stats_->set_pacing_gain(kHighGain); On 2017/05/30 09:45:12, stefan-webrtc wrote: > What is the purpose of stats_? Seems odd that it's responsible for pacing? stats_ is responsible to basically store all the data about current connection state,estimated bandwidth,estimated rtt,the pacing gain which the connection should use,congestion window which connection should use and etc.It just sets pacing gain to some value,I believe it will not be responsible for the pacing. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:111: // switch to Drain mode. On 2017/05/30 09:45:13, stefan-webrtc wrote: > Merge these lines. Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:147: packet_feedbacks_.push_back( On 2017/05/30 09:43:38, philipel wrote: > Instead of > packet_feedbacks_.push_back( > {media_packet.sequence_number(), arrival_time_ms}); > use > packet_feedbacks_.emplace_back( > media_packet.sequence_number(), arrival_time_ms); Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:152: FeedbackPacket* BbrBweReceiver::GetFeedback(int64_t now_ms) { On 2017/05/30 09:43:38, philipel wrote: > I think this function should be as bare bone as possible since it is not > relevant for this CL. Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:154: return NULL; On 2017/05/30 09:43:38, philipel wrote: > NULL --> nullptr Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:158: PacketIdentifierNode* latest = *(received_packets_.begin()); On 2017/05/30 09:45:13, stefan-webrtc wrote: > Remove parentheses. Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:24: #include "webrtc/modules/remote_bitrate_estimator/test/estimators/stats.h" On 2017/05/30 09:45:13, stefan-webrtc wrote: > Do we have to include this here, or can we forward declare instead? Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:47: void OnPacketsSent(const Packets& packets) override; On 2017/05/30 09:45:14, stefan-webrtc wrote: > Please group overrides and non-overrides separately. Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:48: void EnterStartup(); On 2017/05/30 09:43:38, philipel wrote: > All functions that you expect to only be used by this class should be private. Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:62: int GetPacketSize(const uint64_t sequence_number); On 2017/05/30 09:45:14, stefan-webrtc wrote: > No need for const for pass-by-value parameters. > > Should this return size_t? Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:63: int GetFeedbackIntervalMs() const override; On 2017/05/30 09:45:14, stefan-webrtc wrote: > Return int64_t which is typically used for time. This is overrode function,which's return type in simulation tool (in bwe.h) is declared as int.So I cannot change it to int64_t unless I change it in bwe.h. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:72: int64_t last_feedback_ms_ = 0; On 2017/05/30 09:45:13, stefan-webrtc wrote: > Please initialize in initializer list. Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:73: std::map<uint16_t, int64_t> send_time_; On 2017/05/30 09:45:14, stefan-webrtc wrote: > Map from what to what? Add a comment above. Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:77: int64_t rt_count_; On 2017/05/30 09:45:13, stefan-webrtc wrote: > rtt_count_? Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:89: int64_t last_feedback_ms_; On 2017/05/30 09:45:14, stefan-webrtc wrote: > private? Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc (right): https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc:72: // fprintf(stderr, "%ld\n", rtt_ms); On 2017/05/30 09:43:38, philipel wrote: > Undo this change Done. https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/packet.h (right): https://codereview.webrtc.org/2904183002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/packet.h:127: const std::vector<std::pair<uint64_t, int64_t> > packet_feedback_vector_; On 2017/05/30 09:43:38, philipel wrote: > > > Done.
Just some minor comment https://codereview.webrtc.org/2904183002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2904183002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:38: // Queue draining phase,which where created during startup. phase, which https://codereview.webrtc.org/2904183002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:40: // Cruising,probing new bandwidth. whitespace https://codereview.webrtc.org/2904183002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h (right): https://codereview.webrtc.org/2904183002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:34: // Packet sent from sender,meaning it is inflight whitespace https://codereview.webrtc.org/2904183002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:34: // Packet sent from sender,meaning it is inflight I don't quite understand this comment. https://codereview.webrtc.org/2904183002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:37: // Ack was received by sender,meaning whitespace
https://codereview.webrtc.org/2904183002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2904183002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:38: // Queue draining phase,which where created during startup. On 2017/06/01 15:06:53, philipel wrote: > phase, which Done. https://codereview.webrtc.org/2904183002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:40: // Cruising,probing new bandwidth. On 2017/06/01 15:06:53, philipel wrote: > whitespace Done. https://codereview.webrtc.org/2904183002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h (right): https://codereview.webrtc.org/2904183002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:34: // Packet sent from sender,meaning it is inflight On 2017/06/01 15:06:53, philipel wrote: > whitespace Done. https://codereview.webrtc.org/2904183002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:34: // Packet sent from sender,meaning it is inflight On 2017/06/01 15:06:53, philipel wrote: > whitespace Done. https://codereview.webrtc.org/2904183002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:37: // Ack was received by sender,meaning On 2017/06/01 15:06:53, philipel wrote: > whitespace Done.
lgtm
The CQ bit was checked by gnish@chromium.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/17744)
lgtm with nits fixed. May also want to wait for terelius review. https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h (right): https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:32: int64_t data_inflight(); would size_t make more sense here? https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:34: // Packet sent from sender, meaning it is inflight Empty line before each comment in the class declaration. https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:34: // Save bandwidth sample for the current round. Empty line before each comment in the class declaration. https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:40: bool DeliveryrateGrows(); DeliveryRateGrows https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:34: // Checks whether or last discovered min_rtt value Empty line before each comment in the class declaration.
One more thing, update the subject of this CL, "Initial CL" is not very descriptive.
https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h (right): https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:32: int64_t data_inflight(); On 2017/06/02 12:08:34, stefan-webrtc wrote: > would size_t make more sense here? Done. https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:34: // Packet sent from sender, meaning it is inflight On 2017/06/02 12:08:34, stefan-webrtc wrote: > Empty line before each comment in the class declaration. Done. https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:34: // Save bandwidth sample for the current round. On 2017/06/02 12:08:34, stefan-webrtc wrote: > Empty line before each comment in the class declaration. Done. https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:40: bool DeliveryrateGrows(); On 2017/06/02 12:08:34, stefan-webrtc wrote: > DeliveryRateGrows Done. https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2904183002/diff/120001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:34: // Checks whether or last discovered min_rtt value On 2017/06/02 12:08:34, stefan-webrtc wrote: > Empty line before each comment in the class declaration. Done.
gnish@webrtc.org changed reviewers: - gnish@chromium.org
The CQ bit was checked by gnish@webrtc.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.webrtc.org/2904183002/#ps140001 (title: "minor changes")
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 gnish@webrtc.org
The CQ bit was checked by gnish@webrtc.org
The CQ bit was unchecked by gnish@webrtc.org
The CQ bit was checked by gnish@webrtc.org
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/17802)
Description was changed from ========== This is an initial cl, which contains small amount of implemented functions, and large amount of unimplemented ones. Code should implement BBR which is the congestion controlling algorithm. BBR tries to estimate two values bottle-neck bandwidth(bw) and round trip time(rtt),then use these two values to set two control parameters pacing rate(pacing_rate),the rate at which data should be sent and congestion window size (cwnd), cwnd is the upper bound for data in flight,data_in_flight <= cwnd at all time. BBR has four modes: 1)Startup-ramping up throughput discovering estimated bw. 2)Drain-after Startup decrease throughput to drain queues. 3)Probe Bandwidth-most of the time BBR should be in this mode, sending data at the rate of estimated bw, while sometimes trying to discover new bandwidth. 4)Probe Rtt-in this mode BBR tries to discover new rtt for the connection. The key moment in BBR is when we receive feedback from the receiver,as this is the only moment which should effect our two estimators. At this moment all the switches between modes should happen, except switch to ProbeRtt mode (switching to ProbeRtt mode should happen when current min_rtt value expires). This cl serves to emphasize the structure of Bbr, when switches happen and what key classes/functions should be implemented for proper functionality. BUG=webrtc:7713 ========== to ========== This is an initial cl, which contains small amount of implemented functions, and large amount of unimplemented ones. Code should implement BBR which is the congestion controlling algorithm. BBR tries to estimate two values bottle-neck bandwidth(bw) and round trip time(rtt),then use these two values to set two control parameters pacing rate(pacing_rate),the rate at which data should be sent and congestion window size (cwnd), cwnd is the upper bound for data in flight,data_in_flight <= cwnd at all time. BBR has four modes: 1)Startup-ramping up throughput discovering estimated bw. 2)Drain-after Startup decrease throughput to drain queues. 3)Probe Bandwidth-most of the time BBR should be in this mode, sending data at the rate of estimated bw, while sometimes trying to discover new bandwidth. 4)Probe Rtt-in this mode BBR tries to discover new rtt for the connection. The key moment in BBR is when we receive feedback from the receiver,as this is the only moment which should effect our two estimators. At this moment all the switches between modes should happen, except switch to ProbeRtt mode (switching to ProbeRtt mode should happen when current min_rtt value expires). This cl serves to emphasize the structure of Bbr, when switches happen and what key classes/functions should be implemented for proper functionality. BUG=webrtc:7713 NOTRY=True ==========
The CQ bit was checked by gnish@webrtc.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.webrtc.org/2904183002/#ps160001 (title: "Added header files to build file")
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": 160001, "attempt_start_ts": 1496667549558790,
"parent_rev": "59ee91b68a44104b9aec7b34ac67a215bbf84f85", "commit_rev":
"6dcdf10c76d68459758a6503d9874d76ed1473e5"}
Message was sent while issue was closed.
Description was changed from ========== This is an initial cl, which contains small amount of implemented functions, and large amount of unimplemented ones. Code should implement BBR which is the congestion controlling algorithm. BBR tries to estimate two values bottle-neck bandwidth(bw) and round trip time(rtt),then use these two values to set two control parameters pacing rate(pacing_rate),the rate at which data should be sent and congestion window size (cwnd), cwnd is the upper bound for data in flight,data_in_flight <= cwnd at all time. BBR has four modes: 1)Startup-ramping up throughput discovering estimated bw. 2)Drain-after Startup decrease throughput to drain queues. 3)Probe Bandwidth-most of the time BBR should be in this mode, sending data at the rate of estimated bw, while sometimes trying to discover new bandwidth. 4)Probe Rtt-in this mode BBR tries to discover new rtt for the connection. The key moment in BBR is when we receive feedback from the receiver,as this is the only moment which should effect our two estimators. At this moment all the switches between modes should happen, except switch to ProbeRtt mode (switching to ProbeRtt mode should happen when current min_rtt value expires). This cl serves to emphasize the structure of Bbr, when switches happen and what key classes/functions should be implemented for proper functionality. BUG=webrtc:7713 NOTRY=True ========== to ========== This is an initial cl, which contains small amount of implemented functions, and large amount of unimplemented ones. Code should implement BBR which is the congestion controlling algorithm. BBR tries to estimate two values bottle-neck bandwidth(bw) and round trip time(rtt),then use these two values to set two control parameters pacing rate(pacing_rate),the rate at which data should be sent and congestion window size (cwnd), cwnd is the upper bound for data in flight,data_in_flight <= cwnd at all time. BBR has four modes: 1)Startup-ramping up throughput discovering estimated bw. 2)Drain-after Startup decrease throughput to drain queues. 3)Probe Bandwidth-most of the time BBR should be in this mode, sending data at the rate of estimated bw, while sometimes trying to discover new bandwidth. 4)Probe Rtt-in this mode BBR tries to discover new rtt for the connection. The key moment in BBR is when we receive feedback from the receiver,as this is the only moment which should effect our two estimators. At this moment all the switches between modes should happen, except switch to ProbeRtt mode (switching to ProbeRtt mode should happen when current min_rtt value expires). This cl serves to emphasize the structure of Bbr, when switches happen and what key classes/functions should be implemented for proper functionality. BUG=webrtc:7713 NOTRY=True Review-Url: https://codereview.webrtc.org/2904183002 Cr-Commit-Position: refs/heads/master@{#18444} Committed: https://chromium.googlesource.com/external/webrtc/+/6dcdf10c76d68459758a6503d... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/6dcdf10c76d68459758a6503d... |
