diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc index 8b5096b519..08b17c2c21 100644 --- a/call/rtp_transport_controller_send.cc +++ b/call/rtp_transport_controller_send.cc @@ -618,13 +618,19 @@ void RtpTransportControllerSend::NotifyBweOfPacedSentPacket( RTC_DCHECK_NOTREACHED() << "Unknown packet type"; return; } - - RtpPacketSendInfo packet_info = RtpPacketSendInfo::From(packet, pacing_info); + if (packet.HasExtension()) { + // TODO: bugs.webrtc.org/42225697 - Refactor TransportFeedbackDemuxer to use + // TransportPacketsFeedback instead of directly using + // rtcp::TransportFeedback. For now, only use it if TransportSeqeunce number + // header extension is used. + RtpPacketSendInfo packet_info = + RtpPacketSendInfo::From(packet, pacing_info); + feedback_demuxer_.AddPacket(packet_info); + } Timestamp creation_time = Timestamp::Millis(env_.clock().TimeInMilliseconds()); - feedback_demuxer_.AddPacket(packet_info); transport_feedback_adapter_.AddPacket( - packet_info, transport_overhead_bytes_per_packet_, creation_time); + packet, pacing_info, transport_overhead_bytes_per_packet_, creation_time); } void RtpTransportControllerSend::OnTransportFeedback( diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter.cc b/modules/congestion_controller/rtp/transport_feedback_adapter.cc index 4b7ec5a08b..553cf3f9c5 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter.cc @@ -13,14 +13,15 @@ #include #include -#include +#include #include #include -#include "absl/algorithm/container.h" +#include "api/transport/network_types.h" #include "api/units/timestamp.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" +#include "modules/rtp_rtcp/source/rtp_packet_to_send.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" @@ -86,17 +87,26 @@ bool InFlightBytesTracker::NetworkRouteComparator::operator()( TransportFeedbackAdapter::TransportFeedbackAdapter() = default; -void TransportFeedbackAdapter::AddPacket(const RtpPacketSendInfo& packet_info, +void TransportFeedbackAdapter::AddPacket(const RtpPacketToSend& packet_to_send, + const PacedPacketInfo& pacing_info, size_t overhead_bytes, Timestamp creation_time) { - PacketFeedback packet; - packet.creation_time = creation_time; - packet.sent.sequence_number = - seq_num_unwrapper_.Unwrap(packet_info.transport_sequence_number); - packet.sent.size = DataSize::Bytes(packet_info.length + overhead_bytes); - packet.sent.audio = packet_info.packet_type == RtpPacketMediaType::kAudio; - packet.network_route = network_route_; - packet.sent.pacing_info = packet_info.pacing_info; + RTC_DCHECK(packet_to_send.transport_sequence_number()); + PacketFeedback feedback; + + feedback.creation_time = creation_time; + // Note, if transport sequence number header extension is used, transport + // sequence numbers are wrapped to 16 bit. See + // RtpSenderEgress::CompleteSendPacket. + feedback.sent.sequence_number = seq_num_unwrapper_.Unwrap( + packet_to_send.transport_sequence_number().value_or(0)); + feedback.sent.size = DataSize::Bytes(packet_to_send.size() + overhead_bytes); + feedback.sent.audio = + packet_to_send.packet_type() == RtpPacketMediaType::kAudio; + feedback.network_route = network_route_; + feedback.sent.pacing_info = pacing_info; + feedback.ssrc = packet_to_send.Ssrc(); + feedback.rtp_sequence_number = packet_to_send.SequenceNumber(); while (!history_.empty() && creation_time - history_.begin()->second.creation_time > @@ -106,7 +116,7 @@ void TransportFeedbackAdapter::AddPacket(const RtpPacketSendInfo& packet_info, in_flight_.RemoveInFlightPacketBytes(history_.begin()->second); history_.erase(history_.begin()); } - history_.insert(std::make_pair(packet.sent.sequence_number, packet)); + history_.insert(std::make_pair(feedback.sent.sequence_number, feedback)); } std::optional TransportFeedbackAdapter::ProcessSentPacket( @@ -157,30 +167,6 @@ TransportFeedbackAdapter::ProcessTransportFeedback( return std::nullopt; } - TransportPacketsFeedback msg; - msg.feedback_time = feedback_receive_time; - msg.packet_feedbacks = - ProcessTransportFeedbackInner(feedback, feedback_receive_time); - if (msg.packet_feedbacks.empty()) - return std::nullopt; - msg.data_in_flight = in_flight_.GetOutstandingData(network_route_); - - return msg; -} - -void TransportFeedbackAdapter::SetNetworkRoute( - const rtc::NetworkRoute& network_route) { - network_route_ = network_route; -} - -DataSize TransportFeedbackAdapter::GetOutstandingData() const { - return in_flight_.GetOutstandingData(network_route_); -} - -std::vector -TransportFeedbackAdapter::ProcessTransportFeedbackInner( - const rtcp::TransportFeedback& feedback, - Timestamp feedback_receive_time) { // Add timestamp deltas to a local time base selected on first packet arrival. // This won't be the true time base, but makes it easier to manually inspect // time stamps. @@ -206,52 +192,28 @@ TransportFeedbackAdapter::ProcessTransportFeedbackInner( size_t failed_lookups = 0; size_t ignored = 0; - feedback.ForAllPackets( - [&](uint16_t sequence_number, TimeDelta delta_since_base) { - int64_t seq_num = seq_num_unwrapper_.Unwrap(sequence_number); - - if (seq_num > last_ack_seq_num_) { - // Starts at history_.begin() if last_ack_seq_num_ < 0, since any - // valid sequence number is >= 0. - for (auto it = history_.upper_bound(last_ack_seq_num_); - it != history_.upper_bound(seq_num); ++it) { - in_flight_.RemoveInFlightPacketBytes(it->second); - } - last_ack_seq_num_ = seq_num; - } - - auto it = history_.find(seq_num); - if (it == history_.end()) { - ++failed_lookups; - return; - } - - if (it->second.sent.send_time.IsInfinite()) { - // TODO(srte): Fix the tests that makes this happen and make this a - // DCHECK. - RTC_DLOG(LS_ERROR) - << "Received feedback before packet was indicated as sent"; - return; - } - - PacketFeedback packet_feedback = it->second; - if (delta_since_base.IsFinite()) { - packet_feedback.receive_time = - current_offset_ + - delta_since_base.RoundDownTo(TimeDelta::Millis(1)); - // Note: Lost packets are not removed from history because they might - // be reported as received by a later feedback. - history_.erase(it); - } - if (packet_feedback.network_route == network_route_) { - PacketResult result; - result.sent_packet = packet_feedback.sent; - result.receive_time = packet_feedback.receive_time; - packet_result_vector.push_back(result); - } else { - ++ignored; - } - }); + feedback.ForAllPackets([&](uint16_t sequence_number, + TimeDelta delta_since_base) { + int64_t seq_num = seq_num_unwrapper_.Unwrap(sequence_number); + std::optional packet_feedback = RetrievePacketFeedback( + seq_num, /*received=*/delta_since_base.IsFinite()); + if (!packet_feedback) { + ++failed_lookups; + return; + } + if (delta_since_base.IsFinite()) { + packet_feedback->receive_time = + current_offset_ + delta_since_base.RoundDownTo(TimeDelta::Millis(1)); + } + if (packet_feedback->network_route == network_route_) { + PacketResult result; + result.sent_packet = packet_feedback->sent; + result.receive_time = packet_feedback->receive_time; + packet_result_vector.push_back(result); + } else { + ++ignored; + } + }); if (failed_lookups > 0) { RTC_LOG(LS_WARNING) @@ -263,8 +225,69 @@ TransportFeedbackAdapter::ProcessTransportFeedbackInner( RTC_LOG(LS_INFO) << "Ignoring " << ignored << " packets because they were sent on a different route."; } + return ToTransportFeedback(std::move(packet_result_vector), + feedback_receive_time); +} - return packet_result_vector; +std::optional +TransportFeedbackAdapter::ToTransportFeedback( + std::vector packet_results, + Timestamp feedback_receive_time) { + TransportPacketsFeedback msg; + msg.feedback_time = feedback_receive_time; + if (packet_results.empty()) { + return std::nullopt; + } + msg.packet_feedbacks = std::move(packet_results); + msg.data_in_flight = in_flight_.GetOutstandingData(network_route_); + + return msg; +} + +void TransportFeedbackAdapter::SetNetworkRoute( + const rtc::NetworkRoute& network_route) { + network_route_ = network_route; +} + +DataSize TransportFeedbackAdapter::GetOutstandingData() const { + return in_flight_.GetOutstandingData(network_route_); +} + +std::optional TransportFeedbackAdapter::RetrievePacketFeedback( + int64_t seq_num, + bool received) { + if (seq_num > last_ack_seq_num_) { + // Starts at history_.begin() if last_ack_seq_num_ < 0, since any + // valid sequence number is >= 0. + for (auto it = history_.upper_bound(last_ack_seq_num_); + it != history_.upper_bound(seq_num); ++it) { + in_flight_.RemoveInFlightPacketBytes(it->second); + } + last_ack_seq_num_ = seq_num; + } + + auto it = history_.find(seq_num); + if (it == history_.end()) { + RTC_LOG(LS_WARNING) << "Failed to lookup send time for packet with " + << seq_num << ". Send time history too small?"; + return std::nullopt; + } + + if (it->second.sent.send_time.IsInfinite()) { + // TODO(srte): Fix the tests that makes this happen and make this a + // DCHECK. + RTC_DLOG(LS_ERROR) + << "Received feedback before packet was indicated as sent"; + return std::nullopt; + } + + PacketFeedback packet_feedback = it->second; + if (received) { + // Note: Lost packets are not removed from history because they might + // be reported as received by a later feedback. + history_.erase(it); + } + return packet_feedback; } } // namespace webrtc diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter.h b/modules/congestion_controller/rtp/transport_feedback_adapter.h index fb48b170c6..4976a53e0d 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter.h +++ b/modules/congestion_controller/rtp/transport_feedback_adapter.h @@ -11,9 +11,7 @@ #ifndef MODULES_CONGESTION_CONTROLLER_RTP_TRANSPORT_FEEDBACK_ADAPTER_H_ #define MODULES_CONGESTION_CONTROLLER_RTP_TRANSPORT_FEEDBACK_ADAPTER_H_ -#include #include -#include #include #include "api/sequence_checker.h" @@ -23,7 +21,6 @@ #include "rtc_base/network/sent_packet.h" #include "rtc_base/network_route.h" #include "rtc_base/numerics/sequence_number_unwrapper.h" -#include "rtc_base/thread_annotations.h" namespace webrtc { @@ -39,6 +36,9 @@ struct PacketFeedback { // The network route that this packet is associated with. rtc::NetworkRoute network_route; + + uint32_t ssrc = 0; + uint16_t rtp_sequence_number = 0; }; class InFlightBytesTracker { @@ -59,7 +59,8 @@ class TransportFeedbackAdapter { public: TransportFeedbackAdapter(); - void AddPacket(const RtpPacketSendInfo& packet_info, + void AddPacket(const RtpPacketToSend& packet, + const PacedPacketInfo& pacing_info, size_t overhead_bytes, Timestamp creation_time); std::optional ProcessSentPacket( @@ -80,10 +81,17 @@ class TransportFeedbackAdapter { const rtcp::TransportFeedback& feedback, Timestamp feedback_receive_time); + std::optional RetrievePacketFeedback(int64_t seq_num, + bool received); + std::optional ToTransportFeedback( + std::vector packet_results, + Timestamp feedback_receive_time); + DataSize pending_untracked_size_ = DataSize::Zero(); Timestamp last_send_time_ = Timestamp::MinusInfinity(); Timestamp last_untracked_send_time_ = Timestamp::MinusInfinity(); RtpSequenceNumberUnwrapper seq_num_unwrapper_; + std::map history_; // Sequence numbers are never negative, using -1 as it always < a real diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc index 82d93ed327..f0a08db7b4 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc @@ -10,14 +10,17 @@ #include "modules/congestion_controller/rtp/transport_feedback_adapter.h" +#include #include #include +#include #include +#include "api/transport/network_types.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" +#include "modules/rtp_rtcp/source/rtp_packet_to_send.h" #include "rtc_base/checks.h" -#include "rtc_base/numerics/safe_conversions.h" #include "system_wrappers/include/clock.h" #include "test/field_trial.h" #include "test/gmock.h" @@ -26,6 +29,9 @@ namespace webrtc { namespace { + +using ::testing::SizeIs; + constexpr uint32_t kSsrc = 8492; const PacedPacketInfo kPacingInfo0(0, 5, 2000); const PacedPacketInfo kPacingInfo1(1, 8, 4000); @@ -57,6 +63,7 @@ void ComparePacketFeedbackVectors(const std::vector& truth, EXPECT_EQ(truth[i].sent_packet.size, input[i].sent_packet.size); EXPECT_EQ(truth[i].sent_packet.pacing_info, input[i].sent_packet.pacing_info); + EXPECT_EQ(truth[i].sent_packet.audio, input[i].sent_packet.audio); } } @@ -74,6 +81,22 @@ PacketResult CreatePacket(int64_t receive_time_ms, return res; } +RtpPacketToSend CreatePacketToSend(const PacketResult& packet, + uint32_t ssrc, + uint16_t rtp_sequence_number) { + RtpPacketToSend send_packet(nullptr); + send_packet.SetSsrc(ssrc); + send_packet.SetPayloadSize(packet.sent_packet.size.bytes() - + send_packet.headers_size()); + send_packet.SetSequenceNumber(rtp_sequence_number); + send_packet.set_transport_sequence_number(packet.sent_packet.sequence_number); + send_packet.set_packet_type(packet.sent_packet.audio + ? RtpPacketMediaType::kAudio + : RtpPacketMediaType::kVideo); + + return send_packet; +} + class MockStreamFeedbackObserver : public webrtc::StreamFeedbackObserver { public: MOCK_METHOD(void, @@ -96,16 +119,10 @@ class TransportFeedbackAdapterTest : public ::testing::Test { protected: void OnSentPacket(const PacketResult& packet_feedback) { - RtpPacketSendInfo packet_info; - packet_info.media_ssrc = kSsrc; - packet_info.transport_sequence_number = - packet_feedback.sent_packet.sequence_number; - packet_info.rtp_sequence_number = 0; - packet_info.length = packet_feedback.sent_packet.size.bytes(); - packet_info.pacing_info = packet_feedback.sent_packet.pacing_info; - packet_info.packet_type = RtpPacketMediaType::kVideo; - adapter_->AddPacket(RtpPacketSendInfo(packet_info), 0u, - clock_.CurrentTime()); + RtpPacketToSend packet_to_send = + CreatePacketToSend(packet_feedback, kSsrc, /*rtp_sequence_number=*/0); + adapter_->AddPacket(packet_to_send, packet_feedback.sent_packet.pacing_info, + 0u, clock_.CurrentTime()); adapter_->ProcessSentPacket(rtc::SentPacket( packet_feedback.sent_packet.sequence_number, packet_feedback.sent_packet.send_time.ms(), rtc::PacketInfo())); @@ -215,6 +232,22 @@ TEST_F(TransportFeedbackAdapterTest, HandlesDroppedPackets) { ComparePacketFeedbackVectors(expected_packets, res->packet_feedbacks); } +TEST_F(TransportFeedbackAdapterTest, FeedbackReportsIfPacketIsAudio) { + PacketResult packet = CreatePacket(100, 200, 0, 1500, kPacingInfo0); + packet.sent_packet.audio = true; + OnSentPacket(packet); + + rtcp::TransportFeedback feedback; + feedback.SetBase(packet.sent_packet.sequence_number, packet.receive_time); + feedback.AddReceivedPacket(packet.sent_packet.sequence_number, + packet.receive_time); + feedback.Build(); + + auto res = adapter_->ProcessTransportFeedback(feedback, clock_.CurrentTime()); + ASSERT_THAT(res->packet_feedbacks, SizeIs(1)); + EXPECT_TRUE(res->packet_feedbacks[0].sent_packet.audio); +} + TEST_F(TransportFeedbackAdapterTest, SendTimeWrapsBothWays) { TimeDelta kHighArrivalTime = rtcp::TransportFeedback::kDeltaTick * (1 << 8) * ((1 << 23) - 1); @@ -374,15 +407,11 @@ TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) { TEST_F(TransportFeedbackAdapterTest, IgnoreDuplicatePacketSentCalls) { auto packet = CreatePacket(100, 200, 0, 1500, kPacingInfo0); - + RtpPacketToSend packet_to_send = + CreatePacketToSend(packet, kSsrc, /*rtp_sequence_number=*/0); // Add a packet and then mark it as sent. - RtpPacketSendInfo packet_info; - packet_info.media_ssrc = kSsrc; - packet_info.transport_sequence_number = packet.sent_packet.sequence_number; - packet_info.length = packet.sent_packet.size.bytes(); - packet_info.pacing_info = packet.sent_packet.pacing_info; - packet_info.packet_type = RtpPacketMediaType::kVideo; - adapter_->AddPacket(packet_info, 0u, clock_.CurrentTime()); + adapter_->AddPacket(packet_to_send, packet.sent_packet.pacing_info, 0u, + clock_.CurrentTime()); std::optional sent_packet = adapter_->ProcessSentPacket( rtc::SentPacket(packet.sent_packet.sequence_number, packet.sent_packet.send_time.ms(), rtc::PacketInfo())); diff --git a/rtc_tools/rtc_event_log_visualizer/analyzer.cc b/rtc_tools/rtc_event_log_visualizer/analyzer.cc index d9df39c5d4..b8399f9529 100644 --- a/rtc_tools/rtc_event_log_visualizer/analyzer.cc +++ b/rtc_tools/rtc_event_log_visualizer/analyzer.cc @@ -63,6 +63,7 @@ #include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" #include "modules/rtp_rtcp/source/rtp_header_extensions.h" #include "modules/rtp_rtcp/source/rtp_packet_received.h" +#include "modules/rtp_rtcp/source/rtp_packet_to_send.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" #include "rtc_base/network/sent_packet.h" @@ -1744,26 +1745,28 @@ void EventLogAnalyzer::CreateSendSideBweSimulationGraph(Plot* plot) { RTC_DCHECK_EQ(clock.TimeInMicroseconds(), NextRtpTime()); const RtpPacketType& rtp_packet = *rtp_iterator->second; if (rtp_packet.rtp.header.extension.hasTransportSequenceNumber) { - RtpPacketSendInfo packet_info; - packet_info.media_ssrc = rtp_packet.rtp.header.ssrc; - packet_info.transport_sequence_number = - rtp_packet.rtp.header.extension.transportSequenceNumber; - packet_info.rtp_sequence_number = rtp_packet.rtp.header.sequenceNumber; - packet_info.length = rtp_packet.rtp.total_length; + RtpPacketToSend send_packet(/*extensions=*/nullptr); + send_packet.set_transport_sequence_number( + rtp_packet.rtp.header.extension.transportSequenceNumber); + send_packet.SetSsrc(rtp_packet.rtp.header.ssrc); + send_packet.SetSequenceNumber(rtp_packet.rtp.header.sequenceNumber); + send_packet.SetPayloadSize(rtp_packet.rtp.total_length - + send_packet.headers_size()); + RTC_DCHECK_EQ(send_packet.size(), rtp_packet.rtp.total_length); if (IsRtxSsrc(parsed_log_, PacketDirection::kOutgoingPacket, rtp_packet.rtp.header.ssrc)) { // Don't set the optional media type as we don't know if it is // a retransmission, FEC or padding. } else if (IsVideoSsrc(parsed_log_, PacketDirection::kOutgoingPacket, rtp_packet.rtp.header.ssrc)) { - packet_info.packet_type = RtpPacketMediaType::kVideo; + send_packet.set_packet_type(RtpPacketMediaType::kVideo); } else if (IsAudioSsrc(parsed_log_, PacketDirection::kOutgoingPacket, rtp_packet.rtp.header.ssrc)) { - packet_info.packet_type = RtpPacketMediaType::kAudio; + send_packet.set_packet_type(RtpPacketMediaType::kAudio); } transport_feedback.AddPacket( - packet_info, - 0u, // Per packet overhead bytes. + send_packet, PacedPacketInfo(), + 0u, // Per packet overhead bytes., Timestamp::Micros(rtp_packet.rtp.log_time_us())); } rtc::SentPacket sent_packet; diff --git a/rtc_tools/rtc_event_log_visualizer/log_simulation.cc b/rtc_tools/rtc_event_log_visualizer/log_simulation.cc index a1755c08ef..8362b5bf82 100644 --- a/rtc_tools/rtc_event_log_visualizer/log_simulation.cc +++ b/rtc_tools/rtc_event_log_visualizer/log_simulation.cc @@ -10,6 +10,7 @@ #include "rtc_tools/rtc_event_log_visualizer/log_simulation.h" #include +#include #include #include #include @@ -28,6 +29,7 @@ #include "logging/rtc_event_log/rtc_event_processor.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/ntp_time_util.h" +#include "modules/rtp_rtcp/source/rtp_packet_to_send.h" #include "rtc_base/network/sent_packet.h" #include "system_wrappers/include/clock.h" @@ -97,13 +99,13 @@ void LogBasedNetworkControllerSimulation::OnPacketSent( } } - RtpPacketSendInfo packet_info; - packet_info.media_ssrc = packet.ssrc; - packet_info.transport_sequence_number = packet.transport_seq_no; - packet_info.rtp_sequence_number = packet.stream_seq_no; - packet_info.length = packet.size; - packet_info.pacing_info = probe_info; - transport_feedback_.AddPacket(packet_info, packet.overhead, + RtpPacketToSend send_packet(/*extensions=*/nullptr); + send_packet.set_transport_sequence_number(packet.transport_seq_no); + send_packet.SetSsrc(packet.ssrc); + send_packet.SetSequenceNumber(packet.transport_seq_no); + send_packet.SetPayloadSize(packet.size - send_packet.headers_size()); + RTC_DCHECK_EQ(send_packet.size(), packet.size); + transport_feedback_.AddPacket(send_packet, probe_info, packet.overhead, packet.log_packet_time); } rtc::SentPacket sent_packet;