diff --git a/modules/rtp_rtcp/source/rtcp_transceiver.cc b/modules/rtp_rtcp/source/rtcp_transceiver.cc index 7b1790de7b..2060b0b5e0 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver.cc +++ b/modules/rtp_rtcp/source/rtcp_transceiver.cc @@ -133,10 +133,16 @@ void RtcpTransceiver::SendPictureLossIndication(uint32_t ssrc) { } void RtcpTransceiver::SendFullIntraRequest(std::vector ssrcs) { + return SendFullIntraRequest(std::move(ssrcs), true); +} + +void RtcpTransceiver::SendFullIntraRequest(std::vector 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 df66b4cf8e..8bdb0bf913 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver.h +++ b/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 ssrcs); + // If new_request is true then requested sequence no. will increase for each + // requested ssrc. + void SendFullIntraRequest(std::vector 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 977fc8b7b7..6a73a476c2 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::SendPictureLossIndication(uint32_t ssrc) { } void RtcpTransceiverImpl::SendFullIntraRequest( - rtc::ArrayView ssrcs) { + rtc::ArrayView 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 8039f2b70f..6a6454662c 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_impl.h +++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl.h @@ -61,7 +61,10 @@ class RtcpTransceiverImpl { void SendNack(uint32_t ssrc, std::vector sequence_numbers); void SendPictureLossIndication(uint32_t ssrc); - void SendFullIntraRequest(rtc::ArrayView ssrcs); + // If new_request is true then requested sequence no. will increase for each + // requested ssrc. + void SendFullIntraRequest(rtc::ArrayView 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 ebfb068f7e..7d3f092042 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 @@ 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); diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc index 3bd534ca9b..cd35cfb1da 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc +++ b/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