From 279f37052c50ae0e590e7f027d2b4400e9954921 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Tue, 13 Oct 2020 21:55:07 +0200 Subject: [PATCH] Makes WebRTC-Pacer-SmallFirstProbePacket default enabled. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Sebastian Jansson Reviewed-by: Philip Eliasson Cr-Commit-Position: refs/heads/master@{#32394} --- .../goog_cc_network_control_unittest.cc | 4 +- modules/pacing/pacing_controller.cc | 2 +- modules/pacing/pacing_controller_unittest.cc | 4 +- .../task_queue_paced_sender_unittest.cc | 8 +- test/scenario/scenario_config.h | 2 + test/scenario/video_stream.cc | 3 + test/scenario/video_stream_unittest.cc | 64 +++++++ video/video_send_stream_tests.cc | 174 ------------------ 8 files changed, 80 insertions(+), 181 deletions(-) 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 361da92ff2..c12681c5c5 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 @@ TEST_F(GoogCcNetworkControllerTest, // 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 @@ TEST_F(GoogCcNetworkControllerTest, 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 107316d4e1..133c97fe83 100644 --- a/modules/pacing/pacing_controller.cc +++ b/modules/pacing/pacing_controller.cc @@ -112,7 +112,7 @@ PacingController::PacingController(Clock* clock, 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 7340282c07..a953d5b439 100644 --- a/modules/pacing/pacing_controller_unittest.cc +++ b/modules/pacing/pacing_controller_unittest.cc @@ -1429,7 +1429,8 @@ TEST_P(PacingControllerTest, ProbingWithInsertedPackets) { 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, OwnedPacketPrioritizedOnType) { } TEST_P(PacingControllerTest, SmallFirstProbePacket) { - ScopedFieldTrials trial("WebRTC-Pacer-SmallFirstProbePacket/Enabled/"); MockPacketSender callback; pacer_ = std::make_unique(&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 7fe21d1f2e..d389e271f7 100644 --- a/modules/pacing/task_queue_paced_sender_unittest.cc +++ b/modules/pacing/task_queue_paced_sender_unittest.cc @@ -469,6 +469,7 @@ namespace test { // 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 @@ namespace test { 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 @@ namespace test { 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 c9d636a67f..c7320e9dc3 100644 --- a/test/scenario/scenario_config.h +++ b/test/scenario/scenario_config.h @@ -129,6 +129,7 @@ struct VideoStreamConfig { using Codec = VideoCodecType; Codec codec = Codec::kVideoCodecGeneric; absl::optional max_data_rate; + absl::optional min_data_rate; absl::optional max_framerate; // Counted in frame count. absl::optional key_frame_interval = 3000; @@ -149,6 +150,7 @@ struct VideoStreamConfig { 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 4284c190ea..3c368861d3 100644 --- a/test/scenario/video_stream.cc +++ b/test/scenario/video_stream.cc @@ -265,6 +265,7 @@ VideoEncoderConfig CreateVideoEncoderConfig(VideoStreamConfig config) { 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 @@ SendVideoStream::SendVideoStream(CallClient* sender, 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 19735a8fab..52be3f82ff 100644 --- a/test/scenario/video_stream_unittest.cc +++ b/test/scenario/video_stream_unittest.cc @@ -244,5 +244,69 @@ TEST(VideoStreamTest, ResolutionAdaptsToAvailableBandwidth) { 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 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 02e8b2bd7f..300c4ac34e 100644 --- a/video/video_send_stream_tests.cc +++ b/video/video_send_stream_tests.cc @@ -1276,180 +1276,6 @@ TEST_F(VideoSendStreamTest, FragmentsVp8AccordingToMaxPacketSizeWithFec) { 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 { - 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& 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* 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()); - } - RTCPSender::FeedbackState feedback_state; - EXPECT_EQ(0, rtcp_sender.SendRTCP(feedback_state, kRtcpRr)); - } - - std::unique_ptr 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) {