Skip to content

Commit

Permalink
Revert "Revert "Revert "Reland "Moved congestion controller to task q…
Browse files Browse the repository at this point in the history 
…ueue.""""

This reverts commit 65792c5.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> Revert "Revert "Reland "Moved congestion controller to task queue."""
> 
> This reverts commit 4e849f6.
> 
> Reason for revert: <INSERT REASONING HERE>
> 
> Original change's description:
> > Revert "Reland "Moved congestion controller to task queue.""
> > 
> > This reverts commit 57daeb7.
> > 
> > Reason for revert: Cause increased congestion and deadlocks in downstream project
> > 
> > Original change's description:
> > > Reland "Moved congestion controller to task queue."
> > > 
> > > This is a reland of 0cbcba7.
> > > 
> > > Original change's description:
> > > > Moved congestion controller to task queue.
> > > > 
> > > > The goal of this work is to make it easier to experiment with the
> > > > bandwidth estimation implementation. For this reason network control
> > > > functionality is moved from SendSideCongestionController(SSCC),
> > > > PacedSender and BitrateController to the newly created
> > > > GoogCcNetworkController which implements the newly created
> > > > NetworkControllerInterface. This allows the implementation to be
> > > > replaced at runtime in the future.
> > > > 
> > > > This is the first part of a split of a larger CL, see:
> > > > https://webrtc-review.googlesource.com/c/src/+/39788/8
> > > > For further explanations.
> > > > 
> > > > Bug: webrtc:8415
> > > > Change-Id: I770189c04cc31b313bd4e57821acff55fbcb1ad3
> > > > Reviewed-on: https://webrtc-review.googlesource.com/43840
> > > > Commit-Queue: Sebastian Jansson <[email protected]>
> > > > Reviewed-by: Björn Terelius <[email protected]>
> > > > Reviewed-by: Stefan Holmer <[email protected]>
> > > > Cr-Commit-Position: refs/heads/master@{#21868}
> > > 
> > > Bug: webrtc:8415
> > > Change-Id: I1d1756a30deed5b421b1c91c1918a13b6bb455da
> > > Reviewed-on: https://webrtc-review.googlesource.com/48000
> > > Reviewed-by: Stefan Holmer <[email protected]>
> > > Commit-Queue: Sebastian Jansson <[email protected]>
> > > Cr-Commit-Position: refs/heads/master@{#21899}
> > 
> > [email protected],[email protected],[email protected]
> > 
> > # Not skipping CQ checks because original CL landed > 1 day ago.
> > 
> > Bug: webrtc:8415
> > Change-Id: Ida8074dcac2cc28b3629228eb22846d8a8e81b83
> > Reviewed-on: https://webrtc-review.googlesource.com/52980
> > Reviewed-by: Danil Chapovalov <[email protected]>
> > Commit-Queue: Danil Chapovalov <[email protected]>
> > Cr-Commit-Position: refs/heads/master@{#22017}
> 
> [email protected],[email protected],[email protected],[email protected]
> 
> Change-Id: I3393b74370c4f4d0955f50728005b2b925be169b
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: webrtc:8415
> Reviewed-on: https://webrtc-review.googlesource.com/53262
> Reviewed-by: Sebastian Jansson <[email protected]>
> Commit-Queue: Sebastian Jansson <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#22023}

[email protected],[email protected],[email protected],[email protected]

Change-Id: Id68ad986ee51142b7be3381d0793709b4392fe2c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:8415
Reviewed-on: https://webrtc-review.googlesource.com/53360
Reviewed-by: Sebastian Jansson <[email protected]>
Commit-Queue: Sebastian Jansson <[email protected]>
Cr-Commit-Position: refs/heads/master@{#22024}
  • Loading branch information
@jonex
jonex authored and Commit Bot committed Feb 14, 2018
1 parent 65792c5 commit ea86bb7
Show file tree
Hide file tree
Showing 57 changed files with 844 additions and 3,006 deletions.
3 changes: 1 addition & 2 deletions call/rtp_transport_controller_send.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,7 @@ const RtpKeepAliveConfig& RtpTransportControllerSend::keepalive_config() const {
void RtpTransportControllerSend::SetAllocatedSendBitrateLimits(
int min_send_bitrate_bps,
int max_padding_bitrate_bps) {
send_side_cc_.SetSendBitrateLimits(min_send_bitrate_bps,
max_padding_bitrate_bps);
pacer_.SetSendBitrateLimits(min_send_bitrate_bps, max_padding_bitrate_bps);
}

void RtpTransportControllerSend::SetKeepAliveConfig(
Expand Down
4 changes: 2 additions & 2 deletions modules/bitrate_controller/bitrate_controller_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,11 +340,11 @@ TEST_F(BitrateControllerTest, OneBitrateObserverMultipleReportBlocks) {
report_blocks.clear();

// All packets lost on stream with few packets, no back-off.
report_blocks.push_back(CreateReportBlock(1, 2, 0, sequence_number[0]));
report_blocks.push_back(CreateReportBlock(1, 2, 1, sequence_number[0]));
report_blocks.push_back(CreateReportBlock(1, 3, 255, sequence_number[1]));
bandwidth_observer_->OnReceivedRtcpReceiverReport(report_blocks, 50, time_ms);
EXPECT_EQ(bitrate_observer_.last_bitrate_, last_bitrate);
EXPECT_EQ(WeightedLoss(20, 0, 1, 255), bitrate_observer_.last_fraction_loss_);
EXPECT_EQ(WeightedLoss(20, 1, 1, 255), bitrate_observer_.last_fraction_loss_);
EXPECT_EQ(50, bitrate_observer_.last_rtt_);
last_bitrate = bitrate_observer_.last_bitrate_;
sequence_number[0] += 20;
Expand Down
59 changes: 21 additions & 38 deletions modules/bitrate_controller/send_side_bandwidth_estimation.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ bool ReadBweLossExperimentParameters(float* low_loss_threshold,
} // namespace

SendSideBandwidthEstimation::SendSideBandwidthEstimation(RtcEventLog* event_log)
: lost_packets_since_last_loss_update_(0),
: lost_packets_since_last_loss_update_Q8_(0),
expected_packets_since_last_loss_update_(0),
current_bitrate_bps_(0),
min_bitrate_configured_(congestion_controller::GetMinBitrateBps()),
Expand All @@ -125,7 +125,6 @@ SendSideBandwidthEstimation::SendSideBandwidthEstimation(RtcEventLog* event_log)
initially_lost_packets_(0),
bitrate_at_2_seconds_kbps_(0),
uma_update_state_(kNoUpdate),
uma_rtt_state_(kNoUpdate),
rampup_uma_stats_updated_(kNumUmaRampupMetrics, false),
event_log_(event_log),
last_rtc_event_log_ms_(-1),
Expand Down Expand Up @@ -207,51 +206,46 @@ void SendSideBandwidthEstimation::UpdateDelayBasedEstimate(
}

void SendSideBandwidthEstimation::UpdateReceiverBlock(uint8_t fraction_loss,
int64_t rtt_ms,
int64_t rtt,
int number_of_packets,
int64_t now_ms) {
const int kRoundingConstant = 128;
int packets_lost = (static_cast<int>(fraction_loss) * number_of_packets +
kRoundingConstant) >>
8;
UpdatePacketsLost(packets_lost, number_of_packets, now_ms);
UpdateRtt(rtt_ms, now_ms);
}

void SendSideBandwidthEstimation::UpdatePacketsLost(int packets_lost,
int number_of_packets,
int64_t now_ms) {
last_feedback_ms_ = now_ms;
if (first_report_time_ms_ == -1)
first_report_time_ms_ = now_ms;

// Update RTT if we were able to compute an RTT based on this RTCP.
// FlexFEC doesn't send RTCP SR, which means we won't be able to compute RTT.
if (rtt > 0)
last_round_trip_time_ms_ = rtt;

// Check sequence number diff and weight loss report
if (number_of_packets > 0) {
// Calculate number of lost packets.
const int num_lost_packets_Q8 = fraction_loss * number_of_packets;
// Accumulate reports.
lost_packets_since_last_loss_update_ += packets_lost;
lost_packets_since_last_loss_update_Q8_ += num_lost_packets_Q8;
expected_packets_since_last_loss_update_ += number_of_packets;

// Don't generate a loss rate until it can be based on enough packets.
if (expected_packets_since_last_loss_update_ < kLimitNumPackets)
return;

has_decreased_since_last_fraction_loss_ = false;
int64_t lost_q8 = lost_packets_since_last_loss_update_ << 8;
int64_t expected = expected_packets_since_last_loss_update_;
last_fraction_loss_ = std::min<int>(lost_q8 / expected, 255);
last_fraction_loss_ = lost_packets_since_last_loss_update_Q8_ /
expected_packets_since_last_loss_update_;

// Reset accumulators.

lost_packets_since_last_loss_update_ = 0;
lost_packets_since_last_loss_update_Q8_ = 0;
expected_packets_since_last_loss_update_ = 0;
last_packet_report_ms_ = now_ms;
UpdateEstimate(now_ms);
}
UpdateUmaStatsPacketsLost(now_ms, packets_lost);
UpdateUmaStats(now_ms, rtt, (fraction_loss * number_of_packets) >> 8);
}

void SendSideBandwidthEstimation::UpdateUmaStatsPacketsLost(int64_t now_ms,
int packets_lost) {
void SendSideBandwidthEstimation::UpdateUmaStats(int64_t now_ms,
int64_t rtt,
int lost_packets) {
int bitrate_kbps = static_cast<int>((current_bitrate_bps_ + 500) / 1000);
for (size_t i = 0; i < kNumUmaRampupMetrics; ++i) {
if (!rampup_uma_stats_updated_[i] &&
Expand All @@ -262,12 +256,14 @@ void SendSideBandwidthEstimation::UpdateUmaStatsPacketsLost(int64_t now_ms,
}
}
if (IsInStartPhase(now_ms)) {
initially_lost_packets_ += packets_lost;
initially_lost_packets_ += lost_packets;
} else if (uma_update_state_ == kNoUpdate) {
uma_update_state_ = kFirstDone;
bitrate_at_2_seconds_kbps_ = bitrate_kbps;
RTC_HISTOGRAM_COUNTS("WebRTC.BWE.InitiallyLostPackets",
initially_lost_packets_, 0, 100, 50);
RTC_HISTOGRAM_COUNTS("WebRTC.BWE.InitialRtt", static_cast<int>(rtt), 0,
2000, 50);
RTC_HISTOGRAM_COUNTS("WebRTC.BWE.InitialBandwidthEstimate",
bitrate_at_2_seconds_kbps_, 0, 2000, 50);
} else if (uma_update_state_ == kFirstDone &&
Expand All @@ -280,19 +276,6 @@ void SendSideBandwidthEstimation::UpdateUmaStatsPacketsLost(int64_t now_ms,
}
}

void SendSideBandwidthEstimation::UpdateRtt(int64_t rtt_ms, int64_t now_ms) {
// Update RTT if we were able to compute an RTT based on this RTCP.
// FlexFEC doesn't send RTCP SR, which means we won't be able to compute RTT.
if (rtt_ms > 0)
last_round_trip_time_ms_ = rtt_ms;

if (!IsInStartPhase(now_ms) && uma_rtt_state_ == kNoUpdate) {
uma_rtt_state_ = kDone;
RTC_HISTOGRAM_COUNTS("WebRTC.BWE.InitialRtt", static_cast<int>(rtt_ms), 0,
2000, 50);
}
}

void SendSideBandwidthEstimation::UpdateEstimate(int64_t now_ms) {
uint32_t new_bitrate = current_bitrate_bps_;
// We trust the REMB and/or delay-based estimate during the first 2 seconds if
Expand Down Expand Up @@ -374,7 +357,7 @@ void SendSideBandwidthEstimation::UpdateEstimate(int64_t now_ms) {
new_bitrate *= 0.8;
// Reset accumulators since we've already acted on missing feedback and
// shouldn't to act again on these old lost packets.
lost_packets_since_last_loss_update_ = 0;
lost_packets_since_last_loss_update_Q8_ = 0;
expected_packets_since_last_loss_update_ = 0;
last_timeout_ms_ = now_ms;
}
Expand Down
15 changes: 3 additions & 12 deletions modules/bitrate_controller/send_side_bandwidth_estimation.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,18 +42,10 @@ class SendSideBandwidthEstimation {

// Call when we receive a RTCP message with a ReceiveBlock.
void UpdateReceiverBlock(uint8_t fraction_loss,
int64_t rtt_ms,
int64_t rtt,
int number_of_packets,
int64_t now_ms);

// Call when we receive a RTCP message with a ReceiveBlock.
void UpdatePacketsLost(int packets_lost,
int number_of_packets,
int64_t now_ms);

// Call when we receive a RTCP message with a ReceiveBlock.
void UpdateRtt(int64_t rtt, int64_t now_ms);

void SetBitrates(int send_bitrate,
int min_bitrate,
int max_bitrate);
Expand All @@ -66,7 +58,7 @@ class SendSideBandwidthEstimation {

bool IsInStartPhase(int64_t now_ms) const;

void UpdateUmaStatsPacketsLost(int64_t now_ms, int packets_lost);
void UpdateUmaStats(int64_t now_ms, int64_t rtt, int lost_packets);

// Updates history of min bitrates.
// After this method returns min_bitrate_history_.front().second contains the
Expand All @@ -80,7 +72,7 @@ class SendSideBandwidthEstimation {
std::deque<std::pair<int64_t, uint32_t> > min_bitrate_history_;

// incoming filters
int lost_packets_since_last_loss_update_;
int lost_packets_since_last_loss_update_Q8_;
int expected_packets_since_last_loss_update_;

uint32_t current_bitrate_bps_;
Expand All @@ -103,7 +95,6 @@ class SendSideBandwidthEstimation {
int initially_lost_packets_;
int bitrate_at_2_seconds_kbps_;
UmaState uma_update_state_;
UmaState uma_rtt_state_;
std::vector<bool> rampup_uma_stats_updated_;
RtcEventLog* event_log_;
int64_t last_rtc_event_log_ms_;
Expand Down
111 changes: 18 additions & 93 deletions modules/congestion_controller/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ rtc_static_library("congestion_controller") {
sources = [
"include/receive_side_congestion_controller.h",
"include/send_side_congestion_controller.h",
"pacer_controller.cc",
"pacer_controller.h",
"probe_controller.cc",
"probe_controller.h",
"receive_side_congestion_controller.cc",
"send_side_congestion_controller.cc",
]
Expand All @@ -37,14 +37,13 @@ rtc_static_library("congestion_controller") {
}

deps = [
":goog_cc",
":delay_based_bwe",
":estimators",
":transport_feedback",
"..:module_api",
"../..:webrtc_common",
"../../rtc_base:checks",
"../../rtc_base:rate_limiter",
"../../rtc_base:rtc_task_queue_api",
"../../rtc_base:sequenced_task_checker",
"../../system_wrappers",
"../../system_wrappers:field_trial_api",
"../../system_wrappers:metrics_api",
Expand All @@ -53,7 +52,6 @@ rtc_static_library("congestion_controller") {
"../pacing",
"../remote_bitrate_estimator",
"../rtp_rtcp:rtp_rtcp_format",
"./network_control",
]

if (!build_with_mozilla) {
Expand All @@ -71,54 +69,11 @@ rtc_static_library("transport_feedback") {
]

deps = [
"..:module_api",
"../../rtc_base:checks",
"../../rtc_base:rtc_base_approved",
"../../system_wrappers",
"../rtp_rtcp:rtp_rtcp_format",
]
}

rtc_static_library("goog_cc") {
configs += [ ":bwe_test_logging" ]
sources = [
"alr_detector.cc",
"alr_detector.h",
"goog_cc_network_control.cc",
"goog_cc_network_control.h",
"include/goog_cc_factory.h",
"probe_controller.cc",
"probe_controller.h",
]

# TODO(jschuh): Bug 1348: fix this warning.
configs += [ "//build/config/compiler:no_size_t_to_int_warning" ]

if (!build_with_chromium && is_clang) {
# Suppress warnings from the Chromium Clang plugin (bugs.webrtc.org/163).
suppressed_configs += [ "//build/config/clang:find_bad_constructs" ]
}

deps = [
":delay_based_bwe",
":estimators",
"..:module_api",
"../..:webrtc_common",
"../../:typedefs",
"../../api:optional",
"../../logging:rtc_event_log_api",
"../../logging:rtc_event_pacing",
"../../modules:module_api",
"../../rtc_base:checks",
"../../rtc_base:rtc_base_approved",
"../../rtc_base/experiments:alr_experiment",
"../../system_wrappers",
"../../system_wrappers:field_trial_api",
"../../system_wrappers:metrics_api",
"../bitrate_controller",
"../pacing",
"../remote_bitrate_estimator",
"../../system_wrappers:system_wrappers",
"../rtp_rtcp:rtp_rtcp_format",
"./network_control",
]
}

Expand Down Expand Up @@ -155,7 +110,7 @@ rtc_source_set("estimators") {
"../../rtc_base:rtc_numerics",
"../../system_wrappers:field_trial_api",
"../../system_wrappers:metrics_api",
"../remote_bitrate_estimator",
"../remote_bitrate_estimator:remote_bitrate_estimator",
"../rtp_rtcp:rtp_rtcp_format",
]
}
Expand Down Expand Up @@ -190,16 +145,25 @@ if (rtc_include_tests) {
testonly = true

sources = [
"acknowledged_bitrate_estimator_unittest.cc",
"congestion_controller_unittests_helper.cc",
"congestion_controller_unittests_helper.h",
"delay_based_bwe_unittest.cc",
"delay_based_bwe_unittest_helper.cc",
"delay_based_bwe_unittest_helper.h",
"median_slope_estimator_unittest.cc",
"probe_bitrate_estimator_unittest.cc",
"probe_controller_unittest.cc",
"receive_side_congestion_controller_unittest.cc",
"send_side_congestion_controller_unittest.cc",
"send_time_history_unittest.cc",
"transport_feedback_adapter_unittest.cc",
"trendline_estimator_unittest.cc",
]
deps = [
":congestion_controller",
":goog_cc_unittests",
":delay_based_bwe",
":estimators",
":mock_congestion_controller",
":transport_feedback",
"../../logging:mocks",
Expand All @@ -210,51 +174,12 @@ if (rtc_include_tests) {
"../../system_wrappers",
"../../test:field_trial",
"../../test:test_support",
"../bitrate_controller:bitrate_controller",
"../bitrate_controller:mocks",
"../pacing:mock_paced_sender",
"../pacing:pacing",
"../remote_bitrate_estimator:remote_bitrate_estimator",
"../rtp_rtcp:rtp_rtcp_format",
"./network_control",
"//testing/gmock",
]
if (!build_with_chromium && is_clang) {
# Suppress warnings from the Chromium Clang plugin (bugs.webrtc.org/163).
suppressed_configs += [ "//build/config/clang:find_bad_constructs" ]
}
}

rtc_source_set("goog_cc_unittests") {
testonly = true

sources = [
"acknowledged_bitrate_estimator_unittest.cc",
"alr_detector_unittest.cc",
"delay_based_bwe_unittest.cc",
"delay_based_bwe_unittest_helper.cc",
"delay_based_bwe_unittest_helper.h",
"median_slope_estimator_unittest.cc",
"probe_bitrate_estimator_unittest.cc",
"probe_controller_unittest.cc",
"trendline_estimator_unittest.cc",
]
deps = [
":delay_based_bwe",
":estimators",
":goog_cc",
"../../rtc_base:checks",
"../../rtc_base:rtc_base_approved",
"../../rtc_base:rtc_base_tests_utils",
"../../rtc_base/experiments:alr_experiment",
"../../system_wrappers",
"../../test:field_trial",
"../../test:test_support",
"../pacing",
"../remote_bitrate_estimator",
"../rtp_rtcp:rtp_rtcp_format",
"./network_control",
"./network_control:network_control_unittests",
"//testing/gmock",
]
if (!build_with_chromium && is_clang) {
# Suppress warnings from the Chromium Clang plugin (bugs.webrtc.org/163).
Expand Down
2 changes: 0 additions & 2 deletions modules/congestion_controller/acknowledged_bitrate_estimator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ bool IsInSendTimeHistory(const PacketFeedback& packet) {
AcknowledgedBitrateEstimator::AcknowledgedBitrateEstimator()
: AcknowledgedBitrateEstimator(rtc::MakeUnique<BitrateEstimator>()) {}

AcknowledgedBitrateEstimator::~AcknowledgedBitrateEstimator() {}

AcknowledgedBitrateEstimator::AcknowledgedBitrateEstimator(
std::unique_ptr<BitrateEstimator> bitrate_estimator)
: bitrate_estimator_(std::move(bitrate_estimator)) {}
Expand Down
1 change: 0 additions & 1 deletion modules/congestion_controller/acknowledged_bitrate_estimator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ class AcknowledgedBitrateEstimator {
std::unique_ptr<BitrateEstimator> bitrate_estimator);

AcknowledgedBitrateEstimator();
~AcknowledgedBitrateEstimator();

void IncomingPacketFeedbackVector(
const std::vector<PacketFeedback>& packet_feedback_vector);
Expand Down
Loading

0 comments on commit ea86bb7

Please sign in to comment.