From 16999814e6f250a9b6b99b47d4ffc594d5adc0a0 Mon Sep 17 00:00:00 2001 From: Per Kjellander Date: Thu, 10 Oct 2019 12:57:28 +0200 Subject: [PATCH] Add void::RtcpFeedbackSenderInterface::SendCombinedRtcpPacket This method sends arbitrary number rtp::RcpPackets into one or more IP packets. It is implemented both in RtcpTranceiver and in RtpRtcp. Change-Id: I00424ee2f1730ff98626f768846f4ac1ad864933 BUG: webrtc:10742 Change-Id: I00424ee2f1730ff98626f768846f4ac1ad864933 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/156240 Commit-Queue: Per Kjellander Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#29430} --- modules/rtp_rtcp/include/rtp_rtcp_defines.h | 3 + modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 3 + modules/rtp_rtcp/source/rtcp_sender.cc | 96 +++++++++++++++---- modules/rtp_rtcp/source/rtcp_sender.h | 2 + .../rtp_rtcp/source/rtcp_sender_unittest.cc | 18 ++++ modules/rtp_rtcp/source/rtcp_transceiver.cc | 11 +++ modules/rtp_rtcp/source/rtcp_transceiver.h | 6 ++ .../rtp_rtcp/source/rtcp_transceiver_impl.cc | 14 +++ .../rtp_rtcp/source/rtcp_transceiver_impl.h | 5 + .../source/rtcp_transceiver_unittest.cc | 43 +++++++++ modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 5 + modules/rtp_rtcp/source/rtp_rtcp_impl.h | 3 + 12 files changed, 193 insertions(+), 16 deletions(-) diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index 74922175b6..0b9284bf86 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -14,6 +14,7 @@ #include #include +#include #include #include "absl/strings/string_view.h" @@ -317,6 +318,8 @@ class RtcpFeedbackSenderInterface { virtual bool SendFeedbackPacket(const rtcp::TransportFeedback& feedback) = 0; virtual bool SendNetworkStateEstimatePacket( const rtcp::RemoteEstimate& packet) = 0; + virtual void SendCombinedRtcpPacket( + std::vector> rtcp_packets) = 0; virtual void SetRemb(int64_t bitrate_bps, std::vector ssrcs) = 0; virtual void UnsetRemb() = 0; }; diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index 347d4cee0f..bf280f3239 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -150,6 +150,9 @@ class MockRtpRtcp : public RtpRtcp { MOCK_METHOD1(SendFeedbackPacket, bool(const rtcp::TransportFeedback& packet)); MOCK_METHOD1(SendNetworkStateEstimatePacket, bool(const rtcp::RemoteEstimate& packet)); + MOCK_METHOD1( + SendCombinedRtcpPacket, + void(std::vector> rtcp_packets)); MOCK_METHOD1(SetTargetSendBitrate, void(uint32_t bitrate_bps)); MOCK_METHOD4(SendLossNotification, int32_t(uint16_t last_decoded_seq_num, diff --git a/modules/rtp_rtcp/source/rtcp_sender.cc b/modules/rtp_rtcp/source/rtcp_sender.cc index 1c6d15490e..15325d1592 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/modules/rtp_rtcp/source/rtcp_sender.cc @@ -50,22 +50,6 @@ const uint32_t kRtcpAnyExtendedReports = kRtcpXrReceiverReferenceTime | kRtcpXrTargetBitrate; constexpr int32_t kDefaultVideoReportInterval = 1000; constexpr int32_t kDefaultAudioReportInterval = 5000; -} // namespace - -RTCPSender::FeedbackState::FeedbackState() - : packets_sent(0), - media_bytes_sent(0), - send_bitrate(0), - last_rr_ntp_secs(0), - last_rr_ntp_frac(0), - remote_sr(0), - module(nullptr) {} - -RTCPSender::FeedbackState::FeedbackState(const FeedbackState&) = default; - -RTCPSender::FeedbackState::FeedbackState(FeedbackState&&) = default; - -RTCPSender::FeedbackState::~FeedbackState() = default; class PacketContainer : public rtcp::CompoundPacket { public: @@ -96,6 +80,57 @@ class PacketContainer : public rtcp::CompoundPacket { RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(PacketContainer); }; +// Helper to put several RTCP packets into lower layer datagram RTCP packet. +// Prefer to use this class instead of PacketContainer. +class PacketSender { + public: + PacketSender(rtcp::RtcpPacket::PacketReadyCallback callback, + size_t max_packet_size) + : callback_(callback), max_packet_size_(max_packet_size) { + RTC_CHECK_LE(max_packet_size, IP_PACKET_SIZE); + } + ~PacketSender() { RTC_DCHECK_EQ(index_, 0) << "Unsent rtcp packet."; } + + // Appends a packet to pending compound packet. + // Sends rtcp packet if buffer is full and resets the buffer. + void AppendPacket(const rtcp::RtcpPacket& packet) { + packet.Create(buffer_, &index_, max_packet_size_, callback_); + } + + // Sends pending rtcp packet. + void Send() { + if (index_ > 0) { + callback_(rtc::ArrayView(buffer_, index_)); + index_ = 0; + } + } + + bool IsEmpty() const { return index_ == 0; } + + private: + const rtcp::RtcpPacket::PacketReadyCallback callback_; + const size_t max_packet_size_; + size_t index_ = 0; + uint8_t buffer_[IP_PACKET_SIZE]; +}; + +} // namespace + +RTCPSender::FeedbackState::FeedbackState() + : packets_sent(0), + media_bytes_sent(0), + send_bitrate(0), + last_rr_ntp_secs(0), + last_rr_ntp_frac(0), + remote_sr(0), + module(nullptr) {} + +RTCPSender::FeedbackState::FeedbackState(const FeedbackState&) = default; + +RTCPSender::FeedbackState::FeedbackState(FeedbackState&&) = default; + +RTCPSender::FeedbackState::~FeedbackState() = default; + class RTCPSender::RtcpContext { public: RtcpContext(const FeedbackState& feedback_state, @@ -1014,4 +1049,33 @@ bool RTCPSender::SendNetworkStateEstimatePacket( return packet.Build(max_packet_size, callback) && send_success; } +void RTCPSender::SendCombinedRtcpPacket( + std::vector> rtcp_packets) { + size_t max_packet_size; + uint32_t ssrc; + { + rtc::CritScope lock(&critical_section_rtcp_sender_); + if (method_ == RtcpMode::kOff) { + RTC_LOG(LS_WARNING) << "Can't send rtcp if it is disabled."; + return; + } + + max_packet_size = max_packet_size_; + ssrc = ssrc_; + } + RTC_DCHECK_LE(max_packet_size, IP_PACKET_SIZE); + auto callback = [&](rtc::ArrayView packet) { + if (transport_->SendRtcp(packet.data(), packet.size())) { + if (event_log_) + event_log_->Log(std::make_unique(packet)); + } + }; + PacketSender sender(callback, max_packet_size); + for (auto& rtcp_packet : rtcp_packets) { + rtcp_packet->SetSenderSsrc(ssrc); + sender.AppendPacket(*rtcp_packet); + } + sender.Send(); +} + } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtcp_sender.h b/modules/rtp_rtcp/source/rtcp_sender.h index 33db97ad94..6deee878a9 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.h +++ b/modules/rtp_rtcp/source/rtcp_sender.h @@ -142,6 +142,8 @@ class RTCPSender { void SetVideoBitrateAllocation(const VideoBitrateAllocation& bitrate); bool SendFeedbackPacket(const rtcp::TransportFeedback& packet); bool SendNetworkStateEstimatePacket(const rtcp::RemoteEstimate& packet); + void SendCombinedRtcpPacket( + std::vector> rtcp_packets); private: class RtcpContext; diff --git a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index a077836925..c3f3920d3e 100644 --- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -11,6 +11,7 @@ #include "modules/rtp_rtcp/source/rtcp_sender.h" #include +#include #include "absl/base/macros.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" @@ -849,4 +850,21 @@ TEST_F(RtcpSenderTest, SchedulesReportOnSsrcChange) { EXPECT_TRUE(rtcp_sender_->TimeToSendRTCPReport(false)); } +TEST_F(RtcpSenderTest, SendsCombinedRtcpPacket) { + rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); + + std::vector> packets; + auto transport_feedback = std::make_unique(); + transport_feedback->AddReceivedPacket(321, 10000); + packets.push_back(std::move(transport_feedback)); + auto remote_estimate = std::make_unique(); + packets.push_back(std::move(remote_estimate)); + rtcp_sender_->SendCombinedRtcpPacket(std::move(packets)); + + EXPECT_EQ(parser()->transport_feedback()->num_packets(), 1); + EXPECT_EQ(parser()->transport_feedback()->sender_ssrc(), kSenderSsrc); + EXPECT_EQ(parser()->app()->num_packets(), 1); + EXPECT_EQ(parser()->app()->sender_ssrc(), kSenderSsrc); +} + } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtcp_transceiver.cc b/modules/rtp_rtcp/source/rtcp_transceiver.cc index 4538301c2a..46e222cdb6 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver.cc +++ b/modules/rtp_rtcp/source/rtcp_transceiver.cc @@ -12,6 +12,7 @@ #include #include +#include #include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" #include "rtc_base/checks.h" @@ -131,6 +132,16 @@ bool RtcpTransceiver::SendNetworkStateEstimatePacket( return true; } +void RtcpTransceiver::SendCombinedRtcpPacket( + std::vector> rtcp_packets) { + RTC_CHECK(rtcp_transceiver_); + RtcpTransceiverImpl* ptr = rtcp_transceiver_.get(); + task_queue_->PostTask( + [ptr, rtcp_packets = std::move(rtcp_packets)]() mutable { + ptr->SendCombinedRtcpPacket(std::move(rtcp_packets)); + }); +} + void RtcpTransceiver::SendNack(uint32_t ssrc, std::vector sequence_numbers) { RTC_CHECK(rtcp_transceiver_); diff --git a/modules/rtp_rtcp/source/rtcp_transceiver.h b/modules/rtp_rtcp/source/rtcp_transceiver.h index 8b70c6d987..5468e2521e 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver.h +++ b/modules/rtp_rtcp/source/rtcp_transceiver.h @@ -82,6 +82,12 @@ class RtcpTransceiver : public RtcpFeedbackSenderInterface { bool SendNetworkStateEstimatePacket( const rtcp::RemoteEstimate& packet) override; + // TODO(bugs.webrtc.org/8239): Remove SendCombinedRtcpPacket + // and move generating of the TransportFeedback message inside + // RtcpTransceiverImpl when there is one RtcpTransceiver per rtp transport. + void SendCombinedRtcpPacket( + std::vector> rtcp_packets) override; + // Reports missing packets, https://tools.ietf.org/html/rfc4585#section-6.2.1 void SendNack(uint32_t ssrc, std::vector sequence_numbers); diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc b/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc index e982421d0d..977fc8b7b7 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc +++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc @@ -380,6 +380,20 @@ void RtcpTransceiverImpl::SendPeriodicCompoundPacket() { sender.Send(); } +void RtcpTransceiverImpl::SendCombinedRtcpPacket( + std::vector> rtcp_packets) { + auto send_packet = [this](rtc::ArrayView packet) { + config_.outgoing_transport->SendRtcp(packet.data(), packet.size()); + }; + PacketSender sender(send_packet, config_.max_packet_size); + + for (auto& rtcp_packet : rtcp_packets) { + rtcp_packet->SetSenderSsrc(config_.feedback_ssrc); + sender.AppendPacket(*rtcp_packet); + } + sender.Send(); +} + void RtcpTransceiverImpl::SendImmediateFeedback( const rtcp::RtcpPacket& rtcp_packet) { auto send_packet = [this](rtc::ArrayView packet) { diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl.h b/modules/rtp_rtcp/source/rtcp_transceiver_impl.h index 083f77e379..8039f2b70f 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_impl.h +++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl.h @@ -63,6 +63,11 @@ class RtcpTransceiverImpl { void SendPictureLossIndication(uint32_t ssrc); void SendFullIntraRequest(rtc::ArrayView ssrcs); + // SendCombinedRtcpPacket ignores rtcp mode and does not send a compound + // message. https://tools.ietf.org/html/rfc4585#section-3.1 + void SendCombinedRtcpPacket( + std::vector> rtcp_packets); + private: class PacketSender; struct RemoteSenderState; diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc index 8be17ca47e..568d348035 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc @@ -11,7 +11,9 @@ #include "modules/rtp_rtcp/source/rtcp_transceiver.h" #include +#include +#include "modules/rtp_rtcp/source/rtcp_packet/remote_estimate.h" #include "modules/rtp_rtcp/source/rtcp_packet/sender_report.h" #include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" #include "rtc_base/event.h" @@ -19,6 +21,7 @@ #include "test/gmock.h" #include "test/gtest.h" #include "test/mock_transport.h" +#include "test/rtcp_packet_parser.h" namespace { @@ -32,7 +35,10 @@ using ::webrtc::MockTransport; using ::webrtc::RtcpTransceiver; using ::webrtc::RtcpTransceiverConfig; using ::webrtc::TaskQueueForTest; +using ::webrtc::rtcp::RemoteEstimate; +using ::webrtc::rtcp::RtcpPacket; using ::webrtc::rtcp::TransportFeedback; +using ::webrtc::test::RtcpPacketParser; class MockMediaReceiverRtcpObserver : public webrtc::MediaReceiverRtcpObserver { public: @@ -283,4 +289,41 @@ TEST(RtcpTransceiverTest, SendsTransportFeedbackOnTaskQueue) { WaitPostedTasks(&queue); } +TEST(RtcpTransceiverTest, SendsCombinedRtcpPacketOnTaskQueue) { + static constexpr uint32_t kSenderSsrc = 12345; + + MockTransport outgoing_transport; + TaskQueueForTest queue("rtcp"); + RtcpTransceiverConfig config; + config.feedback_ssrc = kSenderSsrc; + config.outgoing_transport = &outgoing_transport; + config.task_queue = &queue; + config.schedule_periodic_compound_packets = false; + RtcpTransceiver rtcp_transceiver(config); + + EXPECT_CALL(outgoing_transport, SendRtcp) + .WillOnce([&](const uint8_t* buffer, size_t size) { + EXPECT_TRUE(queue.IsCurrent()); + RtcpPacketParser rtcp_parser; + rtcp_parser.Parse(buffer, size); + EXPECT_EQ(rtcp_parser.transport_feedback()->num_packets(), 1); + EXPECT_EQ(rtcp_parser.transport_feedback()->sender_ssrc(), kSenderSsrc); + EXPECT_EQ(rtcp_parser.app()->num_packets(), 1); + EXPECT_EQ(rtcp_parser.app()->sender_ssrc(), kSenderSsrc); + return true; + }); + + // Create minimalistic transport feedback packet. + std::vector> packets; + auto transport_feedback = std::make_unique(); + transport_feedback->AddReceivedPacket(321, 10000); + packets.push_back(std::move(transport_feedback)); + + auto remote_estimate = std::make_unique(); + packets.push_back(std::move(remote_estimate)); + + rtcp_transceiver.SendCombinedRtcpPacket(std::move(packets)); + WaitPostedTasks(&queue); +} + } // namespace diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index cb826f6655..7d8e33868a 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -657,6 +657,11 @@ bool ModuleRtpRtcpImpl::SendNetworkStateEstimatePacket( return rtcp_sender_.SendNetworkStateEstimatePacket(packet); } +void ModuleRtpRtcpImpl::SendCombinedRtcpPacket( + std::vector> rtcp_packets) { + rtcp_sender_.SendCombinedRtcpPacket(std::move(rtcp_packets)); +} + int32_t ModuleRtpRtcpImpl::SendLossNotification(uint16_t last_decoded_seq_num, uint16_t last_received_seq_num, bool decodability_flag, diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 2359fec7df..9ec481c842 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -237,6 +237,9 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { bool SendFeedbackPacket(const rtcp::TransportFeedback& packet) override; bool SendNetworkStateEstimatePacket( const rtcp::RemoteEstimate& packet) override; + void SendCombinedRtcpPacket( + std::vector> rtcp_packets) override; + // (APP) Application specific data. int32_t SetRTCPApplicationSpecificData(uint8_t sub_type, uint32_t name,