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}

modules/rtp_rtcp/source/rtcp_transceiver.cc

@@ -133,10 +133,16 @@ void RtcpTransceiver::SendPictureLossIndication(uint32_t ssrc) {
 }
 
 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(
+  task_queue_->PostTask([ptr, ssrcs = std::move(ssrcs), new_request] {
-      [ptr, ssrcs = std::move(ssrcs)] { ptr->SendFullIntraRequest(ssrcs); });
+    ptr->SendFullIntraRequest(ssrcs, new_request);
+  });
 }
 
 }  // namespace webrtc

modules/rtp_rtcp/source/rtcp_transceiver.h

@@ -85,7 +85,11 @@ class RtcpTransceiver : public RtcpFeedbackSenderInterface {
   // 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_;

modules/rtp_rtcp/source/rtcp_transceiver_impl.cc

@@ -200,15 +200,19 @@ void RtcpTransceiverImpl::SendPictureLossIndication(uint32_t ssrc) {
 }
 
 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)
+  for (uint32_t media_ssrc : ssrcs) {
-    fir.AddRequestTo(media_ssrc,
+    uint8_t& command_seq_num = remote_senders_[media_ssrc].fir_sequence_number;
-                     remote_senders_[media_ssrc].fir_sequence_number++);
+    if (new_request)
+      command_seq_num += 1;
+    fir.AddRequestTo(media_ssrc, command_seq_num);
+  }
   SendImmediateFeedback(fir);
 }
 

modules/rtp_rtcp/source/rtcp_transceiver_impl.h

@@ -61,7 +61,10 @@ class RtcpTransceiverImpl {
   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

modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc

@@ -292,7 +292,7 @@ TEST(RtcpTransceiverImplTest, SendsNoRtcpWhenNetworkStateIsDown) {
   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 @@ TEST(RtcpTransceiverImplTest, SendsRtcpWhenNetworkStateIsUp) {
   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 @@ TEST(RtcpTransceiverImplTest, RequestKeyFrameWithFullIntraRequest) {
   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 @@ TEST(RtcpTransceiverImplTest, RequestKeyFrameWithFirIncreaseSeqNoPerSsrc) {
   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 @@ TEST(RtcpTransceiverImplTest, RequestKeyFrameWithFirIncreaseSeqNoPerSsrc) {
   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 @@ TEST(RtcpTransceiverImplTest, KeyFrameRequestCreatesCompoundPacket) {
   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 @@ TEST(RtcpTransceiverImplTest, KeyFrameRequestCreatesReducedSizePacket) {
   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);

modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc

@@ -294,4 +294,43 @@ TEST(RtcpTransceiverTest, SendsCombinedRtcpPacketOnTaskQueue) {
   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