From 2a306937183dd6d1e0427382cfa83d86a86dfe22 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Wed, 1 Jun 2022 12:39:08 +0200 Subject: [PATCH] Add functions using Timestamp and TimeDelta for rtcp::TransportFeedback BaseTime represents fixed point in time with unknown epoch and thus make sense to convert to Timestamp type, however Timestamp should always be positive. however legacy tests expect GetBaseTimeUs to return negative time sometimes. Bug: webrtc:13757 Change-Id: I3f780a7775fdd1e271402c59384c1298db76f75a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264549 Commit-Queue: Danil Chapovalov Reviewed-by: Per Kjellander Cr-Commit-Position: refs/heads/main@{#37076} --- .../source/rtcp_packet/transport_feedback.cc | 50 +++++++++++++------ .../source/rtcp_packet/transport_feedback.h | 29 +++++++++-- 2 files changed, 60 insertions(+), 19 deletions(-) diff --git a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc index 50102c21a3..3059778137 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc +++ b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc @@ -45,6 +45,7 @@ constexpr size_t kMinPayloadSizeBytes = 8 + 8 + 2; constexpr int kBaseScaleFactor = TransportFeedback::kDeltaScaleFactor * (1 << 8); constexpr int64_t kTimeWrapPeriodUs = (1ll << 24) * kBaseScaleFactor; +constexpr TimeDelta kTimeWrapPeriod = TimeDelta::Micros(kTimeWrapPeriodUs); // Message format // @@ -374,28 +375,45 @@ uint16_t TransportFeedback::GetBaseSequence() const { return base_seq_no_; } +Timestamp TransportFeedback::BaseTime() const { + return Timestamp::Zero() + + TimeDelta::Micros(int64_t{base_time_ticks_} * kBaseScaleFactor); +} + int64_t TransportFeedback::GetBaseTimeUs() const { - return static_cast(base_time_ticks_) * kBaseScaleFactor; + // Historically BaseTime was stored as signed integer and could be negative. + // However with new api it is not possible, but for compatibility with legacy + // tests return base time as negative when it used to be negative. + int64_t base_time_us = BaseTime().us() % kTimeWrapPeriodUs; + if (base_time_us >= kTimeWrapPeriodUs / 2) { + return base_time_us - kTimeWrapPeriodUs; + } else { + return base_time_us; + } } -TimeDelta TransportFeedback::GetBaseTime() const { - return TimeDelta::Micros(GetBaseTimeUs()); -} - -int64_t TransportFeedback::GetBaseDeltaUs(int64_t prev_timestamp_us) const { - int64_t delta = GetBaseTimeUs() - prev_timestamp_us; - - // Detect and compensate for wrap-arounds in base time. - if (std::abs(delta - kTimeWrapPeriodUs) < std::abs(delta)) { - delta -= kTimeWrapPeriodUs; // Wrap backwards. - } else if (std::abs(delta + kTimeWrapPeriodUs) < std::abs(delta)) { - delta += kTimeWrapPeriodUs; // Wrap forwards. +namespace { +TimeDelta CompensateForWrapAround(TimeDelta delta) { + if ((delta - kTimeWrapPeriod).Abs() < delta.Abs()) { + delta -= kTimeWrapPeriod; // Wrap backwards. + } else if ((delta + kTimeWrapPeriod).Abs() < delta.Abs()) { + delta += kTimeWrapPeriod; // Wrap forwards. } return delta; } +} // namespace + +int64_t TransportFeedback::GetBaseDeltaUs(int64_t prev_timestamp_us) const { + int64_t delta_us = GetBaseTimeUs() - prev_timestamp_us; + return CompensateForWrapAround(TimeDelta::Micros(delta_us)).us(); +} TimeDelta TransportFeedback::GetBaseDelta(TimeDelta prev_timestamp) const { - return TimeDelta::Micros(GetBaseDeltaUs(prev_timestamp.us())); + return CompensateForWrapAround(GetBaseTime() - prev_timestamp); +} + +TimeDelta TransportFeedback::GetBaseDelta(Timestamp prev_timestamp) const { + return CompensateForWrapAround(BaseTime() - prev_timestamp); } // De-serialize packet. @@ -417,7 +435,7 @@ bool TransportFeedback::Parse(const CommonHeader& packet) { base_seq_no_ = ByteReader::ReadBigEndian(&payload[8]); uint16_t status_count = ByteReader::ReadBigEndian(&payload[10]); - base_time_ticks_ = ByteReader::ReadBigEndian(&payload[12]); + base_time_ticks_ = ByteReader::ReadBigEndian(&payload[12]); feedback_seq_ = payload[15]; Clear(); size_t index = 16; @@ -627,7 +645,7 @@ bool TransportFeedback::Create(uint8_t* packet, ByteWriter::WriteBigEndian(&packet[*position], num_seq_no_); *position += 2; - ByteWriter::WriteBigEndian(&packet[*position], base_time_ticks_); + ByteWriter::WriteBigEndian(&packet[*position], base_time_ticks_); *position += 3; packet[(*position)++] = feedback_seq_; diff --git a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h index e30d338154..21633743d3 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h +++ b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h @@ -14,13 +14,17 @@ #include #include +#include "absl/base/attributes.h" #include "api/units/time_delta.h" +#include "api/units/timestamp.h" #include "modules/rtp_rtcp/source/rtcp_packet/rtpfb.h" namespace webrtc { namespace rtcp { class CommonHeader; +// TODO(bugs.webrtc.org/13757): Uncomment ABSL_DEPRECATED attributes or delete +// functions they are attached to when all usage within webrtc is updated. class TransportFeedback : public Rtpfb { public: class ReceivedPacket { @@ -36,6 +40,7 @@ class TransportFeedback : public Rtpfb { uint16_t sequence_number() const { return sequence_number_; } int16_t delta_ticks() const { return delta_ticks_; } + // ABSL_DEPRECATED("Use delta() that returns TimeDelta") int32_t delta_us() const { return delta_ticks_ * kDeltaScaleFactor; } TimeDelta delta() const { return TimeDelta::Micros(delta_us()); } bool received() const { return received_; } @@ -48,7 +53,9 @@ class TransportFeedback : public Rtpfb { // TODO(sprang): IANA reg? static constexpr uint8_t kFeedbackMessageType = 15; // Convert to multiples of 0.25ms. + // ABSL_DEPRECATED("Use kDeltaTick") static constexpr int kDeltaScaleFactor = 250; + static constexpr TimeDelta kDeltaTick = TimeDelta::Micros(250); // Maximum number of packets (including missing) TransportFeedback can report. static constexpr size_t kMaxReportedPackets = 0xffff; @@ -63,11 +70,21 @@ class TransportFeedback : public Rtpfb { ~TransportFeedback() override; + // ABSL_DEPRECATED("Use version that takes Timestamp") void SetBase(uint16_t base_sequence, // Seq# of first packet in this msg. int64_t ref_timestamp_us); // Reference timestamp for this msg. + void SetBase(uint16_t base_sequence, // Seq# of first packet in this msg. + Timestamp ref_timestamp) { // Reference timestamp for this msg. + SetBase(base_sequence, ref_timestamp.us()); + } + void SetFeedbackSequenceNumber(uint8_t feedback_sequence); // NOTE: This method requires increasing sequence numbers (excepting wraps). + // ABSL_DEPRECATED("Use version that takes Timestamp") bool AddReceivedPacket(uint16_t sequence_number, int64_t timestamp_us); + bool AddReceivedPacket(uint16_t sequence_number, Timestamp timestamp) { + return AddReceivedPacket(sequence_number, timestamp.us()); + } const std::vector& GetReceivedPackets() const; const std::vector& GetAllPackets() const; @@ -76,13 +93,19 @@ class TransportFeedback : public Rtpfb { // Returns number of packets (including missing) this feedback describes. size_t GetPacketStatusCount() const { return num_seq_no_; } - // Get the reference time in microseconds, including any precision loss. + // Get the reference time including any precision loss. + // ABSL_DEPRECATED("Use BaseTime that returns Timestamp") int64_t GetBaseTimeUs() const; - TimeDelta GetBaseTime() const; + // ABSL_DEPRECATED("Use BaseTime that returns Timestamp") + TimeDelta GetBaseTime() const { return BaseTime() - Timestamp::Zero(); } + Timestamp BaseTime() const; // Get the unwrapped delta between current base time and `prev_timestamp_us`. + // ABSL_DEPRECATED("Use GetBaseDelta that takes Timestamp") int64_t GetBaseDeltaUs(int64_t prev_timestamp_us) const; + // ABSL_DEPRECATED("Use GetBaseDelta that takes Timestamp") TimeDelta GetBaseDelta(TimeDelta prev_timestamp) const; + TimeDelta GetBaseDelta(Timestamp prev_timestamp) const; // Does the feedback packet contain timestamp information? bool IncludeTimestamps() const { return include_timestamps_; } @@ -162,7 +185,7 @@ class TransportFeedback : public Rtpfb { const bool include_lost_; uint16_t base_seq_no_; uint16_t num_seq_no_; - int32_t base_time_ticks_; + uint32_t base_time_ticks_; uint8_t feedback_seq_; bool include_timestamps_;