From e86af2c75fe6ac8bfc0b1ee31b1bdf10da14fe52 Mon Sep 17 00:00:00 2001 From: Elad Alon Date: Mon, 3 Jun 2019 14:37:50 +0200 Subject: [PATCH] Allowing buffering a LNTF (loss notification) feedback message in RTCPSender Loss notifications may either be sent immediately, or wait until another RTCP feedback message is sent. Bug: webrtc:10336 Change-Id: I40601d9fa1dec6c17b2ce905cb0c8cd2dcff7893 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/139242 Commit-Queue: Elad Alon Reviewed-by: Karl Wiberg Reviewed-by: Ilya Nikolaevskiy Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#28142} --- DEPS | 1 + abseil-in-webrtc.md | 5 +- modules/include/module_common_types.h | 3 +- modules/rtp_rtcp/BUILD.gn | 1 + modules/rtp_rtcp/include/rtp_rtcp.h | 3 +- modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 5 +- modules/rtp_rtcp/source/rtcp_sender.cc | 9 +++- modules/rtp_rtcp/source/rtcp_sender.h | 3 +- .../rtp_rtcp/source/rtcp_sender_unittest.cc | 50 ++++++++++++++++--- modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 5 +- modules/rtp_rtcp/source/rtp_rtcp_impl.h | 3 +- .../loss_notification_controller.cc | 2 +- .../loss_notification_controller_unittest.cc | 4 +- test/rtcp_packet_parser.cc | 3 ++ test/rtcp_packet_parser.h | 2 + video/rtp_video_stream_receiver.cc | 30 +++++++---- video/rtp_video_stream_receiver.h | 6 ++- 17 files changed, 100 insertions(+), 35 deletions(-) diff --git a/DEPS b/DEPS index 7c952ac5e4..1ce18e6ebd 100644 --- a/DEPS +++ b/DEPS @@ -1633,6 +1633,7 @@ include_rules = [ "+absl/algorithm/container.h", "+absl/base/attributes.h", "+absl/base/config.h", + "+absl/base/macros.h", "+absl/container/inlined_vector.h", "+absl/memory/memory.h", "+absl/meta/type_traits.h", diff --git a/abseil-in-webrtc.md b/abseil-in-webrtc.md index d09ff1dc42..74ceb6ff30 100644 --- a/abseil-in-webrtc.md +++ b/abseil-in-webrtc.md @@ -22,8 +22,9 @@ adds the first use. `absl::is_trivially_destructible` from `absl/meta/type_traits.h`. * `absl::variant` and related stuff from `absl/types/variant.h`. * The functions in `absl/algorithm/algorithm.h` and - `absl/algorithm/container.h` -* The macros in `absl/base/attributes.h` and `absl/base/config.h` + `absl/algorithm/container.h`. +* The macros in `absl/base/attributes.h`, `absl/base/config.h` and + `absl/base/macros.h`. ## **Disallowed** diff --git a/modules/include/module_common_types.h b/modules/include/module_common_types.h index 9295ce5db8..ab103567c4 100644 --- a/modules/include/module_common_types.h +++ b/modules/include/module_common_types.h @@ -100,7 +100,8 @@ class LossNotificationSender { virtual void SendLossNotification(uint16_t last_decoded_seq_num, uint16_t last_received_seq_num, - bool decodability_flag) = 0; + bool decodability_flag, + bool buffering_allowed) = 0; }; } // namespace webrtc diff --git a/modules/rtp_rtcp/BUILD.gn b/modules/rtp_rtcp/BUILD.gn index 02c7207279..60f7085386 100644 --- a/modules/rtp_rtcp/BUILD.gn +++ b/modules/rtp_rtcp/BUILD.gn @@ -472,6 +472,7 @@ if (rtc_include_tests) { "../../test:test_support", "../video_coding:codec_globals_headers", "//third_party/abseil-cpp/absl/algorithm:container", + "//third_party/abseil-cpp/absl/base:core_headers", "//third_party/abseil-cpp/absl/memory", "//third_party/abseil-cpp/absl/types:optional", ] diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index 1c74335c3f..e6cec61fbd 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -435,7 +435,8 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { // Returns -1 on failure else 0. virtual int32_t SendLossNotification(uint16_t last_decoded_seq_num, uint16_t last_received_seq_num, - bool decodability_flag) = 0; + bool decodability_flag, + bool buffering_allowed) = 0; }; } // namespace webrtc diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index 92601537bd..f82b1d6c90 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -151,10 +151,11 @@ class MockRtpRtcp : public RtpRtcp { MOCK_METHOD1(SetTargetSendBitrate, void(uint32_t bitrate_bps)); MOCK_METHOD1(SetKeyFrameRequestMethod, int32_t(KeyFrameRequestMethod method)); MOCK_METHOD0(RequestKeyFrame, int32_t()); - MOCK_METHOD3(SendLossNotification, + MOCK_METHOD4(SendLossNotification, int32_t(uint16_t last_decoded_seq_num, uint16_t last_received_seq_num, - bool decodability_flag)); + bool decodability_flag, + bool buffering_allowed)); MOCK_METHOD0(Process, void()); MOCK_METHOD1(RegisterSendChannelRtpStatisticsCallback, void(StreamDataCountersCallback*)); diff --git a/modules/rtp_rtcp/source/rtcp_sender.cc b/modules/rtp_rtcp/source/rtcp_sender.cc index f362fcb0db..19f1e26578 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/modules/rtp_rtcp/source/rtcp_sender.cc @@ -215,7 +215,8 @@ int32_t RTCPSender::SetSendingStatus(const FeedbackState& feedback_state, int32_t RTCPSender::SendLossNotification(const FeedbackState& feedback_state, uint16_t last_decoded_seq_num, uint16_t last_received_seq_num, - bool decodability_flag) { + bool decodability_flag, + bool buffering_allowed) { rtc::CritScope lock(&critical_section_rtcp_sender_); loss_notification_state_.last_decoded_seq_num = last_decoded_seq_num; @@ -224,7 +225,11 @@ int32_t RTCPSender::SendLossNotification(const FeedbackState& feedback_state, SetFlag(kRtcpLossNotification, /*is_volatile=*/true); - // Send immediately. + if (buffering_allowed) { + // The loss notification will be batched with additional feedback messages. + return 0; + } + return SendCompoundRTCP(feedback_state, {RTCPPacketType::kRtcpLossNotification}); } diff --git a/modules/rtp_rtcp/source/rtcp_sender.h b/modules/rtp_rtcp/source/rtcp_sender.h index 7a9aeacc72..74f4cc17a6 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.h +++ b/modules/rtp_rtcp/source/rtcp_sender.h @@ -117,7 +117,8 @@ class RTCPSender { int32_t SendLossNotification(const FeedbackState& feedback_state, uint16_t last_decoded_seq_num, uint16_t last_received_seq_num, - bool decodability_flag); + bool decodability_flag, + bool buffering_allowed); void SetRemb(int64_t bitrate_bps, std::vector ssrcs); diff --git a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index b6b3c34f51..0ddfb943ed 100644 --- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -10,6 +10,7 @@ #include +#include "absl/base/macros.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtcp_packet/bye.h" #include "modules/rtp_rtcp/source/rtcp_packet/common_header.h" @@ -366,28 +367,61 @@ TEST_F(RtcpSenderTest, SendPli) { TEST_F(RtcpSenderTest, SendNack) { rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); const uint16_t kList[] = {0, 1, 16}; - const int32_t kListLength = sizeof(kList) / sizeof(kList[0]); - EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpNack, kListLength, - kList)); + EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpNack, + ABSL_ARRAYSIZE(kList), kList)); EXPECT_EQ(1, parser()->nack()->num_packets()); EXPECT_EQ(kSenderSsrc, parser()->nack()->sender_ssrc()); EXPECT_EQ(kRemoteSsrc, parser()->nack()->media_ssrc()); EXPECT_THAT(parser()->nack()->packet_ids(), ElementsAre(0, 1, 16)); } -TEST_F(RtcpSenderTest, SendLossNotification) { +TEST_F(RtcpSenderTest, SendLossNotificationBufferingNotAllowed) { rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); constexpr uint16_t kLastDecoded = 0x1234; constexpr uint16_t kLastReceived = 0x4321; constexpr bool kDecodabilityFlag = true; - const int32_t result = rtcp_sender_->SendLossNotification( - feedback_state(), kLastDecoded, kLastReceived, kDecodabilityFlag); - EXPECT_EQ(result, 0); - EXPECT_EQ(1, parser()->loss_notification()->num_packets()); + constexpr bool kBufferingAllowed = false; + EXPECT_EQ(rtcp_sender_->SendLossNotification(feedback_state(), kLastDecoded, + kLastReceived, kDecodabilityFlag, + kBufferingAllowed), + 0); + EXPECT_EQ(parser()->processed_rtcp_packets(), 1u); + EXPECT_EQ(parser()->loss_notification()->num_packets(), 1); EXPECT_EQ(kSenderSsrc, parser()->loss_notification()->sender_ssrc()); EXPECT_EQ(kRemoteSsrc, parser()->loss_notification()->media_ssrc()); } +TEST_F(RtcpSenderTest, SendLossNotificationBufferingAllowed) { + rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); + constexpr uint16_t kLastDecoded = 0x1234; + constexpr uint16_t kLastReceived = 0x4321; + constexpr bool kDecodabilityFlag = true; + constexpr bool kBufferingAllowed = true; + EXPECT_EQ(rtcp_sender_->SendLossNotification(feedback_state(), kLastDecoded, + kLastReceived, kDecodabilityFlag, + kBufferingAllowed), + 0); + + // No RTCP messages sent yet. + ASSERT_EQ(parser()->processed_rtcp_packets(), 0u); + + // Sending another messages triggers sending the LNTF messages as well. + const uint16_t kList[] = {0, 1, 16}; + EXPECT_EQ(rtcp_sender_->SendRTCP(feedback_state(), kRtcpNack, + ABSL_ARRAYSIZE(kList), kList), + 0); + + // Exactly one packet was produced, and it contained both the buffered LNTF + // as well as the message that had triggered the packet. + EXPECT_EQ(parser()->processed_rtcp_packets(), 1u); + EXPECT_EQ(parser()->loss_notification()->num_packets(), 1); + EXPECT_EQ(parser()->loss_notification()->sender_ssrc(), kSenderSsrc); + EXPECT_EQ(parser()->loss_notification()->media_ssrc(), kRemoteSsrc); + EXPECT_EQ(parser()->nack()->num_packets(), 1); + EXPECT_EQ(parser()->nack()->sender_ssrc(), kSenderSsrc); + EXPECT_EQ(parser()->nack()->media_ssrc(), kRemoteSsrc); +} + TEST_F(RtcpSenderTest, RembNotIncludedBeforeSet) { rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 843f616b89..ad83c9e0a5 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -754,10 +754,11 @@ int32_t ModuleRtpRtcpImpl::RequestKeyFrame() { int32_t ModuleRtpRtcpImpl::SendLossNotification(uint16_t last_decoded_seq_num, uint16_t last_received_seq_num, - bool decodability_flag) { + bool decodability_flag, + bool buffering_allowed) { return rtcp_sender_.SendLossNotification( GetFeedbackState(), last_decoded_seq_num, last_received_seq_num, - decodability_flag); + decodability_flag, buffering_allowed); } void ModuleRtpRtcpImpl::SetRemoteSSRC(const uint32_t ssrc) { diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 4cb274b9dd..15bedc73b9 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -263,7 +263,8 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { int32_t SendLossNotification(uint16_t last_decoded_seq_num, uint16_t last_received_seq_num, - bool decodability_flag) override; + bool decodability_flag, + bool buffering_allowed) override; bool LastReceivedNTP(uint32_t* NTPsecs, uint32_t* NTPfrac, diff --git a/modules/video_coding/loss_notification_controller.cc b/modules/video_coding/loss_notification_controller.cc index 9270ca4c20..6389fd0454 100644 --- a/modules/video_coding/loss_notification_controller.cc +++ b/modules/video_coding/loss_notification_controller.cc @@ -181,7 +181,7 @@ void LossNotificationController::HandleLoss(uint16_t last_received_seq_num, last_decodable_non_discardable_->first_seq_num)); loss_notification_sender_->SendLossNotification( last_decodable_non_discardable_->first_seq_num, last_received_seq_num, - decodability_flag); + decodability_flag, /*buffering_allowed=*/true); } else { key_frame_request_sender_->RequestKeyFrame(); } diff --git a/modules/video_coding/loss_notification_controller_unittest.cc b/modules/video_coding/loss_notification_controller_unittest.cc index 2ba12fef74..590cc7716d 100644 --- a/modules/video_coding/loss_notification_controller_unittest.cc +++ b/modules/video_coding/loss_notification_controller_unittest.cc @@ -95,7 +95,9 @@ class LossNotificationControllerBaseTest : public ::testing::Test, // LossNotificationSender implementation. void SendLossNotification(uint16_t last_decoded_seq_num, uint16_t last_received_seq_num, - bool decodability_flag) override { + bool decodability_flag, + bool buffering_allowed) override { + EXPECT_TRUE(buffering_allowed); // (Flag useful elsewhere.) EXPECT_FALSE(LastKeyFrameRequest()); EXPECT_FALSE(LastLossNotification()); last_loss_notification_.emplace(last_decoded_seq_num, last_received_seq_num, diff --git a/test/rtcp_packet_parser.cc b/test/rtcp_packet_parser.cc index 984693cbab..94f500fe57 100644 --- a/test/rtcp_packet_parser.cc +++ b/test/rtcp_packet_parser.cc @@ -22,8 +22,11 @@ RtcpPacketParser::RtcpPacketParser() = default; RtcpPacketParser::~RtcpPacketParser() = default; bool RtcpPacketParser::Parse(const void* data, size_t length) { + ++processed_rtcp_packets_; + const uint8_t* const buffer = static_cast(data); const uint8_t* const buffer_end = buffer + length; + rtcp::CommonHeader header; for (const uint8_t* next_packet = buffer; next_packet != buffer_end; next_packet = header.NextPacket()) { diff --git a/test/rtcp_packet_parser.h b/test/rtcp_packet_parser.h index 809509dc08..4916195822 100644 --- a/test/rtcp_packet_parser.h +++ b/test/rtcp_packet_parser.h @@ -105,6 +105,7 @@ class RtcpPacketParser { return &transport_feedback_; } uint32_t sender_ssrc() const { return sender_ssrc_; } + size_t processed_rtcp_packets() const { return processed_rtcp_packets_; } private: PacketCounter app_; @@ -124,6 +125,7 @@ class RtcpPacketParser { PacketCounter tmmbr_; PacketCounter transport_feedback_; uint32_t sender_ssrc_ = 0; + size_t processed_rtcp_packets_ = 0; }; } // namespace test diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index 3a741e0eeb..c62c0296d7 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -116,7 +116,9 @@ void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendNack( void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendLossNotification( uint16_t last_decoded_seq_num, uint16_t last_received_seq_num, - bool decodability_flag) { + bool decodability_flag, + bool buffering_allowed) { + RTC_DCHECK(buffering_allowed); rtc::CritScope lock(&cs_); RTC_DCHECK(lntf_state_) << "SendLossNotification() called twice in a row with no call to " @@ -125,8 +127,6 @@ void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendLossNotification( last_decoded_seq_num, last_received_seq_num, decodability_flag); } -// TODO(bugs.webrtc.org/10336): Make SendBufferedRtcpFeedback() actually -// set everything, then send it all together. void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendBufferedRtcpFeedback() { bool request_key_frame = false; std::vector nack_sequence_numbers; @@ -139,17 +139,24 @@ void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendBufferedRtcpFeedback() { std::swap(lntf_state, lntf_state_); } + if (lntf_state) { + // If either a NACK or a key frame request is sent, we should buffer + // the LNTF and wait for them (NACK or key frame request) to trigger + // the compound feedback message. + // Otherwise, the LNTF should be sent out immediately. + const bool buffering_allowed = + request_key_frame || !nack_sequence_numbers.empty(); + + loss_notification_sender_->SendLossNotification( + lntf_state->last_decoded_seq_num, lntf_state->last_received_seq_num, + lntf_state->decodability_flag, buffering_allowed); + } + if (request_key_frame) { key_frame_request_sender_->RequestKeyFrame(); } else if (!nack_sequence_numbers.empty()) { nack_sender_->SendNack(nack_sequence_numbers, true); } - - if (lntf_state) { - loss_notification_sender_->SendLossNotification( - lntf_state->last_decoded_seq_num, lntf_state->last_received_seq_num, - lntf_state->decodability_flag); - } } RtpVideoStreamReceiver::RtpVideoStreamReceiver( @@ -478,10 +485,11 @@ void RtpVideoStreamReceiver::RequestKeyFrame() { void RtpVideoStreamReceiver::SendLossNotification( uint16_t last_decoded_seq_num, uint16_t last_received_seq_num, - bool decodability_flag) { + bool decodability_flag, + bool buffering_allowed) { RTC_DCHECK(config_.rtp.lntf.enabled); rtp_rtcp_->SendLossNotification(last_decoded_seq_num, last_received_seq_num, - decodability_flag); + decodability_flag, buffering_allowed); } bool RtpVideoStreamReceiver::IsUlpfecEnabled() const { diff --git a/video/rtp_video_stream_receiver.h b/video/rtp_video_stream_receiver.h index db73bab034..d574184eb6 100644 --- a/video/rtp_video_stream_receiver.h +++ b/video/rtp_video_stream_receiver.h @@ -127,7 +127,8 @@ class RtpVideoStreamReceiver : public LossNotificationSender, // Implements LossNotificationSender. void SendLossNotification(uint16_t last_decoded_seq_num, uint16_t last_received_seq_num, - bool decodability_flag) override; + bool decodability_flag, + bool buffering_allowed) override; bool IsUlpfecEnabled() const; bool IsRetransmissionsEnabled() const; @@ -204,7 +205,8 @@ class RtpVideoStreamReceiver : public LossNotificationSender, // LossNotificationSender implementation. void SendLossNotification(uint16_t last_decoded_seq_num, uint16_t last_received_seq_num, - bool decodability_flag) override; + bool decodability_flag, + bool buffering_allowed) override; // Send all RTCP feedback messages buffered thus far. void SendBufferedRtcpFeedback();