diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc index 25016e053f..ad19bf0883 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc @@ -591,9 +591,9 @@ size_t RTPSender::SendPadData(size_t bytes, size_t padding_bytes_in_packet = std::min(MaxDataPayloadLength(), kMaxPaddingLength); size_t bytes_sent = 0; - bool using_transport_seq = rtp_header_extension_map_.IsRegistered( - kRtpExtensionTransportSequenceNumber) && - transport_sequence_number_allocator_; + bool using_transport_seq = + IsRtpHeaderExtensionRegistered(kRtpExtensionTransportSequenceNumber) && + transport_sequence_number_allocator_; for (; bytes > 0; bytes -= padding_bytes_in_packet) { if (bytes < padding_bytes_in_packet) bytes = padding_bytes_in_packet; @@ -647,9 +647,13 @@ size_t RTPSender::SendPadData(size_t bytes, } uint8_t padding_packet[IP_PACKET_SIZE]; - size_t header_length = - CreateRtpHeader(padding_packet, payload_type, ssrc, false, timestamp, - sequence_number, std::vector()); + size_t header_length = 0; + { + rtc::CritScope lock(&send_critsect_); + header_length = + CreateRtpHeader(padding_packet, payload_type, ssrc, false, timestamp, + sequence_number, std::vector()); + } BuildPaddingPacket(padding_packet, header_length, padding_bytes_in_packet); size_t length = padding_bytes_in_packet + header_length; int64_t now_ms = clock_->TimeInMilliseconds(); @@ -666,13 +670,11 @@ size_t RTPSender::SendPadData(size_t bytes, UpdateAbsoluteSendTime(padding_packet, length, rtp_header, now_ms); PacketOptions options; - if (AllocateTransportSequenceNumber(&options.packet_id)) { - if (UpdateTransportSequenceNumber(options.packet_id, padding_packet, - length, rtp_header)) { - if (transport_feedback_observer_) - transport_feedback_observer_->AddPacket(options.packet_id, length, - probe_cluster_id); - } + if (UpdateTransportSequenceNumber(padding_packet, length, rtp_header, + &options.packet_id)) { + if (transport_feedback_observer_) + transport_feedback_observer_->AddPacket(options.packet_id, length, + probe_cluster_id); } if (!SendPacketToNetwork(padding_packet, length, options)) @@ -859,13 +861,11 @@ bool RTPSender::PrepareAndSendPacket(uint8_t* buffer, UpdateAbsoluteSendTime(buffer_to_send_ptr, length, rtp_header, now_ms); PacketOptions options; - if (AllocateTransportSequenceNumber(&options.packet_id)) { - if (UpdateTransportSequenceNumber(options.packet_id, buffer_to_send_ptr, - length, rtp_header)) { - if (transport_feedback_observer_) - transport_feedback_observer_->AddPacket(options.packet_id, length, - probe_cluster_id); - } + if (UpdateTransportSequenceNumber(buffer_to_send_ptr, length, rtp_header, + &options.packet_id)) { + if (transport_feedback_observer_) + transport_feedback_observer_->AddPacket(options.packet_id, length, + probe_cluster_id); } if (!is_retransmit && !send_over_rtx) { @@ -992,13 +992,11 @@ int32_t RTPSender::SendToNetwork(uint8_t* buffer, } PacketOptions options; - if (AllocateTransportSequenceNumber(&options.packet_id)) { - if (UpdateTransportSequenceNumber(options.packet_id, buffer, length, - rtp_header)) { - if (transport_feedback_observer_) - transport_feedback_observer_->AddPacket(options.packet_id, length, - PacketInfo::kNotAProbe); - } + if (UpdateTransportSequenceNumber(buffer, length, rtp_header, + &options.packet_id)) { + if (transport_feedback_observer_) + transport_feedback_observer_->AddPacket(options.packet_id, length, + PacketInfo::kNotAProbe); } UpdateDelayStatistics(capture_time_ms, now_ms); UpdateOnSendPacket(options.packet_id, capture_time_ms, rtp_header.ssrc); @@ -1587,11 +1585,11 @@ void RTPSender::UpdateAbsoluteSendTime(uint8_t* rtp_packet, ConvertMsTo24Bits(now_ms)); } -bool RTPSender::UpdateTransportSequenceNumber( - uint16_t sequence_number, - uint8_t* rtp_packet, - size_t rtp_packet_length, - const RTPHeader& rtp_header) const { +bool RTPSender::UpdateTransportSequenceNumber(uint8_t* rtp_packet, + size_t rtp_packet_length, + const RTPHeader& rtp_header, + int* sequence_number) const { + RTC_DCHECK(sequence_number); size_t offset; rtc::CritScope lock(&send_critsect_); @@ -1609,7 +1607,10 @@ bool RTPSender::UpdateTransportSequenceNumber( RTC_NOTREACHED(); } - BuildTransportSequenceNumberExtension(rtp_packet + offset, sequence_number); + if (!AllocateTransportSequenceNumber(sequence_number)) + return false; + + BuildTransportSequenceNumberExtension(rtp_packet + offset, *sequence_number); return true; } diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.h b/webrtc/modules/rtp_rtcp/source/rtp_sender.h index 1d6203a68a..f39309a5d1 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.h @@ -166,17 +166,24 @@ class RTPSender : public RTPSenderInterface { size_t RtpHeaderExtensionLength() const; - uint16_t BuildRTPHeaderExtension(uint8_t* data_buffer, bool marker_bit) const; + uint16_t BuildRTPHeaderExtension(uint8_t* data_buffer, bool marker_bit) const + EXCLUSIVE_LOCKS_REQUIRED(send_critsect_); - uint8_t BuildTransmissionTimeOffsetExtension(uint8_t *data_buffer) const; - uint8_t BuildAudioLevelExtension(uint8_t* data_buffer) const; - uint8_t BuildAbsoluteSendTimeExtension(uint8_t* data_buffer) const; - uint8_t BuildVideoRotationExtension(uint8_t* data_buffer) const; + uint8_t BuildTransmissionTimeOffsetExtension(uint8_t* data_buffer) const + EXCLUSIVE_LOCKS_REQUIRED(send_critsect_); + uint8_t BuildAudioLevelExtension(uint8_t* data_buffer) const + EXCLUSIVE_LOCKS_REQUIRED(send_critsect_); + uint8_t BuildAbsoluteSendTimeExtension(uint8_t* data_buffer) const + EXCLUSIVE_LOCKS_REQUIRED(send_critsect_); + uint8_t BuildVideoRotationExtension(uint8_t* data_buffer) const + EXCLUSIVE_LOCKS_REQUIRED(send_critsect_); uint8_t BuildTransportSequenceNumberExtension(uint8_t* data_buffer, - uint16_t sequence_number) const; + uint16_t sequence_number) const + EXCLUSIVE_LOCKS_REQUIRED(send_critsect_); uint8_t BuildPlayoutDelayExtension(uint8_t* data_buffer, uint16_t min_playout_delay_ms, - uint16_t max_playout_delay_ms) const; + uint16_t max_playout_delay_ms) const + EXCLUSIVE_LOCKS_REQUIRED(send_critsect_); // Verifies that the specified extension is registered, and that it is // present in rtp packet. If extension is not registered kNotRegistered is @@ -335,7 +342,8 @@ class RTPSender : public RTPSenderInterface { bool marker_bit, uint32_t timestamp, uint16_t sequence_number, - const std::vector& csrcs) const; + const std::vector& csrcs) const + EXCLUSIVE_LOCKS_REQUIRED(send_critsect_); bool PrepareAndSendPacket(uint8_t* buffer, size_t length, @@ -370,7 +378,8 @@ class RTPSender : public RTPSenderInterface { const uint8_t* rtp_packet, size_t rtp_packet_length, const RTPHeader& rtp_header, - size_t* position) const; + size_t* position) const + EXCLUSIVE_LOCKS_REQUIRED(send_critsect_); void UpdateTransmissionTimeOffset(uint8_t* rtp_packet, size_t rtp_packet_length, @@ -381,10 +390,10 @@ class RTPSender : public RTPSenderInterface { const RTPHeader& rtp_header, int64_t now_ms) const; - bool UpdateTransportSequenceNumber(uint16_t sequence_number, - uint8_t* rtp_packet, + bool UpdateTransportSequenceNumber(uint8_t* rtp_packet, size_t rtp_packet_length, - const RTPHeader& rtp_header) const; + const RTPHeader& rtp_header, + int* sequence_number) const; void UpdatePlayoutDelayLimits(uint8_t* rtp_packet, size_t rtp_packet_length, @@ -423,7 +432,7 @@ class RTPSender : public RTPSenderInterface { int8_t payload_type_ GUARDED_BY(send_critsect_); std::map payload_type_map_; - RtpHeaderExtensionMap rtp_header_extension_map_; + RtpHeaderExtensionMap rtp_header_extension_map_ GUARDED_BY(send_critsect_); int32_t transmission_time_offset_; uint32_t absolute_send_time_; VideoRotation rotation_; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 99cef009e7..e2709db31c 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -152,10 +152,10 @@ class RtpSenderTest : public ::testing::Test { } SimulatedClock fake_clock_; - MockRtcEventLog mock_rtc_event_log_; + testing::NiceMock mock_rtc_event_log_; MockRtpPacketSender mock_paced_sender_; - MockTransportSequenceNumberAllocator seq_num_allocator_; - MockSendPacketObserver send_packet_observer_; + testing::StrictMock seq_num_allocator_; + testing::StrictMock send_packet_observer_; RateLimiter retransmission_rate_limiter_; std::unique_ptr rtp_sender_; int payload_; @@ -491,10 +491,6 @@ TEST_F(RtpSenderTestWithoutPacer, BuildRTPPacketWithAbsoluteSendTimeExtension) { } TEST_F(RtpSenderTestWithoutPacer, SendsPacketsWithTransportSequenceNumber) { - // Ignore rtc event calls. - EXPECT_CALL(mock_rtc_event_log_, - LogRtpHeader(PacketDirection::kOutgoingPacket, _, _, _)); - EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension( kRtpExtensionTransportSequenceNumber, kTransportSequenceNumberExtensionId)); @@ -521,10 +517,14 @@ TEST_F(RtpSenderTestWithoutPacer, SendsPacketsWithTransportSequenceNumber) { rtp_header.extension.transportSequenceNumber); } -TEST_F(RtpSenderTestWithoutPacer, OnSendPacketUpdated) { - EXPECT_CALL(mock_rtc_event_log_, // Ignore rtc event calls. - LogRtpHeader(PacketDirection::kOutgoingPacket, _, _, _)); +TEST_F(RtpSenderTestWithoutPacer, NoAllocationIfNotRegistered) { + SendGenericPayload(); +} +TEST_F(RtpSenderTestWithoutPacer, OnSendPacketUpdated) { + EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension( + kRtpExtensionTransportSequenceNumber, + kTransportSequenceNumberExtensionId)); EXPECT_CALL(seq_num_allocator_, AllocateSequenceNumber()) .WillOnce(testing::Return(kTransportSequenceNumber)); EXPECT_CALL(send_packet_observer_, @@ -972,8 +972,9 @@ TEST_F(RtpSenderTest, SendPadding) { } TEST_F(RtpSenderTest, OnSendPacketUpdated) { - EXPECT_CALL(mock_rtc_event_log_, // Ignore rtc event calls. - LogRtpHeader(PacketDirection::kOutgoingPacket, _, _, _)); + EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension( + kRtpExtensionTransportSequenceNumber, + kTransportSequenceNumberExtensionId)); rtp_sender_->SetStorePacketsStatus(true, 10); EXPECT_CALL(send_packet_observer_, @@ -991,8 +992,9 @@ TEST_F(RtpSenderTest, OnSendPacketUpdated) { } TEST_F(RtpSenderTest, OnSendPacketNotUpdatedForRetransmits) { - EXPECT_CALL(mock_rtc_event_log_, // Ignore rtc event calls. - LogRtpHeader(PacketDirection::kOutgoingPacket, _, _, _)); + EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension( + kRtpExtensionTransportSequenceNumber, + kTransportSequenceNumberExtensionId)); rtp_sender_->SetStorePacketsStatus(true, 10); EXPECT_CALL(send_packet_observer_, OnSendPacket(_, _, _)).Times(0); @@ -1012,6 +1014,9 @@ TEST_F(RtpSenderTest, OnSendPacketNotUpdatedWithoutSeqNumAllocator) { false, &fake_clock_, &transport_, &mock_paced_sender_, nullptr /* TransportSequenceNumberAllocator */, nullptr, nullptr, nullptr, nullptr, nullptr, &send_packet_observer_, nullptr)); + EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension( + kRtpExtensionTransportSequenceNumber, + kTransportSequenceNumberExtensionId)); rtp_sender_->SetSequenceNumber(kSeqNum); rtp_sender_->SetStorePacketsStatus(true, 10); diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index 7e25f95af0..034d063814 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -2077,6 +2077,11 @@ void EndToEndTest::VerifyHistogramStats(bool use_rtx, VideoSendStream::Config* send_config, std::vector* receive_configs, VideoEncoderConfig* encoder_config) override { + static const int kExtensionId = 8; + send_config->rtp.extensions.push_back(RtpExtension( + RtpExtension::kTransportSequenceNumberUri, kExtensionId)); + (*receive_configs)[0].rtp.extensions.push_back(RtpExtension( + RtpExtension::kTransportSequenceNumberUri, kExtensionId)); // NACK send_config->rtp.nack.rtp_history_ms = kNackRtpHistoryMs; (*receive_configs)[0].rtp.nack.rtp_history_ms = kNackRtpHistoryMs;