diff --git a/audio/channel_send.cc b/audio/channel_send.cc index 99e3f9032d..63d61cfc67 100644 --- a/audio/channel_send.cc +++ b/audio/channel_send.cc @@ -314,14 +314,11 @@ class TransportFeedbackProxy : public TransportFeedbackObserver { } // Implements TransportFeedbackObserver. - void AddPacket(uint32_t ssrc, - uint16_t sequence_number, - size_t length, - const PacedPacketInfo& pacing_info) override { + void OnAddPacket(const RtpPacketSendInfo& packet_info) override { RTC_DCHECK(pacer_thread_.IsCurrent()); rtc::CritScope lock(&crit_); if (feedback_observer_) - feedback_observer_->AddPacket(ssrc, sequence_number, length, pacing_info); + feedback_observer_->OnAddPacket(packet_info); } void OnTransportFeedback(const rtcp::TransportFeedback& feedback) override { diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc index 203d41e751..7c8c4c3b81 100644 --- a/call/rtp_transport_controller_send.cc +++ b/call/rtp_transport_controller_send.cc @@ -408,15 +408,12 @@ void RtpTransportControllerSend::OnReceivedRtcpReceiverReport( }); } -void RtpTransportControllerSend::AddPacket(uint32_t ssrc, - uint16_t sequence_number, - size_t length, - const PacedPacketInfo& pacing_info) { - if (send_side_bwe_with_overhead_) { - length += transport_overhead_bytes_per_packet_; - } +void RtpTransportControllerSend::OnAddPacket( + const RtpPacketSendInfo& packet_info) { transport_feedback_adapter_.AddPacket( - ssrc, sequence_number, length, pacing_info, + packet_info, + send_side_bwe_with_overhead_ ? transport_overhead_bytes_per_packet_.load() + : 0, Timestamp::ms(clock_->TimeInMilliseconds())); } diff --git a/call/rtp_transport_controller_send.h b/call/rtp_transport_controller_send.h index df4892ce64..78617afe6a 100644 --- a/call/rtp_transport_controller_send.h +++ b/call/rtp_transport_controller_send.h @@ -110,10 +110,7 @@ class RtpTransportControllerSend final int64_t now_ms) override; // Implements TransportFeedbackObserver interface - void AddPacket(uint32_t ssrc, - uint16_t sequence_number, - size_t length, - const PacedPacketInfo& pacing_info) override; + void OnAddPacket(const RtpPacketSendInfo& packet_info) override; void OnTransportFeedback(const rtcp::TransportFeedback& feedback) override; private: diff --git a/logging/rtc_event_log/rtc_event_log_parser.cc b/logging/rtc_event_log/rtc_event_log_parser.cc index 5a3d1aee01..289744da27 100644 --- a/logging/rtc_event_log/rtc_event_log_parser.cc +++ b/logging/rtc_event_log/rtc_event_log_parser.cc @@ -1961,14 +1961,23 @@ std::vector ParsedRtcEventLog::GetPacketInfos( logged.overhead = current_overhead; if (rtp.header.extension.hasTransportSequenceNumber) { logged.log_feedback_time = Timestamp::PlusInfinity(); + + RtpPacketSendInfo packet_info; + packet_info.ssrc = rtp.header.ssrc; + packet_info.transport_sequence_number = + rtp.header.extension.transportSequenceNumber; + packet_info.rtp_sequence_number = rtp.header.sequenceNumber; + packet_info.has_rtp_sequence_number = true; + packet_info.length = rtp.total_length; + feedback_adapter.AddPacket(packet_info, + 0u, // Should this be current_overhead? + Timestamp::ms(rtp.log_time_ms())); + rtc::SentPacket sent_packet; sent_packet.send_time_ms = rtp.log_time_ms(); sent_packet.info.packet_size_bytes = rtp.total_length; sent_packet.info.included_in_feedback = true; sent_packet.packet_id = rtp.header.extension.transportSequenceNumber; - feedback_adapter.AddPacket(rtp.header.ssrc, sent_packet.packet_id, - rtp.total_length, PacedPacketInfo(), - Timestamp::ms(rtp.log_time_ms())); auto sent_packet_msg = feedback_adapter.ProcessSentPacket(sent_packet); RTC_CHECK(sent_packet_msg); indices[sent_packet_msg->sequence_number] = packets.size(); diff --git a/modules/congestion_controller/include/send_side_congestion_controller.h b/modules/congestion_controller/include/send_side_congestion_controller.h index d1222a2c6b..f78763a302 100644 --- a/modules/congestion_controller/include/send_side_congestion_controller.h +++ b/modules/congestion_controller/include/send_side_congestion_controller.h @@ -43,7 +43,7 @@ class RateLimiter; class RtcEventLog; class CongestionWindowPushbackController; -// Deprecated, for somewhat similar funtionality GoogCcNetworkController can be +// Deprecated, for somewhat similar functionality GoogCcNetworkController can be // used via GoogCcNetworkControllerFactory. class DEPRECATED_SendSideCongestionController : public SendSideCongestionControllerInterface { @@ -107,10 +107,7 @@ class DEPRECATED_SendSideCongestionController void Process() override; // Implements TransportFeedbackObserver. - void AddPacket(uint32_t ssrc, - uint16_t sequence_number, - size_t length, - const PacedPacketInfo& pacing_info) override; + void OnAddPacket(const RtpPacketSendInfo& packet_info) override; void OnTransportFeedback(const rtcp::TransportFeedback& feedback) override; std::vector GetTransportFeedbackVector() const; diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter.cc b/modules/congestion_controller/rtp/transport_feedback_adapter.cc index b1699b4cb6..b99702ba17 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter.cc @@ -83,18 +83,35 @@ void TransportFeedbackAdapter::AddPacket(uint32_t ssrc, size_t length, const PacedPacketInfo& pacing_info, Timestamp creation_time) { + RtpPacketSendInfo packet_info; + packet_info.ssrc = ssrc; + packet_info.transport_sequence_number = sequence_number; + packet_info.length = length; + packet_info.pacing_info = pacing_info; + AddPacket(packet_info, 0u, creation_time); +} + +void TransportFeedbackAdapter::AddPacket(const RtpPacketSendInfo& packet_info, + size_t overhead_bytes, + Timestamp creation_time) { { rtc::CritScope cs(&lock_); - send_time_history_.AddAndRemoveOld( - PacketFeedback(creation_time.ms(), sequence_number, length, - local_net_id_, remote_net_id_, pacing_info), - creation_time.ms()); + PacketFeedback packet_feedback( + creation_time.ms(), packet_info.transport_sequence_number, + packet_info.length + overhead_bytes, local_net_id_, remote_net_id_, + packet_info.pacing_info); + if (packet_info.has_rtp_sequence_number) { + packet_feedback.ssrc = packet_info.ssrc; + packet_feedback.rtp_sequence_number = packet_info.rtp_sequence_number; + } + send_time_history_.AddAndRemoveOld(packet_feedback, creation_time.ms()); } { rtc::CritScope cs(&observers_lock_); for (auto* observer : observers_) { - observer->OnPacketAdded(ssrc, sequence_number); + observer->OnPacketAdded(packet_info.ssrc, + packet_info.transport_sequence_number); } } } diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter.h b/modules/congestion_controller/rtp/transport_feedback_adapter.h index 3fb4fddcee..7c9884ac47 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter.h +++ b/modules/congestion_controller/rtp/transport_feedback_adapter.h @@ -24,6 +24,7 @@ namespace webrtc { class PacketFeedbackObserver; +struct RtpPacketSendInfo; namespace rtcp { class TransportFeedback; @@ -37,12 +38,16 @@ class TransportFeedbackAdapter { void RegisterPacketFeedbackObserver(PacketFeedbackObserver* observer); void DeRegisterPacketFeedbackObserver(PacketFeedbackObserver* observer); + // TODO(webrtc:8975): Remove when downstream projects have been updated. void AddPacket(uint32_t ssrc, uint16_t sequence_number, size_t length, const PacedPacketInfo& pacing_info, Timestamp creation_time); + void AddPacket(const RtpPacketSendInfo& packet_info, + size_t overhead_bytes, + Timestamp creation_time); absl::optional ProcessSentPacket( const rtc::SentPacket& sent_packet); diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc index c9ec37b089..fbd02dd650 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc @@ -64,9 +64,14 @@ class TransportFeedbackAdapterTest : public ::testing::Test { int64_t now_ms) {} void OnSentPacket(const PacketFeedback& packet_feedback) { - adapter_->AddPacket(kSsrc, packet_feedback.sequence_number, - packet_feedback.payload_size, - packet_feedback.pacing_info, + RtpPacketSendInfo packet_info; + packet_info.ssrc = kSsrc; + packet_info.transport_sequence_number = packet_feedback.sequence_number; + packet_info.rtp_sequence_number = 0; + packet_info.has_rtp_sequence_number = true; + packet_info.length = packet_feedback.payload_size; + packet_info.pacing_info = packet_feedback.pacing_info; + adapter_->AddPacket(RtpPacketSendInfo(packet_info), 0u, Timestamp::ms(clock_.TimeInMilliseconds())); adapter_->ProcessSentPacket(rtc::SentPacket(packet_feedback.sequence_number, packet_feedback.send_time_ms, diff --git a/modules/congestion_controller/send_side_congestion_controller.cc b/modules/congestion_controller/send_side_congestion_controller.cc index 1fbc63da39..5a9d3d810c 100644 --- a/modules/congestion_controller/send_side_congestion_controller.cc +++ b/modules/congestion_controller/send_side_congestion_controller.cc @@ -365,17 +365,16 @@ void DEPRECATED_SendSideCongestionController::Process() { MaybeTriggerOnNetworkChanged(); } -void DEPRECATED_SendSideCongestionController::AddPacket( - uint32_t ssrc, - uint16_t sequence_number, - size_t length, - const PacedPacketInfo& pacing_info) { +void DEPRECATED_SendSideCongestionController::OnAddPacket( + const RtpPacketSendInfo& packet_info) { + size_t overhead_bytes = 0; if (send_side_bwe_with_overhead_) { rtc::CritScope cs(&bwe_lock_); - length += transport_overhead_bytes_per_packet_; + overhead_bytes = transport_overhead_bytes_per_packet_; } - transport_feedback_adapter_.AddPacket(ssrc, sequence_number, length, - pacing_info); + transport_feedback_adapter_.AddPacket( + packet_info.ssrc, packet_info.transport_sequence_number, + packet_info.length + overhead_bytes, packet_info.pacing_info); } void DEPRECATED_SendSideCongestionController::OnTransportFeedback( diff --git a/modules/congestion_controller/send_side_congestion_controller_unittest.cc b/modules/congestion_controller/send_side_congestion_controller_unittest.cc index f617e564e2..bb4cb7d302 100644 --- a/modules/congestion_controller/send_side_congestion_controller_unittest.cc +++ b/modules/congestion_controller/send_side_congestion_controller_unittest.cc @@ -77,10 +77,15 @@ class LegacySendSideCongestionControllerTest : public ::testing::Test { } void OnSentPacket(const PacketFeedback& packet_feedback) { - constexpr uint32_t ssrc = 0; - controller_->AddPacket(ssrc, packet_feedback.sequence_number, - packet_feedback.payload_size, - packet_feedback.pacing_info); + RtpPacketSendInfo packet_info; + packet_info.ssrc = 0; + packet_info.transport_sequence_number = packet_feedback.sequence_number; + packet_info.rtp_sequence_number = 0; + packet_info.has_rtp_sequence_number = true; + packet_info.length = packet_feedback.payload_size; + packet_info.pacing_info = packet_feedback.pacing_info; + + controller_->OnAddPacket(packet_info); controller_->OnSentPacket(rtc::SentPacket(packet_feedback.sequence_number, packet_feedback.send_time_ms)); } diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.cc b/modules/rtp_rtcp/include/rtp_rtcp_defines.cc index 0711b413fe..19dafb3296 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.cc +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.cc @@ -95,11 +95,14 @@ PacketFeedback::PacketFeedback(int64_t creation_time_ms, arrival_time_ms(arrival_time_ms), send_time_ms(send_time_ms), sequence_number(sequence_number), + long_sequence_number(0), payload_size(payload_size), unacknowledged_data(0), local_net_id(local_net_id), remote_net_id(remote_net_id), - pacing_info(pacing_info) {} + pacing_info(pacing_info), + ssrc(0), + rtp_sequence_number(0) {} PacketFeedback::PacketFeedback(const PacketFeedback&) = default; PacketFeedback& PacketFeedback::operator=(const PacketFeedback&) = default; diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index 41ab1cf0d2..ac492f6bc8 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -274,6 +274,10 @@ struct PacketFeedback { uint16_t remote_net_id; // Pacing information about this packet. PacedPacketInfo pacing_info; + + // The SSRC and RTP sequence number of the packet this feedback refers to. + uint32_t ssrc; + uint16_t rtp_sequence_number; }; class PacketFeedbackComparator { @@ -287,16 +291,42 @@ class PacketFeedbackComparator { } }; +struct RtpPacketSendInfo { + public: + RtpPacketSendInfo() = default; + + uint16_t transport_sequence_number = 0; + uint32_t ssrc = 0; + uint16_t rtp_sequence_number = 0; + // Get rid of this flag when all code paths populate |rtp_sequence_number|. + bool has_rtp_sequence_number = false; + size_t length = 0; + PacedPacketInfo pacing_info; +}; + class TransportFeedbackObserver { public: TransportFeedbackObserver() {} virtual ~TransportFeedbackObserver() {} - // Note: Transport-wide sequence number as sequence number. + // TODO(webrtc:8975): Remove when downstream projects have been updated. virtual void AddPacket(uint32_t ssrc, - uint16_t sequence_number, + uint16_t sequence_number, // Transport-wide. size_t length, - const PacedPacketInfo& pacing_info) = 0; + const PacedPacketInfo& pacing_info) { + RtpPacketSendInfo packet_info; + packet_info.ssrc = ssrc; + packet_info.transport_sequence_number = sequence_number; + packet_info.length = length; + packet_info.pacing_info = pacing_info; + OnAddPacket(packet_info); + } + + virtual void OnAddPacket(const RtpPacketSendInfo& packet_info) { + // TODO(webrtc:8975): Remove when downstream projects have been updated. + AddPacket(packet_info.ssrc, packet_info.transport_sequence_number, + packet_info.length, packet_info.pacing_info); + } virtual void OnTransportFeedback(const rtcp::TransportFeedback& feedback) = 0; }; diff --git a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index e1483a42f2..332495d677 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -84,9 +84,7 @@ class MockRtcpCallbackImpl : public RtcpStatisticsCallback { class MockTransportFeedbackObserver : public TransportFeedbackObserver { public: - MOCK_METHOD3(AddPacket, void(uint32_t, uint16_t, size_t)); - MOCK_METHOD4(AddPacket, - void(uint32_t, uint16_t, size_t, const PacedPacketInfo&)); + MOCK_METHOD1(OnAddPacket, void(const RtpPacketSendInfo&)); MOCK_METHOD1(OnTransportFeedback, void(const rtcp::TransportFeedback&)); MOCK_CONST_METHOD0(GetTransportFeedbackVector, std::vector()); }; diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 9f3cefce8e..2e198fc999 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -1198,14 +1198,19 @@ void RTPSender::AddPacketToTransportFeedback( uint16_t packet_id, const RtpPacketToSend& packet, const PacedPacketInfo& pacing_info) { - size_t packet_size = packet.payload_size() + packet.padding_size(); - if (send_side_bwe_with_overhead_) { - packet_size = packet.size(); - } - if (transport_feedback_observer_) { - transport_feedback_observer_->AddPacket(SSRC(), packet_id, packet_size, - pacing_info); + size_t packet_size = packet.payload_size() + packet.padding_size(); + if (send_side_bwe_with_overhead_) { + packet_size = packet.size(); + } + + RtpPacketSendInfo packet_info; + packet_info.ssrc = SSRC(); + packet_info.transport_sequence_number = packet_id; + packet_info.rtp_sequence_number = packet.SequenceNumber(); + packet_info.length = packet_size; + packet_info.pacing_info = pacing_info; + transport_feedback_observer_->OnAddPacket(packet_info); } } diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 3d9550b22e..ba841c2d95 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -69,8 +69,10 @@ const char kNoRid[] = ""; const char kNoMid[] = ""; using ::testing::_; +using ::testing::AllOf; using ::testing::ElementsAre; using ::testing::ElementsAreArray; +using ::testing::Field; using ::testing::Invoke; using ::testing::SizeIs; @@ -162,8 +164,7 @@ class MockSendPacketObserver : public SendPacketObserver { class MockTransportFeedbackObserver : public TransportFeedbackObserver { public: - MOCK_METHOD4(AddPacket, - void(uint32_t, uint16_t, size_t, const PacedPacketInfo&)); + MOCK_METHOD1(OnAddPacket, void(const RtpPacketSendInfo&)); MOCK_METHOD1(OnTransportFeedback, void(const rtcp::TransportFeedback&)); MOCK_CONST_METHOD0(GetTransportFeedbackVector, std::vector()); }; @@ -396,8 +397,14 @@ TEST_P(RtpSenderTestWithoutPacer, : sizeof(kPayloadData); EXPECT_CALL(feedback_observer_, - AddPacket(rtp_sender_->SSRC(), kTransportSequenceNumber, - expected_bytes, PacedPacketInfo())) + OnAddPacket(AllOf( + Field(&RtpPacketSendInfo::ssrc, rtp_sender_->SSRC()), + Field(&RtpPacketSendInfo::transport_sequence_number, + kTransportSequenceNumber), + Field(&RtpPacketSendInfo::rtp_sequence_number, + rtp_sender_->SequenceNumber()), + Field(&RtpPacketSendInfo::length, expected_bytes), + Field(&RtpPacketSendInfo::pacing_info, PacedPacketInfo())))) .Times(1); EXPECT_CALL(mock_overhead_observer, OnOverheadChanged(kRtpOverheadBytesPerPacket)) @@ -424,8 +431,13 @@ TEST_P(RtpSenderTestWithoutPacer, SendsPacketsWithTransportSequenceNumber) { .Times(1); EXPECT_CALL(feedback_observer_, - AddPacket(rtp_sender_->SSRC(), kTransportSequenceNumber, _, - PacedPacketInfo())) + OnAddPacket(AllOf( + Field(&RtpPacketSendInfo::ssrc, rtp_sender_->SSRC()), + Field(&RtpPacketSendInfo::transport_sequence_number, + kTransportSequenceNumber), + Field(&RtpPacketSendInfo::rtp_sequence_number, + rtp_sender_->SequenceNumber()), + Field(&RtpPacketSendInfo::pacing_info, PacedPacketInfo())))) .Times(1); SendGenericPacket(); @@ -597,8 +609,13 @@ TEST_P(RtpSenderTest, SendsPacketsWithTransportSequenceNumber) { OnSendPacket(kTransportSequenceNumber, _, _)) .Times(1); EXPECT_CALL(feedback_observer_, - AddPacket(rtp_sender_->SSRC(), kTransportSequenceNumber, _, - PacedPacketInfo())) + OnAddPacket(AllOf( + Field(&RtpPacketSendInfo::ssrc, rtp_sender_->SSRC()), + Field(&RtpPacketSendInfo::transport_sequence_number, + kTransportSequenceNumber), + Field(&RtpPacketSendInfo::rtp_sequence_number, + rtp_sender_->SequenceNumber()), + Field(&RtpPacketSendInfo::pacing_info, PacedPacketInfo())))) .Times(1); SendGenericPacket(); diff --git a/rtc_tools/event_log_visualizer/analyzer.cc b/rtc_tools/event_log_visualizer/analyzer.cc index bb662d03bb..00369e8d92 100644 --- a/rtc_tools/event_log_visualizer/analyzer.cc +++ b/rtc_tools/event_log_visualizer/analyzer.cc @@ -1286,10 +1286,16 @@ void EventLogAnalyzer::CreateSendSideBweSimulationGraph(Plot* plot) { const RtpPacketType& rtp_packet = *rtp_iterator->second; if (rtp_packet.rtp.header.extension.hasTransportSequenceNumber) { RTC_DCHECK(rtp_packet.rtp.header.extension.hasTransportSequenceNumber); + RtpPacketSendInfo packet_info; + packet_info.ssrc = rtp_packet.rtp.header.ssrc; + packet_info.rtp_sequence_number = + rtp_packet.rtp.header.extension.transportSequenceNumber; + packet_info.rtp_sequence_number = rtp_packet.rtp.header.sequenceNumber; + packet_info.has_rtp_sequence_number = true; + packet_info.length = rtp_packet.rtp.total_length; transport_feedback.AddPacket( - rtp_packet.rtp.header.ssrc, - rtp_packet.rtp.header.extension.transportSequenceNumber, - rtp_packet.rtp.total_length, PacedPacketInfo(), + packet_info, + 0u, // Per packet overhead bytes. Timestamp::us(rtp_packet.rtp.log_time_us())); rtc::SentPacket sent_packet( rtp_packet.rtp.header.extension.transportSequenceNumber, diff --git a/rtc_tools/event_log_visualizer/log_simulation.cc b/rtc_tools/event_log_visualizer/log_simulation.cc index 4778cc505d..1595e35a1c 100644 --- a/rtc_tools/event_log_visualizer/log_simulation.cc +++ b/rtc_tools/event_log_visualizer/log_simulation.cc @@ -80,8 +80,15 @@ void LogBasedNetworkControllerSimulation::OnPacketSent( pending_probes_.pop_front(); } } - transport_feedback_.AddPacket(packet.ssrc, packet.transport_seq_no, - packet.size + packet.overhead, probe_info, + + RtpPacketSendInfo packet_info; + packet_info.ssrc = packet.ssrc; + packet_info.transport_sequence_number = packet.transport_seq_no; + packet_info.rtp_sequence_number = packet.stream_seq_no; + packet_info.has_rtp_sequence_number = true; + packet_info.length = packet.size; + packet_info.pacing_info = probe_info; + transport_feedback_.AddPacket(packet_info, packet.overhead, packet.log_packet_time); } rtc::SentPacket sent_packet;