diff --git a/api/transport/network_types.h b/api/transport/network_types.h index ec37a22e39..10fc0beedf 100644 --- a/api/transport/network_types.h +++ b/api/transport/network_types.h @@ -107,7 +107,11 @@ struct SentPacket { DataSize size = DataSize::Zero(); // Size of preceeding packets that are not part of feedback. DataSize prior_unacked_data = DataSize::Zero(); + // Probe cluster id and parameters including bitrate, number of packets and + // number of bytes. PacedPacketInfo pacing_info; + // True if the packet is an audio packet, false for video, padding, RTX etc. + bool audio = false; // Transport independent sequence number, any tracked packet should have a // sequence number that is unique over the whole call and increasing by 1 for // each packet. diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.cc b/modules/congestion_controller/goog_cc/delay_based_bwe.cc index b39da880a0..33995ff2b5 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe.cc +++ b/modules/congestion_controller/goog_cc/delay_based_bwe.cc @@ -34,15 +34,18 @@ constexpr int kAbsSendTimeFraction = 18; constexpr int kAbsSendTimeInterArrivalUpshift = 8; constexpr int kInterArrivalShift = kAbsSendTimeFraction + kAbsSendTimeInterArrivalUpshift; +constexpr int kTimestampGroupTicks = + (kTimestampGroupLengthMs << kInterArrivalShift) / 1000; constexpr double kTimestampToMs = 1000.0 / static_cast(1 << kInterArrivalShift); + // This ssrc is used to fulfill the current API but will be removed // after the API has been changed. constexpr uint32_t kFixedSsrc = 0; - } // namespace constexpr char BweIgnoreSmallPacketsSettings::kKey[]; +constexpr char BweSeparateAudioPacketsSettings::kKey[]; BweIgnoreSmallPacketsSettings::BweIgnoreSmallPacketsSettings( const WebRtcKeyValueConfig* key_value_config) { @@ -58,6 +61,20 @@ BweIgnoreSmallPacketsSettings::Parser() { "small", &small_threshold); } +BweSeparateAudioPacketsSettings::BweSeparateAudioPacketsSettings( + const WebRtcKeyValueConfig* key_value_config) { + Parser()->Parse( + key_value_config->Lookup(BweSeparateAudioPacketsSettings::kKey)); +} + +std::unique_ptr +BweSeparateAudioPacketsSettings::Parser() { + return StructParametersParser::Create( // + "enabled", &enabled, // + "packet_threshold", &packet_threshold, // + "time_threshold", &time_threshold); +} + DelayBasedBwe::Result::Result() : updated(false), probe(false), @@ -72,8 +89,6 @@ DelayBasedBwe::Result::Result(bool probe, DataRate target_bitrate) recovered_from_overuse(false), backoff_in_alr(false) {} -DelayBasedBwe::Result::~Result() {} - DelayBasedBwe::DelayBasedBwe(const WebRtcKeyValueConfig* key_value_config, RtcEventLog* event_log, NetworkStatePredictor* network_state_predictor) @@ -81,10 +96,17 @@ DelayBasedBwe::DelayBasedBwe(const WebRtcKeyValueConfig* key_value_config, key_value_config_(key_value_config), ignore_small_(key_value_config), fraction_large_packets_(0.5), + separate_audio_(key_value_config), + audio_packets_since_last_video_(0), + last_video_packet_recv_time_(Timestamp::MinusInfinity()), network_state_predictor_(network_state_predictor), - inter_arrival_(), - delay_detector_( + video_inter_arrival_(), + video_delay_detector_( new TrendlineEstimator(key_value_config_, network_state_predictor_)), + audio_inter_arrival_(), + audio_delay_detector_( + new TrendlineEstimator(key_value_config_, network_state_predictor_)), + active_delay_detector_(video_delay_detector_.get()), last_seen_packet_(Timestamp::MinusInfinity()), uma_recorded_(false), rate_control_(key_value_config, /*send_side=*/true), @@ -94,8 +116,10 @@ DelayBasedBwe::DelayBasedBwe(const WebRtcKeyValueConfig* key_value_config, alr_limited_backoff_enabled_( key_value_config->Lookup("WebRTC-Bwe-AlrLimitedBackoff") .find("Enabled") == 0) { - RTC_LOG(LS_INFO) << "Initialized DelayBasedBwe with field trial " + RTC_LOG(LS_INFO) << "Initialized DelayBasedBwe with small packet filtering " << ignore_small_.Parser()->Encode() + << ", separate audio overuse detection" + << separate_audio_.Parser()->Encode() << " and alr limited backoff " << (alr_limited_backoff_enabled_ ? "enabled" : "disabled"); } @@ -127,15 +151,15 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector( } bool delayed_feedback = true; bool recovered_from_overuse = false; - BandwidthUsage prev_detector_state = delay_detector_->State(); + BandwidthUsage prev_detector_state = active_delay_detector_->State(); for (const auto& packet_feedback : packet_feedback_vector) { delayed_feedback = false; IncomingPacketFeedback(packet_feedback, msg.feedback_time); if (prev_detector_state == BandwidthUsage::kBwUnderusing && - delay_detector_->State() == BandwidthUsage::kBwNormal) { + active_delay_detector_->State() == BandwidthUsage::kBwNormal) { recovered_from_overuse = true; } - prev_detector_state = delay_detector_->State(); + prev_detector_state = active_delay_detector_->State(); } if (delayed_feedback) { @@ -155,25 +179,18 @@ void DelayBasedBwe::IncomingPacketFeedback(const PacketResult& packet_feedback, // Reset if the stream has timed out. if (last_seen_packet_.IsInfinite() || at_time - last_seen_packet_ > kStreamTimeOut) { - inter_arrival_.reset( - new InterArrival((kTimestampGroupLengthMs << kInterArrivalShift) / 1000, - kTimestampToMs, true)); - delay_detector_.reset( + video_inter_arrival_.reset( + new InterArrival(kTimestampGroupTicks, kTimestampToMs, true)); + video_delay_detector_.reset( new TrendlineEstimator(key_value_config_, network_state_predictor_)); + audio_inter_arrival_.reset( + new InterArrival(kTimestampGroupTicks, kTimestampToMs, true)); + audio_delay_detector_.reset( + new TrendlineEstimator(key_value_config_, network_state_predictor_)); + active_delay_detector_ = video_delay_detector_.get(); } last_seen_packet_ = at_time; - uint32_t send_time_24bits = - static_cast( - ((static_cast(packet_feedback.sent_packet.send_time.ms()) - << kAbsSendTimeFraction) + - 500) / - 1000) & - 0x00FFFFFF; - // Shift up send time to use the full 32 bits that inter_arrival works with, - // so wrapping works properly. - uint32_t timestamp = send_time_24bits << kAbsSendTimeInterArrivalUpshift; - // Ignore "small" packets if many/most packets in the call are "large". The // packet size may have a significant effect on the propagation delay, // especially at low bandwidths. Variations in packet size will then show up @@ -190,17 +207,51 @@ void DelayBasedBwe::IncomingPacketFeedback(const PacketResult& packet_feedback, } } - uint32_t ts_delta = 0; - int64_t t_delta = 0; + // As an alternative to ignoring small packets, we can separate audio and + // video packets for overuse detection. + InterArrival* inter_arrival_for_packet = video_inter_arrival_.get(); + DelayIncreaseDetectorInterface* delay_detector_for_packet = + video_delay_detector_.get(); + if (separate_audio_.enabled) { + if (packet_feedback.sent_packet.audio) { + inter_arrival_for_packet = audio_inter_arrival_.get(); + delay_detector_for_packet = audio_delay_detector_.get(); + audio_packets_since_last_video_++; + if (audio_packets_since_last_video_ > separate_audio_.packet_threshold && + packet_feedback.receive_time - last_video_packet_recv_time_ > + separate_audio_.time_threshold) { + active_delay_detector_ = audio_delay_detector_.get(); + } + } else { + audio_packets_since_last_video_ = 0; + last_video_packet_recv_time_ = + std::max(last_video_packet_recv_time_, packet_feedback.receive_time); + active_delay_detector_ = video_delay_detector_.get(); + } + } + + uint32_t send_time_24bits = + static_cast( + ((static_cast(packet_feedback.sent_packet.send_time.ms()) + << kAbsSendTimeFraction) + + 500) / + 1000) & + 0x00FFFFFF; + // Shift up send time to use the full 32 bits that inter_arrival works with, + // so wrapping works properly. + uint32_t timestamp = send_time_24bits << kAbsSendTimeInterArrivalUpshift; + + uint32_t timestamp_delta = 0; + int64_t recv_delta_ms = 0; int size_delta = 0; - bool calculated_deltas = inter_arrival_->ComputeDeltas( + bool calculated_deltas = inter_arrival_for_packet->ComputeDeltas( timestamp, packet_feedback.receive_time.ms(), at_time.ms(), - packet_size.bytes(), &ts_delta, &t_delta, &size_delta); - double ts_delta_ms = (1000.0 * ts_delta) / (1 << kInterArrivalShift); - delay_detector_->Update(t_delta, ts_delta_ms, - packet_feedback.sent_packet.send_time.ms(), - packet_feedback.receive_time.ms(), - packet_size.bytes(), calculated_deltas); + packet_size.bytes(), ×tamp_delta, &recv_delta_ms, &size_delta); + double send_delta_ms = (1000.0 * timestamp_delta) / (1 << kInterArrivalShift); + delay_detector_for_packet->Update(recv_delta_ms, send_delta_ms, + packet_feedback.sent_packet.send_time.ms(), + packet_feedback.receive_time.ms(), + packet_size.bytes(), calculated_deltas); } DataRate DelayBasedBwe::TriggerOveruse(Timestamp at_time, @@ -219,7 +270,7 @@ DelayBasedBwe::Result DelayBasedBwe::MaybeUpdateEstimate( Result result; // Currently overusing the bandwidth. - if (delay_detector_->State() == BandwidthUsage::kBwOverusing) { + if (active_delay_detector_->State() == BandwidthUsage::kBwOverusing) { if (has_once_detected_overuse_ && in_alr && alr_limited_backoff_enabled_) { if (rate_control_.TimeToReduceFurther(at_time, prev_bitrate_)) { result.updated = @@ -254,7 +305,7 @@ DelayBasedBwe::Result DelayBasedBwe::MaybeUpdateEstimate( result.recovered_from_overuse = recovered_from_overuse; } } - BandwidthUsage detector_state = delay_detector_->State(); + BandwidthUsage detector_state = active_delay_detector_->State(); if ((result.updated && prev_bitrate_ != result.target_bitrate) || detector_state != prev_state_) { DataRate bitrate = result.updated ? result.target_bitrate : prev_bitrate_; @@ -275,7 +326,7 @@ DelayBasedBwe::Result DelayBasedBwe::MaybeUpdateEstimate( bool DelayBasedBwe::UpdateEstimate(Timestamp at_time, absl::optional acked_bitrate, DataRate* target_rate) { - const RateControlInput input(delay_detector_->State(), acked_bitrate); + const RateControlInput input(active_delay_detector_->State(), acked_bitrate); *target_rate = rate_control_.Update(&input, at_time); return rate_control_.ValidEstimate(); } diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.h b/modules/congestion_controller/goog_cc/delay_based_bwe.h index 03845949a4..25f5a3be72 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe.h +++ b/modules/congestion_controller/goog_cc/delay_based_bwe.h @@ -48,12 +48,26 @@ struct BweIgnoreSmallPacketsSettings { std::unique_ptr Parser(); }; +struct BweSeparateAudioPacketsSettings { + static constexpr char kKey[] = "WebRTC-Bwe-SeparateAudioPackets"; + + BweSeparateAudioPacketsSettings() = default; + explicit BweSeparateAudioPacketsSettings( + const WebRtcKeyValueConfig* key_value_config); + + bool enabled = false; + int packet_threshold = 10; + TimeDelta time_threshold = TimeDelta::Seconds(1); + + std::unique_ptr Parser(); +}; + class DelayBasedBwe { public: struct Result { Result(); Result(bool probe, DataRate target_bitrate); - ~Result(); + ~Result() = default; bool updated; bool probe; DataRate target_bitrate = DataRate::Zero(); @@ -108,9 +122,20 @@ class DelayBasedBwe { BweIgnoreSmallPacketsSettings ignore_small_; double fraction_large_packets_; + // Alternatively, run two separate overuse detectors for audio and video, + // and fall back to the audio one if we haven't seen a video packet in a + // while. + BweSeparateAudioPacketsSettings separate_audio_; + int64_t audio_packets_since_last_video_; + Timestamp last_video_packet_recv_time_; + NetworkStatePredictor* network_state_predictor_; - std::unique_ptr inter_arrival_; - std::unique_ptr delay_detector_; + std::unique_ptr video_inter_arrival_; + std::unique_ptr video_delay_detector_; + std::unique_ptr audio_inter_arrival_; + std::unique_ptr audio_delay_detector_; + DelayIncreaseDetectorInterface* active_delay_detector_; + Timestamp last_seen_packet_; bool uma_recorded_; AimdRateControl rate_control_; diff --git a/modules/congestion_controller/goog_cc/test/goog_cc_printer.cc b/modules/congestion_controller/goog_cc/test/goog_cc_printer.cc index bfbc054cad..52baab06c7 100644 --- a/modules/congestion_controller/goog_cc/test/goog_cc_printer.cc +++ b/modules/congestion_controller/goog_cc/test/goog_cc_printer.cc @@ -80,7 +80,7 @@ std::deque GoogCcStatePrinter::CreateLoggers() { }; auto trend = [this] { return reinterpret_cast( - controller_->delay_based_bwe_->delay_detector_.get()); + controller_->delay_based_bwe_->active_delay_detector_); }; auto acknowledged_rate = [this] { return controller_->acknowledged_bitrate_estimator_->bitrate(); diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter.cc b/modules/congestion_controller/rtp/transport_feedback_adapter.cc index b98de9c768..d2256eae97 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter.cc @@ -75,6 +75,7 @@ void TransportFeedbackAdapter::AddPacket(const RtpPacketSendInfo& packet_info, packet.sent.sequence_number = seq_num_unwrapper_.Unwrap(packet_info.transport_sequence_number); packet.sent.size = DataSize::Bytes(packet_info.length + overhead_bytes); + packet.sent.audio = packet_info.packet_type == RtpPacketMediaType::kAudio; packet.local_net_id = local_net_id_; packet.remote_net_id = remote_net_id_; packet.sent.pacing_info = packet_info.pacing_info; @@ -89,6 +90,7 @@ void TransportFeedbackAdapter::AddPacket(const RtpPacketSendInfo& packet_info, } history_.insert(std::make_pair(packet.sent.sequence_number, packet)); } + absl::optional TransportFeedbackAdapter::ProcessSentPacket( const rtc::SentPacket& sent_packet) { auto send_time = Timestamp::Millis(sent_packet.send_time_ms); diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc index 31692d589a..8356928ba7 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc @@ -113,6 +113,7 @@ class TransportFeedbackAdapterTest : public ::testing::Test { packet_info.has_rtp_sequence_number = true; packet_info.length = packet_feedback.sent_packet.size.bytes(); packet_info.pacing_info = packet_feedback.sent_packet.pacing_info; + packet_info.packet_type = RtpPacketMediaType::kVideo; adapter_->AddPacket(RtpPacketSendInfo(packet_info), 0u, clock_.CurrentTime()); adapter_->ProcessSentPacket(rtc::SentPacket( @@ -395,6 +396,7 @@ TEST_F(TransportFeedbackAdapterTest, IgnoreDuplicatePacketSentCalls) { packet_info.transport_sequence_number = packet.sent_packet.sequence_number; packet_info.length = packet.sent_packet.size.bytes(); packet_info.pacing_info = packet.sent_packet.pacing_info; + packet_info.packet_type = RtpPacketMediaType::kVideo; adapter_->AddPacket(packet_info, 0u, clock_.CurrentTime()); absl::optional sent_packet = adapter_->ProcessSentPacket( rtc::SentPacket(packet.sent_packet.sequence_number, diff --git a/pc/BUILD.gn b/pc/BUILD.gn index c41f21a5a7..d8561af442 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -598,6 +598,7 @@ if (rtc_include_tests) { "../test:field_trial", "../test:fileutils", "../test:rtp_test_utils", + "./scenario_tests:pc_scenario_tests", "//third_party/abseil-cpp/absl/algorithm:container", "//third_party/abseil-cpp/absl/memory", "//third_party/abseil-cpp/absl/strings", diff --git a/pc/scenario_tests/BUILD.gn b/pc/scenario_tests/BUILD.gn new file mode 100644 index 0000000000..bcb69b9129 --- /dev/null +++ b/pc/scenario_tests/BUILD.gn @@ -0,0 +1,25 @@ +# Copyright (c) 2020 The WebRTC project authors. All Rights Reserved. +# +# Use of this source code is governed by a BSD-style license +# that can be found in the LICENSE file in the root of the source +# tree. An additional intellectual property rights grant can be found +# in the file PATENTS. All contributing project authors may +# be found in the AUTHORS file in the root of the source tree. + +import("../../webrtc.gni") + +if (rtc_include_tests) { + rtc_library("pc_scenario_tests") { + testonly = true + sources = [ "goog_cc_test.cc" ] + deps = [ + "../../api:rtc_stats_api", + "../../modules/rtp_rtcp:rtp_rtcp", + "../../pc:pc_test_utils", + "../../pc:rtc_pc_base", + "../../test:field_trial", + "../../test:test_support", + "../../test/peer_scenario:peer_scenario", + ] + } +} diff --git a/pc/scenario_tests/goog_cc_test.cc b/pc/scenario_tests/goog_cc_test.cc new file mode 100644 index 0000000000..fba617dd5c --- /dev/null +++ b/pc/scenario_tests/goog_cc_test.cc @@ -0,0 +1,109 @@ +/* + * Copyright (c) 2020 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "api/stats/rtc_stats_collector_callback.h" +#include "api/stats/rtcstats_objects.h" +#include "pc/test/mock_peer_connection_observers.h" +#include "test/field_trial.h" +#include "test/gtest.h" +#include "test/peer_scenario/peer_scenario.h" +#include "test/peer_scenario/peer_scenario_client.h" + +namespace webrtc { +namespace test { + +// TODO(terelius): Use fake encoder and enable on Android once +// https://bugs.chromium.org/p/webrtc/issues/detail?id=11408 is fixed. +#if defined(WEBRTC_ANDROID) +#define MAYBE_NoBweChangeFromVideoUnmute DISABLED_NoBweChangeFromVideoUnmute +#else +#define MAYBE_NoBweChangeFromVideoUnmute NoBweChangeFromVideoUnmute +#endif +TEST(GoogCcPeerScenarioTest, MAYBE_NoBweChangeFromVideoUnmute) { + // If transport wide sequence numbers are used for audio, and the call + // switches from audio only to video only, there will be a sharp change in + // packets sizes. This will create a change in propagation time which might be + // detected as an overuse. Using separate overuse detectors for audio and + // video avoids the issue. + std::string audio_twcc_trials( + "WebRTC-Audio-SendSideBwe/Enabled/" // + "WebRTC-SendSideBwe-WithOverhead/Enabled/" // + "WebRTC-Audio-AlrProbing/Disabled/"); + std::string separate_audio_video( + "WebRTC-Bwe-SeparateAudioPackets/" + "enabled:true,packet_threshold:15,time_threshold:1000ms/"); + ScopedFieldTrials field_trial(audio_twcc_trials + separate_audio_video); + PeerScenario s(*test_info_); + auto* caller = s.CreateClient(PeerScenarioClient::Config()); + auto* callee = s.CreateClient(PeerScenarioClient::Config()); + + BuiltInNetworkBehaviorConfig net_conf; + net_conf.link_capacity_kbps = 350; + net_conf.queue_delay_ms = 50; + auto send_node = s.net()->CreateEmulatedNode(net_conf); + auto ret_node = s.net()->CreateEmulatedNode(net_conf); + + PeerScenarioClient::VideoSendTrackConfig video_conf; + video_conf.generator.squares_video->framerate = 15; + auto video = caller->CreateVideo("VIDEO", video_conf); + auto audio = caller->CreateAudio("AUDIO", cricket::AudioOptions()); + + // Start ICE and exchange SDP. + s.SimpleConnection(caller, callee, {send_node}, {ret_node}); + + // Limit the encoder bitrate to ensure that there are no actual BWE overuses. + ASSERT_EQ(caller->pc()->GetSenders().size(), 2u); // 2 senders. + int num_video_streams = 0; + for (auto& rtp_sender : caller->pc()->GetSenders()) { + auto parameters = rtp_sender->GetParameters(); + ASSERT_EQ(parameters.encodings.size(), 1u); // 1 stream per sender. + for (auto& encoding_parameters : parameters.encodings) { + if (encoding_parameters.ssrc == video.sender->ssrc()) { + num_video_streams++; + encoding_parameters.max_bitrate_bps = 220000; + encoding_parameters.max_framerate = 15; + } + } + rtp_sender->SetParameters(parameters); + } + ASSERT_EQ(num_video_streams, 1); // Exactly 1 video stream. + + auto get_bwe = [&] { + rtc::scoped_refptr callback( + new rtc::RefCountedObject()); + caller->pc()->GetStats(callback); + s.net()->time_controller()->Wait([&] { return callback->called(); }); + auto stats = + callback->report()->GetStatsOfType()[0]; + return DataRate::BitsPerSec(*stats->available_outgoing_bitrate); + }; + + s.ProcessMessages(TimeDelta::Seconds(15)); + const DataRate initial_bwe = get_bwe(); + EXPECT_GE(initial_bwe, DataRate::KilobitsPerSec(300)); + + // 10 seconds audio only. Bandwidth should not drop. + video.capturer->Stop(); + s.ProcessMessages(TimeDelta::Seconds(10)); + EXPECT_GE(get_bwe(), initial_bwe); + + // Resume video but stop audio. Bandwidth should not drop. + video.capturer->Start(); + RTCError status = caller->pc()->RemoveTrackNew(audio.sender); + ASSERT_TRUE(status.ok()); + audio.track->set_enabled(false); + for (int i = 0; i < 10; i++) { + s.ProcessMessages(TimeDelta::Seconds(1)); + EXPECT_GE(get_bwe(), initial_bwe); + } +} + +} // namespace test +} // namespace webrtc diff --git a/test/peer_scenario/peer_scenario_client.h b/test/peer_scenario/peer_scenario_client.h index 6e82b23567..d939d7f3a7 100644 --- a/test/peer_scenario/peer_scenario_client.h +++ b/test/peer_scenario/peer_scenario_client.h @@ -98,8 +98,8 @@ class PeerScenarioClient { }; struct AudioSendTrack { - AudioTrackInterface* track; - RtpSenderInterface* sender; + rtc::scoped_refptr track; + rtc::scoped_refptr sender; }; struct VideoSendTrack { diff --git a/test/peer_scenario/tests/peer_scenario_quality_test.cc b/test/peer_scenario/tests/peer_scenario_quality_test.cc index 2b79e5b21c..5d69a0923f 100644 --- a/test/peer_scenario/tests/peer_scenario_quality_test.cc +++ b/test/peer_scenario/tests/peer_scenario_quality_test.cc @@ -10,6 +10,7 @@ #include "test/gtest.h" #include "test/peer_scenario/peer_scenario.h" +#include "test/peer_scenario/peer_scenario_client.h" namespace webrtc { namespace test {