From faf398785b7076b760bcfb455e140a990da9ea8e Mon Sep 17 00:00:00 2001 From: Per K Date: Fri, 22 Mar 2024 12:16:18 +0100 Subject: [PATCH] Split ModuleRtpRtcpImpl2::TrySendPacket into three subfunctions. The purpose of these new methods are to allow creating a RTP packet with sequence numbers that can be inspected and is ensured to be sent if SendPacket is invoked. virtual bool CanSendPacket(const RtpPacketToSend& packet) const = 0; virtual void AssignSequenceNumber(RtpPacketToSend& packet) = 0; virtual void SendPacket(std::unique_ptr packet, const PacedPacketInfo& pacing_info) = 0; Bug: webrtc:15368 Change-Id: I671e737575e15328e796aa98761a4d540c5812d6 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/343785 Reviewed-by: Danil Chapovalov Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/main@{#41951} --- modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 13 +++++ modules/rtp_rtcp/source/rtp_rtcp_impl.h | 15 +++++ modules/rtp_rtcp/source/rtp_rtcp_impl2.cc | 38 +++++++++---- modules/rtp_rtcp/source/rtp_rtcp_impl2.h | 7 +++ .../source/rtp_rtcp_impl2_unittest.cc | 56 +++++++++++++++++++ modules/rtp_rtcp/source/rtp_rtcp_interface.h | 19 ++++++- 6 files changed, 136 insertions(+), 12 deletions(-) diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index 6872448f98..6aec7234a6 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -83,6 +83,19 @@ class MockRtpRtcpInterface : public RtpRtcpInterface { OnSendingRtpFrame, (uint32_t, int64_t, int, bool), (override)); + MOCK_METHOD(bool, + CanSendPacket, + (const RtpPacketToSend& packet), + (const override)); + MOCK_METHOD(void, + AssignSequenceNumber, + (RtpPacketToSend & packet), + (override)); + MOCK_METHOD(void, + SendPacket, + (std::unique_ptr packet, + const PacedPacketInfo& pacing_info), + (override)); MOCK_METHOD(bool, TrySendPacket, (std::unique_ptr packet, diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index d298081432..143b5f10cc 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -34,6 +34,7 @@ #include "modules/rtp_rtcp/source/rtp_packet_history.h" #include "modules/rtp_rtcp/source/rtp_packet_to_send.h" #include "modules/rtp_rtcp/source/rtp_sender.h" +#include "rtc_base/checks.h" #include "rtc_base/gtest_prod_util.h" #include "rtc_base/synchronization/mutex.h" @@ -135,6 +136,20 @@ class ABSL_DEPRECATED("") ModuleRtpRtcpImpl bool TrySendPacket(std::unique_ptr packet, const PacedPacketInfo& pacing_info) override; + bool CanSendPacket(const RtpPacketToSend& packet) const override { + RTC_DCHECK_NOTREACHED() << "Not implemented"; + return false; + } + + void AssignSequenceNumber(RtpPacketToSend& packet) override { + RTC_DCHECK_NOTREACHED() << "Not implemented"; + } + + void SendPacket(std::unique_ptr packet, + const PacedPacketInfo& pacing_info) override { + RTC_DCHECK_NOTREACHED() << "Not implemented"; + } + void OnBatchComplete() override {} void SetFecProtectionParams(const FecProtectionParams& delta_params, diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc index 3dbbc77086..7f0fdb8947 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc @@ -340,27 +340,45 @@ bool ModuleRtpRtcpImpl2::OnSendingRtpFrame(uint32_t timestamp, return true; } -bool ModuleRtpRtcpImpl2::TrySendPacket(std::unique_ptr packet, - const PacedPacketInfo& pacing_info) { +bool ModuleRtpRtcpImpl2::CanSendPacket(const RtpPacketToSend& packet) const { RTC_DCHECK(rtp_sender_); RTC_DCHECK_RUN_ON(&rtp_sender_->sequencing_checker); if (!rtp_sender_->packet_generator.SendingMedia()) { return false; } - if (packet->packet_type() == RtpPacketMediaType::kPadding && - packet->Ssrc() == rtp_sender_->packet_generator.SSRC() && + if (packet.packet_type() == RtpPacketMediaType::kPadding && + packet.Ssrc() == rtp_sender_->packet_generator.SSRC() && !rtp_sender_->sequencer.CanSendPaddingOnMediaSsrc()) { // New media packet preempted this generated padding packet, discard it. return false; } - bool is_flexfec = - packet->packet_type() == RtpPacketMediaType::kForwardErrorCorrection && - packet->Ssrc() == rtp_sender_->packet_generator.FlexfecSsrc(); - if (!is_flexfec) { - rtp_sender_->sequencer.Sequence(*packet); - } + return true; +} +void ModuleRtpRtcpImpl2::AssignSequenceNumber(RtpPacketToSend& packet) { + RTC_DCHECK_RUN_ON(&rtp_sender_->sequencing_checker); + bool is_flexfec = + packet.packet_type() == RtpPacketMediaType::kForwardErrorCorrection && + packet.Ssrc() == rtp_sender_->packet_generator.FlexfecSsrc(); + if (!is_flexfec) { + rtp_sender_->sequencer.Sequence(packet); + } +} + +void ModuleRtpRtcpImpl2::SendPacket(std::unique_ptr packet, + const PacedPacketInfo& pacing_info) { + RTC_DCHECK_RUN_ON(&rtp_sender_->sequencing_checker); + RTC_DCHECK(CanSendPacket(*packet)); rtp_sender_->packet_sender.SendPacket(std::move(packet), pacing_info); +} + +bool ModuleRtpRtcpImpl2::TrySendPacket(std::unique_ptr packet, + const PacedPacketInfo& pacing_info) { + if (!packet || !CanSendPacket(*packet)) { + return false; + } + AssignSequenceNumber(*packet); + SendPacket(std::move(packet), pacing_info); return true; } diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.h b/modules/rtp_rtcp/source/rtp_rtcp_impl2.h index 36e6deffba..c6f631137e 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.h @@ -144,6 +144,13 @@ class ModuleRtpRtcpImpl2 final : public RtpRtcpInterface, int payload_type, bool force_sender_report) override; + bool CanSendPacket(const RtpPacketToSend& packet) const override; + + void AssignSequenceNumber(RtpPacketToSend& packet) override; + + void SendPacket(std::unique_ptr packet, + const PacedPacketInfo& pacing_info) override; + bool TrySendPacket(std::unique_ptr packet, const PacedPacketInfo& pacing_info) override; void OnBatchComplete() override; diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc index 8df4c3ecbd..6dd6ae8684 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc @@ -19,12 +19,14 @@ #include "absl/types/optional.h" #include "api/field_trials_registry.h" #include "api/units/time_delta.h" +#include "modules/rtp_rtcp/include/rtp_header_extension_map.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtcp_packet.h" #include "modules/rtp_rtcp/source/rtcp_packet/nack.h" #include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "modules/rtp_rtcp/source/rtp_rtcp_interface.h" #include "modules/rtp_rtcp/source/rtp_sender_video.h" +#include "modules/rtp_rtcp/source/rtp_sequence_number_map.h" #include "rtc_base/logging.h" #include "rtc_base/rate_limiter.h" #include "rtc_base/strings/string_builder.h" @@ -1149,4 +1151,58 @@ TEST_F(RtpRtcpImpl2Test, RtxRtpStateReflectsCurrentState) { EXPECT_EQ(rtx_state.sequence_number, rtx_packet.SequenceNumber() + 1); } +TEST_F(RtpRtcpImpl2Test, CanSendPacketReturnTrueForMediaPacketIfSendingMedia) { + RtpHeaderExtensionMap extensions; + RtpPacketToSend packet(&extensions); + packet.SetSsrc(sender_.impl_->SSRC()); + packet.set_packet_type(RtpPacketMediaType::kAudio); + sender_.impl_->SetSendingMediaStatus(true); + + EXPECT_TRUE(sender_.impl_->CanSendPacket(packet)); +} + +TEST_F(RtpRtcpImpl2Test, + CanSendPacketReturnFalseForMediaPacketIfNotSendingMedia) { + RtpHeaderExtensionMap extensions; + RtpPacketToSend packet(&extensions); + packet.SetSsrc(sender_.impl_->SSRC()); + packet.set_packet_type(RtpPacketMediaType::kAudio); + sender_.impl_->SetSendingMediaStatus(false); + + EXPECT_FALSE(sender_.impl_->CanSendPacket(packet)); +} + +TEST_F(RtpRtcpImpl2Test, + CanSendPacketReturnFalseForPaddingPacketOnMediaSsrcBeforeMediaPacket) { + RtpHeaderExtensionMap extensions; + RtpPacketToSend packet(&extensions); + packet.SetSsrc(sender_.impl_->SSRC()); + packet.set_packet_type(RtpPacketMediaType::kPadding); + sender_.impl_->SetSendingMediaStatus(true); + + EXPECT_FALSE(sender_.impl_->CanSendPacket(packet)); +} + +TEST_F(RtpRtcpImpl2Test, RtpSequenceNumberSetByAssignSequenceNumber) { + RtpHeaderExtensionMap extensions; + RtpPacketToSend packet(&extensions); + packet.SetSsrc(sender_.impl_->SSRC()); + + sender_.impl_->SetSequenceNumber(1); + sender_.impl_->AssignSequenceNumber(packet); + EXPECT_EQ(packet.SequenceNumber(), 1); + sender_.impl_->AssignSequenceNumber(packet); + EXPECT_EQ(packet.SequenceNumber(), 2); +} + +TEST_F(RtpRtcpImpl2Test, SendPacketSendsPacketOnTransport) { + RtpHeaderExtensionMap extensions; + auto packet = std::make_unique(&extensions); + packet->SetSsrc(sender_.impl_->SSRC()); + packet->set_packet_type(RtpPacketMediaType::kAudio); + + sender_.impl_->SendPacket(std::move(packet), PacedPacketInfo()); + EXPECT_EQ(sender_.RtpSent(), 1); +} + } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_rtcp_interface.h b/modules/rtp_rtcp/source/rtp_rtcp_interface.h index 924c764ccd..e41a3fde0b 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_interface.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_interface.h @@ -313,8 +313,23 @@ class RtpRtcpInterface : public RtcpFeedbackSenderInterface { virtual bool TrySendPacket(std::unique_ptr packet, const PacedPacketInfo& pacing_info) = 0; - // Notifies that a batch of packet sends is completed. The implementation can - // use this to optimize packet sending. + // Returns true if the module can send media packets and the module is ready + // so send `packet` A RTP Sequence numbers may or may not have been assigned + // to the packet. + virtual bool CanSendPacket(const RtpPacketToSend& packet) const = 0; + + // Assigns continuous RTP sequence number to packet. + virtual void AssignSequenceNumber(RtpPacketToSend& packet) = 0; + + // Send the packet to transport. Before using this method, a caller must + // ensure the packet can be sent by first checking if the packet can be sent + // using CanSendPacket and the packet must be assigned a sequence number using + // AssignSequenceNumber. + virtual void SendPacket(std::unique_ptr packet, + const PacedPacketInfo& pacing_info) = 0; + + // Notifies that a batch of packet sends is completed. The implementation + // can use this to optimize packet sending. virtual void OnBatchComplete() = 0; // Update the FEC protection parameters to use for delta- and key-frames.