From 6c032cb8356b0d3f717c4fcf50796241f2bba6c2 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Thu, 26 Jan 2023 11:19:09 +0100 Subject: [PATCH] in rtcp::TransportFeedback do not memorise all described packet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead generate such info on request Bug: None Change-Id: I8c3b54c8acdd0e3df822ecbc313ab8c232de5812 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/269251 Commit-Queue: Danil Chapovalov Reviewed-by: Björn Terelius Cr-Commit-Position: refs/heads/main@{#39207} --- .../rtc_event_log/events/logged_rtp_rtcp.h | 3 +- logging/rtc_event_log/rtc_event_log_parser.cc | 20 ++++---- .../rtp/transport_feedback_adapter.cc | 18 +++---- .../rtp/transport_feedback_demuxer.cc | 23 +++++---- .../source/rtcp_packet/transport_feedback.cc | 50 ++++++++----------- .../source/rtcp_packet/transport_feedback.h | 22 ++++---- .../transport_feedback_unittest.cc | 31 ++++++------ 7 files changed, 79 insertions(+), 88 deletions(-) diff --git a/logging/rtc_event_log/events/logged_rtp_rtcp.h b/logging/rtc_event_log/events/logged_rtp_rtcp.h index 00689a0a16..c441dea1a4 100644 --- a/logging/rtc_event_log/events/logged_rtp_rtcp.h +++ b/logging/rtc_event_log/events/logged_rtp_rtcp.h @@ -214,8 +214,7 @@ struct LoggedRtcpPacketPli { struct LoggedRtcpPacketTransportFeedback { LoggedRtcpPacketTransportFeedback() - : transport_feedback(/*include_timestamps=*/true, /*include_lost*/ true) { - } + : transport_feedback(/*include_timestamps=*/true) {} LoggedRtcpPacketTransportFeedback( Timestamp timestamp, const rtcp::TransportFeedback& transport_feedback) diff --git a/logging/rtc_event_log/rtc_event_log_parser.cc b/logging/rtc_event_log/rtc_event_log_parser.cc index 34e89ab70d..a793efb5a0 100644 --- a/logging/rtc_event_log/rtc_event_log_parser.cc +++ b/logging/rtc_event_log/rtc_event_log_parser.cc @@ -2370,28 +2370,26 @@ std::vector ParsedRtcEventLog::GetPacketInfos( last_feedback_base_time = feedback.BaseTime(); std::vector packet_feedbacks; - packet_feedbacks.reserve(feedback.GetAllPackets().size()); - Timestamp receive_timestamp = feedback_base_time; + packet_feedbacks.reserve(feedback.GetPacketStatusCount()); std::vector unknown_seq_nums; - for (const auto& packet : feedback.GetAllPackets()) { - int64_t unwrapped_seq_num = - seq_num_unwrapper.Unwrap(packet.sequence_number()); + feedback.ForAllPackets([&](uint16_t sequence_number, + TimeDelta delta_since_base) { + int64_t unwrapped_seq_num = seq_num_unwrapper.Unwrap(sequence_number); auto it = indices.find(unwrapped_seq_num); if (it == indices.end()) { unknown_seq_nums.push_back(unwrapped_seq_num); - continue; + return; } LoggedPacketInfo* sent = &packets[it->second]; if (log_feedback_time - sent->log_packet_time > TimeDelta::Seconds(60)) { RTC_LOG(LS_WARNING) << "Received very late feedback, possibly due to wraparound."; - continue; + return; } - if (packet.received()) { - receive_timestamp += packet.delta(); + if (delta_since_base.IsFinite()) { if (sent->reported_recv_time.IsInfinite()) { - sent->reported_recv_time = receive_timestamp; + sent->reported_recv_time = feedback_base_time + delta_since_base; sent->log_feedback_time = log_feedback_time; } } else { @@ -2402,7 +2400,7 @@ std::vector ParsedRtcEventLog::GetPacketInfos( } } packet_feedbacks.push_back(sent); - } + }); if (!unknown_seq_nums.empty()) { RTC_LOG(LS_WARNING) << "Received feedback for unknown packets: " diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter.cc b/modules/congestion_controller/rtp/transport_feedback_adapter.cc index d4cc915fd1..e83d09d263 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter.cc @@ -212,9 +212,10 @@ TransportFeedbackAdapter::ProcessTransportFeedbackInner( size_t failed_lookups = 0; size_t ignored = 0; - TimeDelta packet_offset = TimeDelta::Zero(); - for (const auto& packet : feedback.GetAllPackets()) { - int64_t seq_num = seq_num_unwrapper_.Unwrap(packet.sequence_number()); + + 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 @@ -229,7 +230,7 @@ TransportFeedbackAdapter::ProcessTransportFeedbackInner( auto it = history_.find(seq_num); if (it == history_.end()) { ++failed_lookups; - continue; + return; } if (it->second.sent.send_time.IsInfinite()) { @@ -237,14 +238,13 @@ TransportFeedbackAdapter::ProcessTransportFeedbackInner( // DCHECK. RTC_DLOG(LS_ERROR) << "Received feedback before packet was indicated as sent"; - continue; + return; } PacketFeedback packet_feedback = it->second; - if (packet.received()) { - packet_offset += packet.delta(); + if (delta_since_base.IsFinite()) { packet_feedback.receive_time = - current_offset_ + packet_offset.RoundDownTo(TimeDelta::Millis(1)); + 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); @@ -257,7 +257,7 @@ TransportFeedbackAdapter::ProcessTransportFeedbackInner( } else { ++ignored; } - } + }); if (failed_lookups > 0) { RTC_LOG(LS_WARNING) << "Failed to lookup send time for " << failed_lookups diff --git a/modules/congestion_controller/rtp/transport_feedback_demuxer.cc b/modules/congestion_controller/rtp/transport_feedback_demuxer.cc index b284a74f1a..469c21434a 100644 --- a/modules/congestion_controller/rtp/transport_feedback_demuxer.cc +++ b/modules/congestion_controller/rtp/transport_feedback_demuxer.cc @@ -65,17 +65,18 @@ void TransportFeedbackDemuxer::OnTransportFeedback( RTC_DCHECK_RUN_ON(&observer_checker_); std::vector stream_feedbacks; - for (const auto& packet : feedback.GetAllPackets()) { - int64_t seq_num = seq_num_unwrapper_.PeekUnwrap(packet.sequence_number()); - auto it = history_.find(seq_num); - if (it != history_.end()) { - auto packet_info = it->second; - packet_info.received = packet.received(); - stream_feedbacks.push_back(std::move(packet_info)); - if (packet.received()) - history_.erase(it); - } - } + feedback.ForAllPackets( + [&](uint16_t sequence_number, TimeDelta delta_since_base) { + RTC_DCHECK_RUN_ON(&observer_checker_); + auto it = history_.find(seq_num_unwrapper_.PeekUnwrap(sequence_number)); + if (it != history_.end()) { + auto packet_info = it->second; + packet_info.received = delta_since_base.IsFinite(); + stream_feedbacks.push_back(std::move(packet_info)); + if (delta_since_base.IsFinite()) + history_.erase(it); + } + }); for (auto& observer : observers_) { std::vector selected_feedback; diff --git a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc index bb1578fd8c..003effad29 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc +++ b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc @@ -273,11 +273,10 @@ void TransportFeedback::LastChunk::DecodeRunLength(uint16_t chunk, } TransportFeedback::TransportFeedback() - : TransportFeedback(/*include_timestamps=*/true, /*include_lost=*/true) {} + : TransportFeedback(/*include_timestamps=*/true) {} -TransportFeedback::TransportFeedback(bool include_timestamps, bool include_lost) - : include_lost_(include_lost), - base_seq_no_(0), +TransportFeedback::TransportFeedback(bool include_timestamps) + : base_seq_no_(0), num_seq_no_(0), base_time_ticks_(0), feedback_seq_(0), @@ -288,8 +287,7 @@ TransportFeedback::TransportFeedback(bool include_timestamps, bool include_lost) TransportFeedback::TransportFeedback(const TransportFeedback&) = default; TransportFeedback::TransportFeedback(TransportFeedback&& other) - : include_lost_(other.include_lost_), - base_seq_no_(other.base_seq_no_), + : base_seq_no_(other.base_seq_no_), num_seq_no_(other.num_seq_no_), base_time_ticks_(other.base_time_ticks_), feedback_seq_(other.feedback_seq_), @@ -355,11 +353,6 @@ bool TransportFeedback::AddReceivedPacket(uint16_t sequence_number, uint16_t num_missing_packets = sequence_number - next_seq_no; if (!AddMissingPackets(num_missing_packets)) return false; - if (include_lost_) { - for (; next_seq_no != sequence_number; ++next_seq_no) { - all_packets_.emplace_back(next_seq_no); - } - } } DeltaSize delta_size = (delta >= 0 && delta <= 0xff) ? 1 : 2; @@ -367,8 +360,6 @@ bool TransportFeedback::AddReceivedPacket(uint16_t sequence_number, return false; received_packets_.emplace_back(sequence_number, delta); - if (include_lost_) - all_packets_.emplace_back(sequence_number, delta); last_timestamp_ += delta * kDeltaTick; if (include_timestamps_) { size_bytes_ += delta_size; @@ -381,10 +372,22 @@ TransportFeedback::GetReceivedPackets() const { return received_packets_; } -const std::vector& -TransportFeedback::GetAllPackets() const { - RTC_DCHECK(include_lost_); - return all_packets_; +void TransportFeedback::ForAllPackets( + rtc::FunctionView handler) const { + TimeDelta delta_since_base = TimeDelta::Zero(); + auto received_it = received_packets_.begin(); + const uint16_t last_seq_num = base_seq_no_ + num_seq_no_; + for (uint16_t seq_num = base_seq_no_; seq_num != last_seq_num; ++seq_num) { + if (received_it != received_packets_.end() && + received_it->sequence_number() == seq_num) { + delta_since_base += received_it->delta(); + handler(seq_num, delta_since_base); + ++received_it; + } else { + handler(seq_num, TimeDelta::PlusInfinity()); + } + } + RTC_DCHECK(received_it == received_packets_.end()); } uint16_t TransportFeedback::GetBaseSequence() const { @@ -469,14 +472,10 @@ bool TransportFeedback::Parse(const CommonHeader& packet) { RTC_DCHECK_LE(index + delta_size, end_index); switch (delta_size) { case 0: - if (include_lost_) - all_packets_.emplace_back(seq_no); break; case 1: { int16_t delta = payload[index]; received_packets_.emplace_back(seq_no, delta); - if (include_lost_) - all_packets_.emplace_back(seq_no, delta); last_timestamp_ += delta * kDeltaTick; index += delta_size; break; @@ -484,8 +483,6 @@ bool TransportFeedback::Parse(const CommonHeader& packet) { case 2: { int16_t delta = ByteReader::ReadBigEndian(&payload[index]); received_packets_.emplace_back(seq_no, delta); - if (include_lost_) - all_packets_.emplace_back(seq_no, delta); last_timestamp_ += delta * kDeltaTick; index += delta_size; break; @@ -509,13 +506,6 @@ bool TransportFeedback::Parse(const CommonHeader& packet) { if (delta_size > 0) { received_packets_.emplace_back(seq_no, 0); } - if (include_lost_) { - if (delta_size > 0) { - all_packets_.emplace_back(seq_no, 0); - } else { - all_packets_.emplace_back(seq_no); - } - } ++seq_no; } } diff --git a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h index c580632337..4d17b54c3a 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h +++ b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h @@ -16,6 +16,7 @@ #include #include "absl/base/attributes.h" +#include "api/function_view.h" #include "api/units/time_delta.h" #include "api/units/timestamp.h" #include "modules/rtp_rtcp/source/rtcp_packet/rtpfb.h" @@ -29,23 +30,17 @@ class TransportFeedback : public Rtpfb { class ReceivedPacket { public: ReceivedPacket(uint16_t sequence_number, int16_t delta_ticks) - : sequence_number_(sequence_number), - delta_ticks_(delta_ticks), - received_(true) {} - explicit ReceivedPacket(uint16_t sequence_number) - : sequence_number_(sequence_number), received_(false) {} + : sequence_number_(sequence_number), delta_ticks_(delta_ticks) {} ReceivedPacket(const ReceivedPacket&) = default; ReceivedPacket& operator=(const ReceivedPacket&) = default; uint16_t sequence_number() const { return sequence_number_; } int16_t delta_ticks() const { return delta_ticks_; } TimeDelta delta() const { return delta_ticks_ * kDeltaTick; } - bool received() const { return received_; } private: uint16_t sequence_number_; int16_t delta_ticks_; - bool received_; }; // TODO(sprang): IANA reg? static constexpr uint8_t kFeedbackMessageType = 15; @@ -58,8 +53,7 @@ class TransportFeedback : public Rtpfb { // If `include_timestamps` is set to false, the created packet will not // contain the receive delta block. - explicit TransportFeedback(bool include_timestamps, - bool include_lost = false); + explicit TransportFeedback(bool include_timestamps); TransportFeedback(const TransportFeedback&); TransportFeedback(TransportFeedback&&); @@ -72,7 +66,14 @@ class TransportFeedback : public Rtpfb { // NOTE: This method requires increasing sequence numbers (excepting wraps). bool AddReceivedPacket(uint16_t sequence_number, Timestamp timestamp); const std::vector& GetReceivedPackets() const; - const std::vector& GetAllPackets() const; + + // Calls `handler` for all packets this feedback describes. + // For received packets pass receieve time as `delta_since_base` since the + // `BaseTime()`. For missed packets calls `handler` with `delta_since_base = + // PlusInfinity()`. + void ForAllPackets( + rtc::FunctionView handler) const; uint16_t GetBaseSequence() const; @@ -164,7 +165,6 @@ class TransportFeedback : public Rtpfb { // Adds `num_missing_packets` deltas of size 0. bool AddMissingPackets(size_t num_missing_packets); - const bool include_lost_; uint16_t base_seq_no_; uint16_t num_seq_no_; uint32_t base_time_ticks_; diff --git a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc index b3b1175305..4248a4d3ee 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc @@ -30,6 +30,9 @@ using ::testing::AllOf; using ::testing::Each; using ::testing::ElementsAreArray; using ::testing::Eq; +using ::testing::InSequence; +using ::testing::MockFunction; +using ::testing::Ne; using ::testing::Property; using ::testing::SizeIs; @@ -633,13 +636,13 @@ TEST(TransportFeedbackTest, ReportsMissingPackets) { feedback_builder.AddReceivedPacket(kBaseSeqNo + 3, kBaseTimestamp + TimeDelta::Millis(2)); - EXPECT_THAT( - Parse(feedback_builder.Build()).GetAllPackets(), - ElementsAre( - Property(&TransportFeedback::ReceivedPacket::received, true), - Property(&TransportFeedback::ReceivedPacket::received, false), - Property(&TransportFeedback::ReceivedPacket::received, false), - Property(&TransportFeedback::ReceivedPacket::received, true))); + MockFunction handler; + InSequence s; + EXPECT_CALL(handler, Call(kBaseSeqNo + 0, Ne(TimeDelta::PlusInfinity()))); + EXPECT_CALL(handler, Call(kBaseSeqNo + 1, TimeDelta::PlusInfinity())); + EXPECT_CALL(handler, Call(kBaseSeqNo + 2, TimeDelta::PlusInfinity())); + EXPECT_CALL(handler, Call(kBaseSeqNo + 3, Ne(TimeDelta::PlusInfinity()))); + Parse(feedback_builder.Build()).ForAllPackets(handler.AsStdFunction()); } TEST(TransportFeedbackTest, ReportsMissingPacketsWithoutTimestamps) { @@ -652,13 +655,13 @@ TEST(TransportFeedbackTest, ReportsMissingPacketsWithoutTimestamps) { // Packet losses indicated by jump in sequence number. feedback_builder.AddReceivedPacket(kBaseSeqNo + 3, Timestamp::Zero()); - EXPECT_THAT( - Parse(feedback_builder.Build()).GetAllPackets(), - ElementsAre( - Property(&TransportFeedback::ReceivedPacket::received, true), - Property(&TransportFeedback::ReceivedPacket::received, false), - Property(&TransportFeedback::ReceivedPacket::received, false), - Property(&TransportFeedback::ReceivedPacket::received, true))); + MockFunction handler; + InSequence s; + EXPECT_CALL(handler, Call(kBaseSeqNo + 0, Ne(TimeDelta::PlusInfinity()))); + EXPECT_CALL(handler, Call(kBaseSeqNo + 1, TimeDelta::PlusInfinity())); + EXPECT_CALL(handler, Call(kBaseSeqNo + 2, TimeDelta::PlusInfinity())); + EXPECT_CALL(handler, Call(kBaseSeqNo + 3, Ne(TimeDelta::PlusInfinity()))); + Parse(feedback_builder.Build()).ForAllPackets(handler.AsStdFunction()); } } // namespace } // namespace webrtc