Makes WebRTC-Pacer-SmallFirstProbePacket default enabled.
This is expected to yield slightly higher bandwidth estimates when
probing is used, since it reduces a bias in how packet sizes are counted.
Bug: webrtc:11780
Change-Id: I6a4a3af0c50670d248dbe043a4d9da60915e3699
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/187491
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32394}
diff --git a/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc b/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc
index 361da92..c12681c 100644
--- a/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc
+++ b/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc
@@ -573,7 +573,7 @@
// trial, we have worse behavior.
DataRate average_bitrate =
AverageBitrateAfterCrossInducedLoss("googcc_unit/no_cross_loss_based");
- RTC_DCHECK_LE(average_bitrate, DataRate::KilobitsPerSec(650));
+ RTC_DCHECK_LE(average_bitrate, DataRate::KilobitsPerSec(625));
}
TEST_F(GoogCcNetworkControllerTest,
@@ -583,7 +583,7 @@
ScopedFieldTrials trial("WebRTC-Bwe-LossBasedControl/Enabled/");
DataRate average_bitrate =
AverageBitrateAfterCrossInducedLoss("googcc_unit/cross_loss_based");
- RTC_DCHECK_GE(average_bitrate, DataRate::KilobitsPerSec(750));
+ RTC_DCHECK_GE(average_bitrate, DataRate::KilobitsPerSec(725));
}
TEST_F(GoogCcNetworkControllerTest, LossBasedEstimatorCapsRateAtModerateLoss) {
diff --git a/modules/pacing/pacing_controller.cc b/modules/pacing/pacing_controller.cc
index 107316d..133c97f 100644
--- a/modules/pacing/pacing_controller.cc
+++ b/modules/pacing/pacing_controller.cc
@@ -112,7 +112,7 @@
IsEnabled(*field_trials_, "WebRTC-Pacer-PadInSilence")),
pace_audio_(IsEnabled(*field_trials_, "WebRTC-Pacer-BlockAudio")),
small_first_probe_packet_(
- IsEnabled(*field_trials_, "WebRTC-Pacer-SmallFirstProbePacket")),
+ !IsDisabled(*field_trials_, "WebRTC-Pacer-SmallFirstProbePacket")),
ignore_transport_overhead_(
IsEnabled(*field_trials_, "WebRTC-Pacer-IgnoreTransportOverhead")),
padding_target_duration_(GetDynamicPaddingTarget(*field_trials_)),
diff --git a/modules/pacing/pacing_controller_unittest.cc b/modules/pacing/pacing_controller_unittest.cc
index 7340282..a953d5b 100644
--- a/modules/pacing/pacing_controller_unittest.cc
+++ b/modules/pacing/pacing_controller_unittest.cc
@@ -1429,7 +1429,8 @@
EXPECT_NEAR((packets_sent - 1) * kPacketSize * 8000 /
(clock_.TimeInMilliseconds() - start),
kFirstClusterRate.bps(), kProbingErrorMargin.bps());
- EXPECT_EQ(0, packet_sender.padding_sent());
+ // Probing always starts with a small padding packet.
+ EXPECT_EQ(1, packet_sender.padding_sent());
clock_.AdvanceTime(TimeUntilNextProcess());
start = clock_.TimeInMilliseconds();
@@ -1738,7 +1739,6 @@
}
TEST_P(PacingControllerTest, SmallFirstProbePacket) {
- ScopedFieldTrials trial("WebRTC-Pacer-SmallFirstProbePacket/Enabled/");
MockPacketSender callback;
pacer_ = std::make_unique<PacingController>(&clock_, &callback, nullptr,
nullptr, GetParam());
diff --git a/modules/pacing/task_queue_paced_sender_unittest.cc b/modules/pacing/task_queue_paced_sender_unittest.cc
index 7fe21d1..d389e27 100644
--- a/modules/pacing/task_queue_paced_sender_unittest.cc
+++ b/modules/pacing/task_queue_paced_sender_unittest.cc
@@ -469,6 +469,7 @@
// Expected size for each probe in a cluster is twice the expected bits
// sent during min_probe_delta.
+ // Expect one additional call since probe always starts with a small
const TimeDelta kProbeTimeDelta = TimeDelta::Millis(2);
const DataSize kProbeSize = kProbeRate * kProbeTimeDelta;
const size_t kNumPacketsInProbe =
@@ -477,7 +478,7 @@
packet_router,
SendPacket(_, ::testing::Field(&PacedPacketInfo::probe_cluster_id,
kProbeClusterId)))
- .Times(kNumPacketsInProbe);
+ .Times(kNumPacketsInProbe + 1);
pacer.EnqueuePackets(
GeneratePackets(RtpPacketMediaType::kVideo, kNumPacketsInProbe));
@@ -546,7 +547,10 @@
time_controller.AdvanceTime(kMinProbeDelta);
// Verify the amount of probing data sent.
- EXPECT_EQ(data_sent, kProbingRate * TimeDelta::Millis(1));
+ // Probe always starts with a small (1 byte) padding packet that's not
+ // counted into the probe rate here.
+ EXPECT_EQ(data_sent,
+ kProbingRate * TimeDelta::Millis(1) + DataSize::Bytes(1));
}
} // namespace test
} // namespace webrtc
diff --git a/test/scenario/scenario_config.h b/test/scenario/scenario_config.h
index c9d636a..c7320e9 100644
--- a/test/scenario/scenario_config.h
+++ b/test/scenario/scenario_config.h
@@ -129,6 +129,7 @@
using Codec = VideoCodecType;
Codec codec = Codec::kVideoCodecGeneric;
absl::optional<DataRate> max_data_rate;
+ absl::optional<DataRate> min_data_rate;
absl::optional<int> max_framerate;
// Counted in frame count.
absl::optional<int> key_frame_interval = 3000;
@@ -149,6 +150,7 @@
DegradationPreference degradation_preference =
DegradationPreference::MAINTAIN_FRAMERATE;
+ bool suspend_below_min_bitrate = false;
} encoder;
struct Stream {
Stream();
diff --git a/test/scenario/video_stream.cc b/test/scenario/video_stream.cc
index 4284c19..3c368861 100644
--- a/test/scenario/video_stream.cc
+++ b/test/scenario/video_stream.cc
@@ -265,6 +265,7 @@
if (config.encoder.max_framerate) {
for (auto& layer : encoder_config.simulcast_layers) {
layer.max_framerate = *config.encoder.max_framerate;
+ layer.min_bitrate_bps = config.encoder.min_data_rate->bps_or(-1);
}
}
@@ -412,6 +413,8 @@
send_config.encoder_settings.encoder_factory = encoder_factory_.get();
send_config.encoder_settings.bitrate_allocator_factory =
bitrate_allocator_factory_.get();
+ send_config.suspend_below_min_bitrate =
+ config.encoder.suspend_below_min_bitrate;
sender_->SendTask([&] {
if (config.stream.fec_controller_factory) {
diff --git a/test/scenario/video_stream_unittest.cc b/test/scenario/video_stream_unittest.cc
index 19735a8..52be3f8 100644
--- a/test/scenario/video_stream_unittest.cc
+++ b/test/scenario/video_stream_unittest.cc
@@ -244,5 +244,69 @@
EXPECT_GT(num_vga_frames_, 0u);
}
+TEST(VideoStreamTest, SuspendsBelowMinBitrate) {
+ const DataRate kMinVideoBitrate = DataRate::KilobitsPerSec(30);
+
+ // Declared before scenario to avoid use after free.
+ std::atomic<Timestamp> last_frame_timestamp(Timestamp::MinusInfinity());
+
+ Scenario s;
+ NetworkSimulationConfig net_config;
+ net_config.bandwidth = kMinVideoBitrate * 4;
+ net_config.delay = TimeDelta::Millis(10);
+ auto* client = s.CreateClient("send", [&](CallClientConfig* c) {
+ // Min transmit rate needs to be lower than kMinVideoBitrate for this test
+ // to make sense.
+ c->transport.rates.min_rate = kMinVideoBitrate / 2;
+ c->transport.rates.start_rate = kMinVideoBitrate;
+ c->transport.rates.max_rate = kMinVideoBitrate * 2;
+ });
+ auto send_net = s.CreateMutableSimulationNode(
+ [&](NetworkSimulationConfig* c) { *c = net_config; });
+ auto ret_net = {s.CreateSimulationNode(net_config)};
+ auto* route =
+ s.CreateRoutes(client, {send_net->node()},
+ s.CreateClient("return", CallClientConfig()), ret_net);
+
+ s.CreateVideoStream(route->forward(), [&](VideoStreamConfig* c) {
+ c->hooks.frame_pair_handlers = {[&](const VideoFramePair& pair) {
+ if (pair.repeated == 0) {
+ last_frame_timestamp = pair.capture_time;
+ }
+ }};
+ c->source.framerate = 30;
+ c->source.generator.width = 320;
+ c->source.generator.height = 180;
+ c->encoder.implementation = CodecImpl::kFake;
+ c->encoder.codec = Codec::kVideoCodecVP8;
+ c->encoder.min_data_rate = kMinVideoBitrate;
+ c->encoder.suspend_below_min_bitrate = true;
+ c->stream.pad_to_rate = kMinVideoBitrate;
+ });
+
+ // Run for a few seconds, check we have received at least one frame.
+ s.RunFor(TimeDelta::Seconds(2));
+ EXPECT_TRUE(last_frame_timestamp.load().IsFinite());
+
+ // Degrade network to below min bitrate.
+ send_net->UpdateConfig([&](NetworkSimulationConfig* c) {
+ c->bandwidth = kMinVideoBitrate * 0.9;
+ });
+
+ // Run for 20s, verify that no frames arrive that were captured after the
+ // first five seconds, allowing some margin for BWE backoff to trigger and
+ // packets already in the pipeline to potentially arrive.
+ s.RunFor(TimeDelta::Seconds(20));
+ EXPECT_GT(s.Now() - last_frame_timestamp, TimeDelta::Seconds(15));
+
+ // Relax the network constraints and run for a while more, verify that we
+ // start receiving frames again.
+ send_net->UpdateConfig(
+ [&](NetworkSimulationConfig* c) { c->bandwidth = kMinVideoBitrate * 4; });
+ last_frame_timestamp = Timestamp::MinusInfinity();
+ s.RunFor(TimeDelta::Seconds(15));
+ EXPECT_TRUE(last_frame_timestamp.load().IsFinite());
+}
+
} // namespace test
} // namespace webrtc
diff --git a/video/video_send_stream_tests.cc b/video/video_send_stream_tests.cc
index 02e8b2b..300c4ac 100644
--- a/video/video_send_stream_tests.cc
+++ b/video/video_send_stream_tests.cc
@@ -1276,180 +1276,6 @@
TestPacketFragmentationSize(kVP8, true);
}
-// The test will go through a number of phases.
-// 1. Start sending packets.
-// 2. As soon as the RTP stream has been detected, signal a low REMB value to
-// suspend the stream.
-// 3. Wait until |kSuspendTimeFrames| have been captured without seeing any RTP
-// packets.
-// 4. Signal a high REMB and then wait for the RTP stream to start again.
-// When the stream is detected again, and the stats show that the stream
-// is no longer suspended, the test ends.
-TEST_F(VideoSendStreamTest, SuspendBelowMinBitrate) {
- static const int kSuspendTimeFrames = 60; // Suspend for 2 seconds @ 30 fps.
-
- class RembObserver : public test::SendTest {
- public:
- class CaptureObserver : public rtc::VideoSinkInterface<VideoFrame> {
- public:
- explicit CaptureObserver(RembObserver* remb_observer)
- : remb_observer_(remb_observer) {}
-
- void OnFrame(const VideoFrame&) {
- MutexLock lock(&remb_observer_->mutex_);
- if (remb_observer_->test_state_ == kDuringSuspend &&
- ++remb_observer_->suspended_frame_count_ > kSuspendTimeFrames) {
- VideoSendStream::Stats stats = remb_observer_->stream_->GetStats();
- EXPECT_TRUE(stats.suspended);
- remb_observer_->SendRtcpFeedback(remb_observer_->high_remb_bps_);
- remb_observer_->test_state_ = kWaitingForPacket;
- }
- }
-
- private:
- RembObserver* const remb_observer_;
- };
-
- RembObserver()
- : SendTest(kDefaultTimeoutMs),
- clock_(Clock::GetRealTimeClock()),
- capture_observer_(this),
- stream_(nullptr),
- test_state_(kBeforeSuspend),
- rtp_count_(0),
- last_sequence_number_(0),
- suspended_frame_count_(0),
- low_remb_bps_(0),
- high_remb_bps_(0) {}
-
- private:
- Action OnSendRtp(const uint8_t* packet, size_t length) override {
- MutexLock lock(&mutex_);
- ++rtp_count_;
- RtpPacket rtp_packet;
- EXPECT_TRUE(rtp_packet.Parse(packet, length));
- last_sequence_number_ = rtp_packet.SequenceNumber();
-
- if (test_state_ == kBeforeSuspend) {
- // The stream has started. Try to suspend it.
- SendRtcpFeedback(low_remb_bps_);
- test_state_ = kDuringSuspend;
- } else if (test_state_ == kDuringSuspend) {
- if (rtp_packet.padding_size() == 0) {
- // Received non-padding packet during suspension period. Reset the
- // counter.
- suspended_frame_count_ = 0;
- }
- SendRtcpFeedback(0); // REMB is only sent if value is > 0.
- } else if (test_state_ == kWaitingForPacket) {
- if (rtp_packet.padding_size() == 0) {
- // Non-padding packet observed. Test is almost complete. Will just
- // have to wait for the stats to change.
- test_state_ = kWaitingForStats;
- }
- SendRtcpFeedback(0); // REMB is only sent if value is > 0.
- } else if (test_state_ == kWaitingForStats) {
- VideoSendStream::Stats stats = stream_->GetStats();
- if (stats.suspended == false) {
- // Stats flipped to false. Test is complete.
- observation_complete_.Set();
- }
- SendRtcpFeedback(0); // REMB is only sent if value is > 0.
- }
-
- return SEND_PACKET;
- }
-
- void set_low_remb_bps(int value) {
- MutexLock lock(&mutex_);
- low_remb_bps_ = value;
- }
-
- void set_high_remb_bps(int value) {
- MutexLock lock(&mutex_);
- high_remb_bps_ = value;
- }
-
- void OnVideoStreamsCreated(
- VideoSendStream* send_stream,
- const std::vector<VideoReceiveStream*>& receive_streams) override {
- stream_ = send_stream;
- }
-
- void OnFrameGeneratorCapturerCreated(
- test::FrameGeneratorCapturer* frame_generator_capturer) override {
- frame_generator_capturer->AddOrUpdateSink(&capture_observer_,
- rtc::VideoSinkWants());
- }
-
- void ModifyVideoConfigs(
- VideoSendStream::Config* send_config,
- std::vector<VideoReceiveStream::Config>* receive_configs,
- VideoEncoderConfig* encoder_config) override {
- RTC_DCHECK_EQ(1, encoder_config->number_of_streams);
- transport_adapter_.reset(
- new internal::TransportAdapter(send_config->send_transport));
- transport_adapter_->Enable();
- send_config->rtp.nack.rtp_history_ms = kNackRtpHistoryMs;
- send_config->suspend_below_min_bitrate = true;
- int min_bitrate_bps =
- test::DefaultVideoStreamFactory::kDefaultMinBitratePerStream[0];
- set_low_remb_bps(min_bitrate_bps - 10000);
- int threshold_window = std::max(min_bitrate_bps / 10, 20000);
- ASSERT_GT(encoder_config->max_bitrate_bps,
- min_bitrate_bps + threshold_window + 5000);
- set_high_remb_bps(min_bitrate_bps + threshold_window + 5000);
- }
-
- void PerformTest() override {
- EXPECT_TRUE(Wait()) << "Timed out during suspend-below-min-bitrate test.";
- }
-
- enum TestState {
- kBeforeSuspend,
- kDuringSuspend,
- kWaitingForPacket,
- kWaitingForStats
- };
-
- virtual void SendRtcpFeedback(int remb_value)
- RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_) {
- FakeReceiveStatistics receive_stats(kVideoSendSsrcs[0],
- last_sequence_number_, rtp_count_, 0);
- RtpRtcpInterface::Configuration config;
- config.clock = clock_;
- config.receive_statistics = &receive_stats;
- config.outgoing_transport = transport_adapter_.get();
- config.rtcp_report_interval_ms = kRtcpIntervalMs;
- config.local_media_ssrc = kVideoSendSsrcs[0];
- RTCPSender rtcp_sender(config);
-
- rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize);
- rtcp_sender.SetRemoteSSRC(kVideoSendSsrcs[0]);
- if (remb_value > 0) {
- rtcp_sender.SetRemb(remb_value, std::vector<uint32_t>());
- }
- RTCPSender::FeedbackState feedback_state;
- EXPECT_EQ(0, rtcp_sender.SendRTCP(feedback_state, kRtcpRr));
- }
-
- std::unique_ptr<internal::TransportAdapter> transport_adapter_;
- Clock* const clock_;
- CaptureObserver capture_observer_;
- VideoSendStream* stream_;
-
- Mutex mutex_;
- TestState test_state_ RTC_GUARDED_BY(mutex_);
- int rtp_count_ RTC_GUARDED_BY(mutex_);
- int last_sequence_number_ RTC_GUARDED_BY(mutex_);
- int suspended_frame_count_ RTC_GUARDED_BY(mutex_);
- int low_remb_bps_ RTC_GUARDED_BY(mutex_);
- int high_remb_bps_ RTC_GUARDED_BY(mutex_);
- } test;
-
- RunBaseTest(&test);
-}
-
// This test that padding stops being send after a while if the Camera stops
// producing video frames and that padding resumes if the camera restarts.
TEST_F(VideoSendStreamTest, NoPaddingWhenVideoIsMuted) {
