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 <eladalon@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28142}
diff --git a/DEPS b/DEPS
index 7c952ac..1ce18e6 100644
--- a/DEPS
+++ b/DEPS
@@ -1633,6 +1633,7 @@
"+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 d09ff1d..74ceb6f 100644
--- a/abseil-in-webrtc.md
+++ b/abseil-in-webrtc.md
@@ -22,8 +22,9 @@
`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 9295ce5..ab10356 100644
--- a/modules/include/module_common_types.h
+++ b/modules/include/module_common_types.h
@@ -100,7 +100,8 @@
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 02c7207..60f7085 100644
--- a/modules/rtp_rtcp/BUILD.gn
+++ b/modules/rtp_rtcp/BUILD.gn
@@ -472,6 +472,7 @@
"../../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 1c74335..e6cec61 100644
--- a/modules/rtp_rtcp/include/rtp_rtcp.h
+++ b/modules/rtp_rtcp/include/rtp_rtcp.h
@@ -435,7 +435,8 @@
// 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 9260153..f82b1d6 100644
--- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h
+++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h
@@ -151,10 +151,11 @@
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 f362fcb..19f1e26 100644
--- a/modules/rtp_rtcp/source/rtcp_sender.cc
+++ b/modules/rtp_rtcp/source/rtcp_sender.cc
@@ -215,7 +215,8 @@
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 @@
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 7a9aeac..74f4cc1 100644
--- a/modules/rtp_rtcp/source/rtcp_sender.h
+++ b/modules/rtp_rtcp/source/rtcp_sender.h
@@ -117,7 +117,8 @@
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<uint32_t> ssrcs);
diff --git a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
index b6b3c34..0ddfb94 100644
--- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
+++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
@@ -10,6 +10,7 @@
#include <memory>
+#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, 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 843f616..ad83c9e 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::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 4cb274b..15bedc7 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h
@@ -263,7 +263,8 @@
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 9270ca4..6389fd0 100644
--- a/modules/video_coding/loss_notification_controller.cc
+++ b/modules/video_coding/loss_notification_controller.cc
@@ -181,7 +181,7 @@
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 2ba12fe..590cc77 100644
--- a/modules/video_coding/loss_notification_controller_unittest.cc
+++ b/modules/video_coding/loss_notification_controller_unittest.cc
@@ -95,7 +95,9 @@
// 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 984693c..94f500f 100644
--- a/test/rtcp_packet_parser.cc
+++ b/test/rtcp_packet_parser.cc
@@ -22,8 +22,11 @@
RtcpPacketParser::~RtcpPacketParser() = default;
bool RtcpPacketParser::Parse(const void* data, size_t length) {
+ ++processed_rtcp_packets_;
+
const uint8_t* const buffer = static_cast<const uint8_t*>(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 809509d..4916195 100644
--- a/test/rtcp_packet_parser.h
+++ b/test/rtcp_packet_parser.h
@@ -105,6 +105,7 @@
return &transport_feedback_;
}
uint32_t sender_ssrc() const { return sender_ssrc_; }
+ size_t processed_rtcp_packets() const { return processed_rtcp_packets_; }
private:
PacketCounter<rtcp::App> app_;
@@ -124,6 +125,7 @@
PacketCounter<rtcp::Tmmbr> tmmbr_;
PacketCounter<rtcp::TransportFeedback> 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 3a741e0..c62c029 100644
--- a/video/rtp_video_stream_receiver.cc
+++ b/video/rtp_video_stream_receiver.cc
@@ -116,7 +116,9 @@
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 @@
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<uint16_t> nack_sequence_numbers;
@@ -139,17 +139,24 @@
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::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 db73bab..d574184 100644
--- a/video/rtp_video_stream_receiver.h
+++ b/video/rtp_video_stream_receiver.h
@@ -127,7 +127,8 @@
// 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 @@
// 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();