diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc index 7113807cf8..d4c1cd1e1f 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc @@ -12,6 +12,8 @@ #include // memcpy +#include + #include "webrtc/base/checks.h" #include "webrtc/base/constructormagic.h" #include "webrtc/base/logging.h" @@ -435,6 +437,8 @@ bool RTCPSender::TimeToSendRTCPReport(bool sendKeyframeBeforeRTP) const { } std::unique_ptr RTCPSender::BuildSR(const RtcpContext& ctx) { + // Timestamp shouldn't be estimated before first media frame. + RTC_DCHECK_GE(last_frame_capture_time_ms_, 0); // The timestamp of this RTCP packet should be estimated as the timestamp of // the frame being captured at this moment. We are calculating that // timestamp as the last frame's timestamp + the time since the last frame @@ -766,6 +770,28 @@ int32_t RTCPSender::SendCompoundRTCP( LOG(LS_WARNING) << "Can't send rtcp if it is disabled."; return -1; } + // Add all flags as volatile. Non volatile entries will not be overwritten. + // All new volatile flags added will be consumed by the end of this call. + SetFlags(packet_types, true); + + // Prevent sending streams to send SR before any media has been sent. + const bool can_calculate_rtp_timestamp = (last_frame_capture_time_ms_ >= 0); + if (!can_calculate_rtp_timestamp) { + bool consumed_sr_flag = ConsumeFlag(kRtcpSr); + bool consumed_report_flag = sending_ && ConsumeFlag(kRtcpReport); + bool sender_report = consumed_report_flag || consumed_sr_flag; + if (sender_report && AllVolatileFlagsConsumed()) { + // This call was for Sender Report and nothing else. + return 0; + } + if (sending_ && method_ == RtcpMode::kCompound) { + // Not allowed to send any RTCP packet without sender report. + return -1; + } + } + + if (packet_type_counter_.first_packet_time_ms == -1) + packet_type_counter_.first_packet_time_ms = clock_->TimeInMilliseconds(); // We need to send our NTP even if we haven't received any reports. uint32_t ntp_sec; @@ -774,7 +800,7 @@ int32_t RTCPSender::SendCompoundRTCP( RtcpContext context(feedback_state, nack_size, nack_list, repeat, pictureID, ntp_sec, ntp_frac); - PrepareReport(packet_types, feedback_state); + PrepareReport(feedback_state); std::unique_ptr packet_bye; @@ -818,15 +844,7 @@ int32_t RTCPSender::SendCompoundRTCP( return bytes_sent == 0 ? -1 : 0; } -void RTCPSender::PrepareReport(const std::set& packetTypes, - const FeedbackState& feedback_state) { - // Add all flags as volatile. Non volatile entries will not be overwritten - // and all new volatile flags added will be consumed by the end of this call. - SetFlags(packetTypes, true); - - if (packet_type_counter_.first_packet_time_ms == -1) - packet_type_counter_.first_packet_time_ms = clock_->TimeInMilliseconds(); - +void RTCPSender::PrepareReport(const FeedbackState& feedback_state) { bool generate_report; if (IsFlagPresent(kRtcpSr) || IsFlagPresent(kRtcpRr)) { // Report type already explicitly set, don't automatically populate. diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.h b/webrtc/modules/rtp_rtcp/source/rtcp_sender.h index 6f94de564b..58f19b0918 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.h @@ -156,8 +156,7 @@ class RTCPSender { class RtcpContext; // Determine which RTCP messages should be sent and setup flags. - void PrepareReport(const std::set& packetTypes, - const FeedbackState& feedback_state) + void PrepareReport(const FeedbackState& feedback_state) EXCLUSIVE_LOCKS_REQUIRED(critical_section_rtcp_sender_); bool AddReportBlock(const FeedbackState& feedback_state, diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index a4d6e59c8f..ec8330820c 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -221,6 +221,8 @@ class TestTransport : public Transport, namespace { static const uint32_t kSenderSsrc = 0x11111111; static const uint32_t kRemoteSsrc = 0x22222222; +static const uint32_t kStartRtpTimestamp = 0x34567; +static const uint32_t kRtpTimestamp = 0x45678; } class RtcpSenderTest : public ::testing::Test { @@ -238,6 +240,8 @@ class RtcpSenderTest : public ::testing::Test { nullptr, nullptr, &test_transport_)); rtcp_sender_->SetSSRC(kSenderSsrc); rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); + rtcp_sender_->SetStartTimestamp(kStartRtpTimestamp); + rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds()); } void InsertIncomingPacket(uint32_t remote_ssrc, uint16_t seq_num) { @@ -285,6 +289,7 @@ TEST_F(RtcpSenderTest, SendSr) { const uint32_t kOctetCount = 0x23456; rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); RTCPSender::FeedbackState feedback_state = rtp_rtcp_impl_->GetFeedbackState(); + rtcp_sender_->SetSendingStatus(feedback_state, true); feedback_state.packets_sent = kPacketCount; feedback_state.media_bytes_sent = kOctetCount; uint32_t ntp_secs; @@ -297,9 +302,43 @@ TEST_F(RtcpSenderTest, SendSr) { EXPECT_EQ(ntp_frac, parser()->sender_report()->NtpFrac()); EXPECT_EQ(kPacketCount, parser()->sender_report()->PacketCount()); EXPECT_EQ(kOctetCount, parser()->sender_report()->OctetCount()); + EXPECT_EQ(kStartRtpTimestamp + kRtpTimestamp, + parser()->sender_report()->RtpTimestamp()); EXPECT_EQ(0, parser()->report_block()->num_packets()); } +TEST_F(RtcpSenderTest, DoNotSendSrBeforeRtp) { + rtcp_sender_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(), + nullptr, nullptr, &test_transport_)); + rtcp_sender_->SetSSRC(kSenderSsrc); + rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); + rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); + rtcp_sender_->SetSendingStatus(feedback_state(), true); + + // Sender Report shouldn't be send as an SR nor as a Report. + rtcp_sender_->SendRTCP(feedback_state(), kRtcpSr); + EXPECT_EQ(0, parser()->sender_report()->num_packets()); + rtcp_sender_->SendRTCP(feedback_state(), kRtcpReport); + EXPECT_EQ(0, parser()->sender_report()->num_packets()); + // Other packets (e.g. Pli) are allowed, even if useless. + EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpPli)); + EXPECT_EQ(1, parser()->pli()->num_packets()); +} + +TEST_F(RtcpSenderTest, DoNotSendCompundBeforeRtp) { + rtcp_sender_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(), + nullptr, nullptr, &test_transport_)); + rtcp_sender_->SetSSRC(kSenderSsrc); + rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); + rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound); + rtcp_sender_->SetSendingStatus(feedback_state(), true); + + // In compound mode no packets are allowed (e.g. Pli) because compound mode + // should start with Sender Report. + EXPECT_EQ(-1, rtcp_sender_->SendRTCP(feedback_state(), kRtcpPli)); + EXPECT_EQ(0, parser()->pli()->num_packets()); +} + TEST_F(RtcpSenderTest, SendRr) { rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpRr)); @@ -688,6 +727,7 @@ TEST_F(RtcpSenderTest, TmmbrIncludedInCompoundPacketIfEnabled) { TEST_F(RtcpSenderTest, SendTmmbn) { rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound); + rtcp_sender_->SetSendingStatus(feedback_state(), true); std::vector bounding_set; const uint32_t kBitrateKbps = 32768; const uint32_t kPacketOh = 40; @@ -714,6 +754,7 @@ TEST_F(RtcpSenderTest, SendTmmbn) { // situation where this caused confusion. TEST_F(RtcpSenderTest, SendsTmmbnIfSetAndEmpty) { rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound); + rtcp_sender_->SetSendingStatus(feedback_state(), true); std::vector bounding_set; rtcp_sender_->SetTMMBN(&bounding_set); EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpSr)); @@ -768,6 +809,8 @@ TEST_F(RtcpSenderTest, ByeMustBeLast) { nullptr, nullptr, &mock_transport)); rtcp_sender_->SetSSRC(kSenderSsrc); rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); + rtcp_sender_->SetStartTimestamp(kStartRtpTimestamp); + rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds()); // Set up XR VoIP metric to be included with BYE rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound); diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index dbd919d056..40e73ebd0e 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -207,13 +207,8 @@ void ModuleRtpRtcpImpl::Process() { } } - // For sending streams, make sure to not send a SR before media has been sent. - if (rtcp_sender_.TimeToSendRTCPReport()) { - RTCPSender::FeedbackState state = GetFeedbackState(); - // Prevent sending streams to send SR before any media has been sent. - if (!rtcp_sender_.Sending() || state.packets_sent > 0) - rtcp_sender_.SendRTCP(state, kRtcpReport); - } + if (rtcp_sender_.TimeToSendRTCPReport()) + rtcp_sender_.SendRTCP(GetFeedbackState(), kRtcpReport); if (UpdateRTCPReceiveInformationTimers()) { // A receiver has timed out diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc index 1e2cc61fca..9dfcc13a0e 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc @@ -297,7 +297,9 @@ TEST_F(RtpRtcpImplTest, Rtt) { header.headerLength = 12; receiver_.receive_statistics_->IncomingPacket(header, 100, false); - // Sender module should send a SR. + // Send Frame before sending an SR. + SendFrame(&sender_, kBaseLayerTid); + // Sender module should send an SR. EXPECT_EQ(0, sender_.impl_->SendRTCP(kRtcpReport)); // Receiver module should send a RR with a response to the last received SR. @@ -343,6 +345,8 @@ TEST_F(RtpRtcpImplTest, RttForReceiverOnly) { // Sender module should send a response to the last received RTRR (DLRR). clock_.AdvanceTimeMilliseconds(1000); + // Send Frame before sending a SR. + SendFrame(&sender_, kBaseLayerTid); EXPECT_EQ(0, sender_.impl_->SendRTCP(kRtcpReport)); // Verify RTT. @@ -462,6 +466,8 @@ TEST_F(RtpRtcpImplTest, SendsInitialNackList) { const uint16_t kNackLength = 1; uint16_t nack_list[kNackLength] = {123}; EXPECT_EQ(0U, sender_.RtcpSent().nack_packets); + // Send Frame before sending a compound RTCP that starts with SR. + SendFrame(&sender_, kBaseLayerTid); EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength)); EXPECT_EQ(1U, sender_.RtcpSent().nack_packets); EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123)); @@ -472,6 +478,8 @@ TEST_F(RtpRtcpImplTest, SendsExtendedNackList) { const uint16_t kNackLength = 1; uint16_t nack_list[kNackLength] = {123}; EXPECT_EQ(0U, sender_.RtcpSent().nack_packets); + // Send Frame before sending a compound RTCP that starts with SR. + SendFrame(&sender_, kBaseLayerTid); EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength)); EXPECT_EQ(1U, sender_.RtcpSent().nack_packets); EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123)); @@ -495,6 +503,8 @@ TEST_F(RtpRtcpImplTest, ReSendsNackListAfterRttMs) { const uint16_t kNackLength = 2; uint16_t nack_list[kNackLength] = {123, 125}; EXPECT_EQ(0U, sender_.RtcpSent().nack_packets); + // Send Frame before sending a compound RTCP that starts with SR. + SendFrame(&sender_, kBaseLayerTid); EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength)); EXPECT_EQ(1U, sender_.RtcpSent().nack_packets); EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123, 125)); diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index 21a6654a83..1f4ed77ffa 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -25,6 +25,7 @@ #include "webrtc/modules/include/module_common_types.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h" #include "webrtc/modules/rtp_rtcp/source/byte_io.h" +#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/rapid_resync_request.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_utility.h" #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h" #include "webrtc/modules/video_coding/codecs/h264/include/h264.h" @@ -111,7 +112,7 @@ class EndToEndTest : public test::CallTest { void RespectsRtcpMode(RtcpMode rtcp_mode); void TestXrReceiverReferenceTimeReport(bool enable_rrtr); void TestSendsSetSsrcs(size_t num_ssrcs, bool send_single_ssrc_first); - void TestRtpStatePreservation(bool use_rtx); + void TestRtpStatePreservation(bool use_rtx, bool provoke_rtcpsr_before_rtp); void VerifyHistogramStats(bool use_rtx, bool use_red, bool screenshare); void VerifyNewVideoSendStreamsRespectNetworkState( MediaType network_to_bring_down, @@ -2965,7 +2966,8 @@ TEST_F(EndToEndTest, DISABLED_RedundantPayloadsTransmittedOnAllSsrcs) { RunBaseTest(&test); } -void EndToEndTest::TestRtpStatePreservation(bool use_rtx) { +void EndToEndTest::TestRtpStatePreservation(bool use_rtx, + bool provoke_rtcpsr_before_rtp) { class RtpSequenceObserver : public test::RtpRtcpObserver { public: explicit RtpSequenceObserver(bool use_rtx) @@ -2985,6 +2987,28 @@ void EndToEndTest::TestRtpStatePreservation(bool use_rtx) { } private: + void ValidateTimestampGap(uint32_t ssrc, + uint32_t timestamp, + bool only_padding) + EXCLUSIVE_LOCKS_REQUIRED(crit_) { + static const int32_t kMaxTimestampGap = kDefaultTimeoutMs * 90; + auto timestamp_it = last_observed_timestamp_.find(ssrc); + if (timestamp_it == last_observed_timestamp_.end()) { + EXPECT_FALSE(only_padding); + last_observed_timestamp_[ssrc] = timestamp; + } else { + // Verify timestamps are reasonably close. + uint32_t latest_observed = timestamp_it->second; + // Wraparound handling is unnecessary here as long as an int variable + // is used to store the result. + int32_t timestamp_gap = timestamp - latest_observed; + EXPECT_LE(std::abs(timestamp_gap), kMaxTimestampGap) + << "Gap in timestamps (" << latest_observed << " -> " << timestamp + << ") too large for SSRC: " << ssrc << "."; + timestamp_it->second = timestamp; + } + } + Action OnSendRtp(const uint8_t* packet, size_t length) override { RTPHeader header; EXPECT_TRUE(parser_->Parse(packet, length, &header)); @@ -3021,24 +3045,9 @@ void EndToEndTest::TestRtpStatePreservation(bool use_rtx) { } } - static const int32_t kMaxTimestampGap = kDefaultTimeoutMs * 90; - auto timestamp_it = last_observed_timestamp_.find(ssrc); - if (timestamp_it == last_observed_timestamp_.end()) { - EXPECT_FALSE(only_padding); - last_observed_timestamp_[ssrc] = timestamp; - } else { - // Verify timestamps are reasonably close. - uint32_t latest_observed = timestamp_it->second; - // Wraparound handling is unnecessary here as long as an int variable - // is used to store the result. - int32_t timestamp_gap = timestamp - latest_observed; - EXPECT_LE(std::abs(timestamp_gap), kMaxTimestampGap) - << "Gap in timestamps (" << latest_observed << " -> " - << timestamp << ") too large for SSRC: " << ssrc << "."; - timestamp_it->second = timestamp; - } - rtc::CritScope lock(&crit_); + ValidateTimestampGap(ssrc, timestamp, only_padding); + // Wait for media packets on all ssrcs. if (!ssrc_observed_[ssrc] && !only_padding) { ssrc_observed_[ssrc] = true; @@ -3049,6 +3058,19 @@ void EndToEndTest::TestRtpStatePreservation(bool use_rtx) { return SEND_PACKET; } + Action OnSendRtcp(const uint8_t* packet, size_t length) override { + test::RtcpPacketParser rtcp_parser; + rtcp_parser.Parse(packet, length); + if (rtcp_parser.sender_report()->num_packets() > 0) { + uint32_t ssrc = rtcp_parser.sender_report()->Ssrc(); + uint32_t rtcp_timestamp = rtcp_parser.sender_report()->RtpTimestamp(); + + rtc::CritScope lock(&crit_); + ValidateTimestampGap(ssrc, rtcp_timestamp, false); + } + return SEND_PACKET; + } + SequenceNumberUnwrapper seq_numbers_unwrapper_; std::map> last_observed_seq_numbers_; std::map last_observed_timestamp_; @@ -3118,6 +3140,15 @@ void EndToEndTest::TestRtpStatePreservation(bool use_rtx) { video_send_stream_ = sender_call_->CreateVideoSendStream(video_send_config_, one_stream); video_send_stream_->Start(); + if (provoke_rtcpsr_before_rtp) { + // Rapid Resync Request forces sending RTCP Sender Report back. + // Using this request speeds up this test because then there is no need + // to wait for a second for periodic Sender Report. + rtcp::RapidResyncRequest force_send_sr_back_request; + rtc::Buffer packet = force_send_sr_back_request.Build(); + static_cast(receive_transport) + .SendRtcp(packet.data(), packet.size()); + } CreateFrameGeneratorCapturer(); frame_generator_capturer_->Start(); @@ -3150,13 +3181,18 @@ void EndToEndTest::TestRtpStatePreservation(bool use_rtx) { } TEST_F(EndToEndTest, RestartingSendStreamPreservesRtpState) { - TestRtpStatePreservation(false); + TestRtpStatePreservation(false, false); } -// This test is flaky. See: +// These tests are flaky. See: // https://bugs.chromium.org/p/webrtc/issues/detail?id=4332 TEST_F(EndToEndTest, DISABLED_RestartingSendStreamPreservesRtpStatesWithRtx) { - TestRtpStatePreservation(true); + TestRtpStatePreservation(true, false); +} + +TEST_F(EndToEndTest, + DISABLED_RestartingSendStreamKeepsRtpAndRtcpTimestampsSynced) { + TestRtpStatePreservation(true, true); } TEST_F(EndToEndTest, RespectsNetworkState) {