From ffdecdce2cd0c2936edb140fb9798a65169b3901 Mon Sep 17 00:00:00 2001 From: Per K Date: Wed, 22 Feb 2023 13:53:27 +0100 Subject: [PATCH] Add possibility to start probe without waiting for media packet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:14928 Change-Id: If3c7fe39979e7a4d45b01dd59debd8ac69de3d01 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/294521 Reviewed-by: Erik Språng Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/main@{#39496} --- modules/pacing/bitrate_prober.cc | 30 +++++-- modules/pacing/pacing_controller.cc | 6 -- modules/pacing/pacing_controller_unittest.cc | 90 +++++++++++++++++++- 3 files changed, 110 insertions(+), 16 deletions(-) diff --git a/modules/pacing/bitrate_prober.cc b/modules/pacing/bitrate_prober.cc index e8ebf54f32..b218846c76 100644 --- a/modules/pacing/bitrate_prober.cc +++ b/modules/pacing/bitrate_prober.cc @@ -15,6 +15,7 @@ #include "absl/memory/memory.h" #include "api/rtc_event_log/rtc_event.h" #include "api/rtc_event_log/rtc_event_log.h" +#include "api/units/data_size.h" #include "logging/rtc_event_log/events/rtc_event_probe_cluster_created.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" @@ -92,15 +93,30 @@ void BitrateProber::CreateProbeCluster( cluster.pace_info.probe_cluster_id = cluster_config.id; clusters_.push(cluster); - RTC_LOG(LS_INFO) << "Probe cluster (bitrate:min bytes:min packets): (" + if (config_.min_packet_size.Get() > DataSize::Zero()) { + // If we are already probing, continue to do so. Otherwise set it to + // kInactive and wait for OnIncomingPacket with a large enough packet + // to start the probing. + if (probing_state_ != ProbingState::kActive) { + probing_state_ = ProbingState::kInactive; + } + } else { + // Probing is allowed to start without first sending a "large enough" + // packet. + if (probing_state_ == ProbingState::kInactive) { + // Send next probe right away. + next_probe_time_ = Timestamp::MinusInfinity(); + probing_state_ = ProbingState::kActive; + } + } + + RTC_LOG(LS_INFO) << "Probe cluster (bitrate_bps:min bytes:min packets): (" << cluster.pace_info.send_bitrate_bps << ":" << cluster.pace_info.probe_cluster_min_bytes << ":" - << cluster.pace_info.probe_cluster_min_probes << ")"; - - // If we are already probing, continue to do so. Otherwise set it to - // kInactive and wait for OnIncomingPacket to start the probing. - if (probing_state_ != ProbingState::kActive) - probing_state_ = ProbingState::kInactive; + << cluster.pace_info.probe_cluster_min_probes << ", " + << (probing_state_ == ProbingState::kInactive ? "Inactive" + : "Active") + << ")"; } Timestamp BitrateProber::NextProbeTime(Timestamp now) const { diff --git a/modules/pacing/pacing_controller.cc b/modules/pacing/pacing_controller.cc index 4b61bbcb4e..cd94c7bf3e 100644 --- a/modules/pacing/pacing_controller.cc +++ b/modules/pacing/pacing_controller.cc @@ -553,12 +553,6 @@ DataSize PacingController::PaddingToAdd(DataSize recommended_probe_size, return DataSize::Zero(); } - if (!seen_first_packet_) { - // We can not send padding unless a normal packet has first been sent. If - // we do, timestamps get messed up. - return DataSize::Zero(); - } - if (!recommended_probe_size.IsZero()) { if (recommended_probe_size > data_sent) { return recommended_probe_size - data_sent; diff --git a/modules/pacing/pacing_controller_unittest.cc b/modules/pacing/pacing_controller_unittest.cc index 3b3c3eb761..688a3cc613 100644 --- a/modules/pacing/pacing_controller_unittest.cc +++ b/modules/pacing/pacing_controller_unittest.cc @@ -20,6 +20,7 @@ #include "api/transport/network_types.h" #include "api/units/data_rate.h" #include "api/units/time_delta.h" +#include "api/units/timestamp.h" #include "modules/pacing/packet_router.h" #include "system_wrappers/include/clock.h" #include "test/explicit_key_value_config.h" @@ -211,12 +212,20 @@ class PacingControllerPadding : public PacingController::PacketSender { class PacingControllerProbing : public PacingController::PacketSender { public: - PacingControllerProbing() : packets_sent_(0), padding_sent_(0) {} + PacingControllerProbing() = default; + // Controls if padding can be generated or not. + // In real implementation, padding can only be generated after a sent + // media packet, or if the sender support RTX. + void SetCanGeneratePadding(bool can_generate) { + can_generate_padding_ = can_generate; + } void SendPacket(std::unique_ptr packet, const PacedPacketInfo& pacing_info) override { if (packet->packet_type() != RtpPacketMediaType::kPadding) { ++packets_sent_; + } else { + ++padding_packets_sent_; } last_pacing_info_ = pacing_info; } @@ -227,6 +236,9 @@ class PacingControllerProbing : public PacingController::PacketSender { std::vector> GeneratePadding( DataSize target_size) override { + if (!can_generate_padding_) { + return {}; + } // From RTPSender: // Max in the RFC 3550 is 255 bytes, we limit it to be modulus 32 for SRTP. const DataSize kMaxPadding = DataSize::Bytes(224); @@ -250,13 +262,16 @@ class PacingControllerProbing : public PacingController::PacketSender { } int packets_sent() const { return packets_sent_; } + int padding_packets_sent() const { return padding_packets_sent_; } int padding_sent() const { return padding_sent_; } int total_packets_sent() const { return packets_sent_ + padding_sent_; } PacedPacketInfo last_pacing_info() const { return last_pacing_info_; } private: - int packets_sent_; - int padding_sent_; + bool can_generate_padding_ = true; + int packets_sent_ = 0; + int padding_packets_sent_ = 0; + int padding_sent_ = 0; PacedPacketInfo last_pacing_info_; }; @@ -1337,6 +1352,75 @@ TEST_F(PacingControllerTest, ProbingWithPaddingSupport) { kFirstClusterRate.bps(), kProbingErrorMargin.bps()); } +TEST_F(PacingControllerTest, CanProbeWithPaddingBeforeFirstMediaPacket) { + // const size_t kPacketSize = 1200; + const int kInitialBitrateBps = 300000; + + PacingControllerProbing packet_sender; + const test::ExplicitKeyValueConfig trials( + "WebRTC-Bwe-ProbingBehavior/min_packet_size:0/"); + auto pacer = + std::make_unique(&clock_, &packet_sender, trials); + std::vector probe_clusters = { + {.at_time = clock_.CurrentTime(), + .target_data_rate = kFirstClusterRate, + .target_duration = TimeDelta::Millis(15), + .target_probe_count = 5, + .id = 0}}; + pacer->CreateProbeClusters(probe_clusters); + + pacer->SetPacingRates( + DataRate::BitsPerSec(kInitialBitrateBps * kPaceMultiplier), + DataRate::Zero()); + + Timestamp start = clock_.CurrentTime(); + Timestamp next_process = pacer->NextSendTime(); + while (clock_.CurrentTime() < start + TimeDelta::Millis(100) && + next_process.IsFinite()) { + AdvanceTimeUntil(next_process); + pacer->ProcessPackets(); + next_process = pacer->NextSendTime(); + } + EXPECT_GT(packet_sender.padding_packets_sent(), 5); +} + +TEST_F(PacingControllerTest, CanNotProbeWithPaddingIfGeneratePaddingFails) { + // const size_t kPacketSize = 1200; + const int kInitialBitrateBps = 300000; + + PacingControllerProbing packet_sender; + packet_sender.SetCanGeneratePadding(false); + const test::ExplicitKeyValueConfig trials( + "WebRTC-Bwe-ProbingBehavior/min_packet_size:0/"); + auto pacer = + std::make_unique(&clock_, &packet_sender, trials); + std::vector probe_clusters = { + {.at_time = clock_.CurrentTime(), + .target_data_rate = kFirstClusterRate, + .target_duration = TimeDelta::Millis(15), + .target_probe_count = 5, + .id = 0}}; + pacer->CreateProbeClusters(probe_clusters); + + pacer->SetPacingRates( + DataRate::BitsPerSec(kInitialBitrateBps * kPaceMultiplier), + DataRate::Zero()); + + Timestamp start = clock_.CurrentTime(); + int process_count = 0; + Timestamp next_process = pacer->NextSendTime(); + while (clock_.CurrentTime() < start + TimeDelta::Millis(100) && + next_process.IsFinite()) { + AdvanceTimeUntil(next_process); + pacer->ProcessPackets(); + ++process_count; + next_process = pacer->NextSendTime(); + } + + EXPECT_LT(process_count, 10); + EXPECT_EQ(packet_sender.padding_packets_sent(), 0); +} + TEST_F(PacingControllerTest, PaddingOveruse) { uint32_t ssrc = 12346; uint16_t sequence_number = 1234;