Add new_request flag to SendFullIntraRequest
This allows one to request the same sequence number again
in the case of resending an FIR to the a sender before the
sender has time to send a key-frame.
Bug: webrtc:11171
Change-Id: Idd8e8120ccbcc194cefb8d0cf3f7cc64e7f76aa5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/161236
Commit-Queue: Evan Shrubsole <eshr@google.com>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30006}
diff --git a/modules/rtp_rtcp/source/rtcp_transceiver.cc b/modules/rtp_rtcp/source/rtcp_transceiver.cc
index 7b1790d..2060b0b 100644
--- a/modules/rtp_rtcp/source/rtcp_transceiver.cc
+++ b/modules/rtp_rtcp/source/rtcp_transceiver.cc
@@ -133,10 +133,16 @@
}
void RtcpTransceiver::SendFullIntraRequest(std::vector<uint32_t> ssrcs) {
+ return SendFullIntraRequest(std::move(ssrcs), true);
+}
+
+void RtcpTransceiver::SendFullIntraRequest(std::vector<uint32_t> ssrcs,
+ bool new_request) {
RTC_CHECK(rtcp_transceiver_);
RtcpTransceiverImpl* ptr = rtcp_transceiver_.get();
- task_queue_->PostTask(
- [ptr, ssrcs = std::move(ssrcs)] { ptr->SendFullIntraRequest(ssrcs); });
+ task_queue_->PostTask([ptr, ssrcs = std::move(ssrcs), new_request] {
+ ptr->SendFullIntraRequest(ssrcs, new_request);
+ });
}
} // namespace webrtc
diff --git a/modules/rtp_rtcp/source/rtcp_transceiver.h b/modules/rtp_rtcp/source/rtcp_transceiver.h
index df66b4c..8bdb0bf 100644
--- a/modules/rtp_rtcp/source/rtcp_transceiver.h
+++ b/modules/rtp_rtcp/source/rtcp_transceiver.h
@@ -85,7 +85,11 @@
// using PLI, https://tools.ietf.org/html/rfc4585#section-6.3.1.1
void SendPictureLossIndication(uint32_t ssrc);
// using FIR, https://tools.ietf.org/html/rfc5104#section-4.3.1.2
+ // Use the SendFullIntraRequest(ssrcs, true) instead.
void SendFullIntraRequest(std::vector<uint32_t> ssrcs);
+ // If new_request is true then requested sequence no. will increase for each
+ // requested ssrc.
+ void SendFullIntraRequest(std::vector<uint32_t> ssrcs, bool new_request);
private:
rtc::TaskQueue* const task_queue_;
diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc b/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc
index 977fc8b..6a73a47 100644
--- a/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc
+++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc
@@ -200,15 +200,19 @@
}
void RtcpTransceiverImpl::SendFullIntraRequest(
- rtc::ArrayView<const uint32_t> ssrcs) {
+ rtc::ArrayView<const uint32_t> ssrcs,
+ bool new_request) {
RTC_DCHECK(!ssrcs.empty());
if (!ready_to_send_)
return;
rtcp::Fir fir;
fir.SetSenderSsrc(config_.feedback_ssrc);
- for (uint32_t media_ssrc : ssrcs)
- fir.AddRequestTo(media_ssrc,
- remote_senders_[media_ssrc].fir_sequence_number++);
+ for (uint32_t media_ssrc : ssrcs) {
+ uint8_t& command_seq_num = remote_senders_[media_ssrc].fir_sequence_number;
+ if (new_request)
+ command_seq_num += 1;
+ fir.AddRequestTo(media_ssrc, command_seq_num);
+ }
SendImmediateFeedback(fir);
}
diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl.h b/modules/rtp_rtcp/source/rtcp_transceiver_impl.h
index 8039f2b..6a64546 100644
--- a/modules/rtp_rtcp/source/rtcp_transceiver_impl.h
+++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl.h
@@ -61,7 +61,10 @@
void SendNack(uint32_t ssrc, std::vector<uint16_t> sequence_numbers);
void SendPictureLossIndication(uint32_t ssrc);
- void SendFullIntraRequest(rtc::ArrayView<const uint32_t> ssrcs);
+ // If new_request is true then requested sequence no. will increase for each
+ // requested ssrc.
+ void SendFullIntraRequest(rtc::ArrayView<const uint32_t> ssrcs,
+ bool new_request);
// SendCombinedRtcpPacket ignores rtcp mode and does not send a compound
// message. https://tools.ietf.org/html/rfc4585#section-3.1
diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc b/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc
index ebfb068..7d3f092 100644
--- a/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc
+++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc
@@ -292,7 +292,7 @@
rtcp_transceiver.SendRawPacket(raw);
rtcp_transceiver.SendNack(ssrcs[0], sequence_numbers);
rtcp_transceiver.SendPictureLossIndication(ssrcs[0]);
- rtcp_transceiver.SendFullIntraRequest(ssrcs);
+ rtcp_transceiver.SendFullIntraRequest(ssrcs, true);
}
TEST(RtcpTransceiverImplTest, SendsRtcpWhenNetworkStateIsUp) {
@@ -313,7 +313,7 @@
rtcp_transceiver.SendRawPacket(raw);
rtcp_transceiver.SendNack(ssrcs[0], sequence_numbers);
rtcp_transceiver.SendPictureLossIndication(ssrcs[0]);
- rtcp_transceiver.SendFullIntraRequest(ssrcs);
+ rtcp_transceiver.SendFullIntraRequest(ssrcs, true);
}
TEST(RtcpTransceiverImplTest, SendsPeriodicRtcpWhenNetworkStateIsUp) {
@@ -805,7 +805,7 @@
config.outgoing_transport = &transport;
RtcpTransceiverImpl rtcp_transceiver(config);
- rtcp_transceiver.SendFullIntraRequest(kRemoteSsrcs);
+ rtcp_transceiver.SendFullIntraRequest(kRemoteSsrcs, true);
EXPECT_EQ(rtcp_parser.fir()->num_packets(), 1);
EXPECT_EQ(rtcp_parser.fir()->sender_ssrc(), kSenderSsrc);
@@ -824,18 +824,18 @@
const uint32_t kBothRemoteSsrcs[] = {4321, 5321};
const uint32_t kOneRemoteSsrc[] = {4321};
- rtcp_transceiver.SendFullIntraRequest(kBothRemoteSsrcs);
+ rtcp_transceiver.SendFullIntraRequest(kBothRemoteSsrcs, true);
ASSERT_EQ(rtcp_parser.fir()->requests()[0].ssrc, kBothRemoteSsrcs[0]);
uint8_t fir_sequence_number0 = rtcp_parser.fir()->requests()[0].seq_nr;
ASSERT_EQ(rtcp_parser.fir()->requests()[1].ssrc, kBothRemoteSsrcs[1]);
uint8_t fir_sequence_number1 = rtcp_parser.fir()->requests()[1].seq_nr;
- rtcp_transceiver.SendFullIntraRequest(kOneRemoteSsrc);
+ rtcp_transceiver.SendFullIntraRequest(kOneRemoteSsrc, true);
ASSERT_EQ(rtcp_parser.fir()->requests().size(), 1u);
ASSERT_EQ(rtcp_parser.fir()->requests()[0].ssrc, kBothRemoteSsrcs[0]);
EXPECT_EQ(rtcp_parser.fir()->requests()[0].seq_nr, fir_sequence_number0 + 1);
- rtcp_transceiver.SendFullIntraRequest(kBothRemoteSsrcs);
+ rtcp_transceiver.SendFullIntraRequest(kBothRemoteSsrcs, true);
ASSERT_EQ(rtcp_parser.fir()->requests().size(), 2u);
ASSERT_EQ(rtcp_parser.fir()->requests()[0].ssrc, kBothRemoteSsrcs[0]);
EXPECT_EQ(rtcp_parser.fir()->requests()[0].seq_nr, fir_sequence_number0 + 2);
@@ -843,6 +843,31 @@
EXPECT_EQ(rtcp_parser.fir()->requests()[1].seq_nr, fir_sequence_number1 + 1);
}
+TEST(RtcpTransceiverImplTest, SendFirDoesNotIncreaseSeqNoIfOldRequest) {
+ RtcpTransceiverConfig config;
+ config.schedule_periodic_compound_packets = false;
+ RtcpPacketParser rtcp_parser;
+ RtcpParserTransport transport(&rtcp_parser);
+ config.outgoing_transport = &transport;
+ RtcpTransceiverImpl rtcp_transceiver(config);
+
+ const uint32_t kBothRemoteSsrcs[] = {4321, 5321};
+
+ rtcp_transceiver.SendFullIntraRequest(kBothRemoteSsrcs, true);
+ ASSERT_EQ(rtcp_parser.fir()->requests().size(), 2u);
+ ASSERT_EQ(rtcp_parser.fir()->requests()[0].ssrc, kBothRemoteSsrcs[0]);
+ uint8_t fir_sequence_number0 = rtcp_parser.fir()->requests()[0].seq_nr;
+ ASSERT_EQ(rtcp_parser.fir()->requests()[1].ssrc, kBothRemoteSsrcs[1]);
+ uint8_t fir_sequence_number1 = rtcp_parser.fir()->requests()[1].seq_nr;
+
+ rtcp_transceiver.SendFullIntraRequest(kBothRemoteSsrcs, false);
+ ASSERT_EQ(rtcp_parser.fir()->requests().size(), 2u);
+ ASSERT_EQ(rtcp_parser.fir()->requests()[0].ssrc, kBothRemoteSsrcs[0]);
+ EXPECT_EQ(rtcp_parser.fir()->requests()[0].seq_nr, fir_sequence_number0);
+ ASSERT_EQ(rtcp_parser.fir()->requests()[1].ssrc, kBothRemoteSsrcs[1]);
+ EXPECT_EQ(rtcp_parser.fir()->requests()[1].seq_nr, fir_sequence_number1);
+}
+
TEST(RtcpTransceiverImplTest, KeyFrameRequestCreatesCompoundPacket) {
const uint32_t kRemoteSsrcs[] = {4321};
RtcpTransceiverConfig config;
@@ -855,7 +880,7 @@
config.rtcp_mode = webrtc::RtcpMode::kCompound;
RtcpTransceiverImpl rtcp_transceiver(config);
- rtcp_transceiver.SendFullIntraRequest(kRemoteSsrcs);
+ rtcp_transceiver.SendFullIntraRequest(kRemoteSsrcs, true);
// Test sent packet is compound by expecting presense of receiver report.
EXPECT_EQ(transport.num_packets(), 1);
@@ -874,7 +899,7 @@
config.rtcp_mode = webrtc::RtcpMode::kReducedSize;
RtcpTransceiverImpl rtcp_transceiver(config);
- rtcp_transceiver.SendFullIntraRequest(kRemoteSsrcs);
+ rtcp_transceiver.SendFullIntraRequest(kRemoteSsrcs, true);
// Test sent packet is reduced size by expecting absense of receiver report.
EXPECT_EQ(transport.num_packets(), 1);
diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc
index 3bd534c..cd35cfb 100644
--- a/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc
+++ b/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc
@@ -294,4 +294,43 @@
WaitPostedTasks(&queue);
}
+TEST(RtcpTransceiverTest, SendFrameIntraRequestDefaultsToNewRequest) {
+ 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);
+
+ uint8_t first_seq_nr;
+ 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.fir()->requests()[0].ssrc, kSenderSsrc);
+ first_seq_nr = rtcp_parser.fir()->requests()[0].seq_nr;
+ return true;
+ })
+ .WillOnce([&](const uint8_t* buffer, size_t size) {
+ EXPECT_TRUE(queue.IsCurrent());
+ RtcpPacketParser rtcp_parser;
+ rtcp_parser.Parse(buffer, size);
+ EXPECT_EQ(rtcp_parser.fir()->requests()[0].ssrc, kSenderSsrc);
+ EXPECT_EQ(rtcp_parser.fir()->requests()[0].seq_nr, first_seq_nr + 1);
+ return true;
+ });
+
+ // Send 2 FIR packets because the sequence numbers are incremented after,
+ // sending. One wouldn't be able to differentiate the new_request.
+ rtcp_transceiver.SendFullIntraRequest({kSenderSsrc});
+ rtcp_transceiver.SendFullIntraRequest({kSenderSsrc});
+
+ WaitPostedTasks(&queue);
+}
+
} // namespace
