Refactor PlayoutDelayOracle with separate update methods

There's now one const method PlayoutDelayToSend to produce the delay
values to insert into outgoing packets, and two update methods,
OnSentPacket, and OnReceivedAck, to observe outgoing packets and acks,
respectively.

Bug: webrtc:7135
Change-Id: I07498c30f0de87ae0113f7e2eb6357a091a1f0af
Reviewed-on: https://webrtc-review.googlesource.com/c/120603
Commit-Queue: Niels Moller <nisse@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26474}

modules/rtp_rtcp/source/playout_delay_oracle.cc

@@ -8,57 +8,82 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
+#include <algorithm>
+
 #include "modules/rtp_rtcp/source/playout_delay_oracle.h"
 
-#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "modules/rtp_rtcp/source/rtp_header_extensions.h"
 #include "rtc_base/checks.h"
+#include "rtc_base/logging.h"
 
 namespace webrtc {
 
-PlayoutDelayOracle::PlayoutDelayOracle()
+PlayoutDelayOracle::PlayoutDelayOracle() = default;
-    : high_sequence_number_(0),
-      send_playout_delay_(false),
-      ssrc_(0),
-      playout_delay_{-1, -1} {}
 
-PlayoutDelayOracle::~PlayoutDelayOracle() {}
+PlayoutDelayOracle::~PlayoutDelayOracle() = default;
 
-void PlayoutDelayOracle::UpdateRequest(uint32_t ssrc,
+absl::optional<PlayoutDelay> PlayoutDelayOracle::PlayoutDelayToSend(
-                                       PlayoutDelay playout_delay,
+    PlayoutDelay requested_delay) const {
-                                       uint16_t seq_num) {
   rtc::CritScope lock(&crit_sect_);
-  RTC_DCHECK_LE(playout_delay.min_ms, PlayoutDelayLimits::kMaxMs);
+  if (requested_delay.min_ms > PlayoutDelayLimits::kMaxMs ||
-  RTC_DCHECK_LE(playout_delay.max_ms, PlayoutDelayLimits::kMaxMs);
+      requested_delay.max_ms > PlayoutDelayLimits::kMaxMs) {
-  RTC_DCHECK_LE(playout_delay.min_ms, playout_delay.max_ms);
+    RTC_DLOG(LS_ERROR)
-  int64_t unwrapped_seq_num = unwrapper_.Unwrap(seq_num);
+        << "Requested playout delay values out of range, ignored";
-  if (playout_delay.min_ms >= 0 &&
+    return absl::nullopt;
-      playout_delay.min_ms != playout_delay_.min_ms) {
+  }
-    send_playout_delay_ = true;
+  if (requested_delay.max_ms != -1 &&
-    playout_delay_.min_ms = playout_delay.min_ms;
+      requested_delay.min_ms > requested_delay.max_ms) {
-    high_sequence_number_ = unwrapped_seq_num;
+    RTC_DLOG(LS_ERROR) << "Requested playout delay values out of order";
+    return absl::nullopt;
+  }
+  if ((requested_delay.min_ms == -1 ||
+       requested_delay.min_ms == latest_delay_.min_ms) &&
+      (requested_delay.max_ms == -1 ||
+       requested_delay.max_ms == latest_delay_.max_ms)) {
+    // Unchanged.
+    return unacked_sequence_number_ ? absl::make_optional(latest_delay_)
+                                    : absl::nullopt;
+  }
+  if (requested_delay.min_ms == -1) {
+    RTC_DCHECK_GE(requested_delay.max_ms, 0);
+    requested_delay.min_ms =
+        std::min(latest_delay_.min_ms, requested_delay.max_ms);
+  }
+  if (requested_delay.max_ms == -1) {
+    requested_delay.max_ms =
+        std::max(latest_delay_.max_ms, requested_delay.min_ms);
+  }
+  return requested_delay;
+}
+
+void PlayoutDelayOracle::OnSentPacket(uint16_t sequence_number,
+                                      absl::optional<PlayoutDelay> delay) {
+  rtc::CritScope lock(&crit_sect_);
+  int64_t unwrapped_sequence_number = unwrapper_.Unwrap(sequence_number);
+
+  if (!delay) {
+    return;
   }
 
-  if (playout_delay.max_ms >= 0 &&
+  RTC_DCHECK_LE(0, delay->min_ms);
-      playout_delay.max_ms != playout_delay_.max_ms) {
+  RTC_DCHECK_LE(delay->max_ms, PlayoutDelayLimits::kMaxMs);
-    send_playout_delay_ = true;
+  RTC_DCHECK_LE(delay->min_ms, delay->max_ms);
-    playout_delay_.max_ms = playout_delay.max_ms;
+
-    high_sequence_number_ = unwrapped_seq_num;
+  if (delay->min_ms != latest_delay_.min_ms ||
+      delay->max_ms != latest_delay_.max_ms) {
+    latest_delay_ = *delay;
+    unacked_sequence_number_ = unwrapped_sequence_number;
   }
-  ssrc_ = ssrc;
 }
 
 // If an ACK is received on the packet containing the playout delay extension,
 // we stop sending the extension on future packets.
-void PlayoutDelayOracle::OnReceivedRtcpReportBlocks(
+void PlayoutDelayOracle::OnReceivedAck(
-    const ReportBlockList& report_blocks) {
+    int64_t extended_highest_sequence_number) {
   rtc::CritScope lock(&crit_sect_);
-  for (const RTCPReportBlock& report_block : report_blocks) {
+  if (unacked_sequence_number_ &&
-    if ((ssrc_ == report_block.source_ssrc) && send_playout_delay_ &&
+      extended_highest_sequence_number > *unacked_sequence_number_) {
-        (report_block.extended_highest_sequence_number >
+    unacked_sequence_number_ = absl::nullopt;
-         high_sequence_number_)) {
-      send_playout_delay_ = false;
-    }
   }
 }
 

modules/rtp_rtcp/source/playout_delay_oracle.h

@@ -13,9 +13,9 @@
 
 #include <stdint.h>
 
+#include "absl/types/optional.h"
 #include "common_types.h"  // NOLINT(build/include)
 #include "modules/include/module_common_types_public.h"
-#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "rtc_base/constructor_magic.h"
 #include "rtc_base/critical_section.h"
 #include "rtc_base/thread_annotations.h"
@@ -38,42 +38,34 @@ class PlayoutDelayOracle {
   PlayoutDelayOracle();
   ~PlayoutDelayOracle();
 
-  // Returns true if the current frame should include the playout delay
+  // The playout delay to be added to a packet. The input delays are provided by
-  // extension
+  // the application, with -1 meaning unchanged/unspecified. The output delay
-  bool send_playout_delay() const {
+  // are the values to be attached to packets on the wire. Presence and value
-    rtc::CritScope lock(&crit_sect_);
+  // depends on the current input, previous inputs, and received acks from the
-    return send_playout_delay_;
+  // remote end.
-  }
+  absl::optional<PlayoutDelay> PlayoutDelayToSend(
+      PlayoutDelay requested_delay) const;
 
-  // Returns current playout delay.
+  void OnSentPacket(uint16_t sequence_number,
-  PlayoutDelay playout_delay() const {
+                    absl::optional<PlayoutDelay> playout_delay);
-    rtc::CritScope lock(&crit_sect_);
-    return playout_delay_;
-  }
 
-  // Updates the application requested playout delay, current ssrc
+  void OnReceivedAck(int64_t extended_highest_sequence_number);
-  // and the current sequence number.
-  void UpdateRequest(uint32_t ssrc,
-                     PlayoutDelay playout_delay,
-                     uint16_t seq_num);
-
-  void OnReceivedRtcpReportBlocks(const ReportBlockList& report_blocks);
 
  private:
   // The playout delay information is updated from the encoder thread(s).
   // The sequence number feedback is updated from the worker thread.
   // Guards access to data across multiple threads.
   rtc::CriticalSection crit_sect_;
-  // The current highest sequence number on which playout delay has been sent.
+  // The oldest sequence number on which the current playout delay values have
-  int64_t high_sequence_number_ RTC_GUARDED_BY(crit_sect_);
+  // been sent. When set, it means we need to attach extension to sent packets.
-  // Indicates whether the playout delay should go on the next frame.
+  absl::optional<int64_t> unacked_sequence_number_ RTC_GUARDED_BY(crit_sect_);
-  bool send_playout_delay_ RTC_GUARDED_BY(crit_sect_);
+  // Sequence number unwrapper for sent packets.
-  // Sender ssrc.
+
-  uint32_t ssrc_ RTC_GUARDED_BY(crit_sect_);
+  // TODO(nisse): Could potentially get out of sync with the unwrapper used by
-  // Sequence number unwrapper.
+  // the caller of OnReceivedAck.
   SequenceNumberUnwrapper unwrapper_ RTC_GUARDED_BY(crit_sect_);
   // Playout delay values on the next frame if |send_playout_delay_| is set.
-  PlayoutDelay playout_delay_ RTC_GUARDED_BY(crit_sect_);
+  PlayoutDelay latest_delay_ RTC_GUARDED_BY(crit_sect_) = {-1, -1};
 
   RTC_DISALLOW_COPY_AND_ASSIGN(PlayoutDelayOracle);
 };

modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc

@@ -16,54 +16,37 @@
 namespace webrtc {
 
 namespace {
-constexpr int kSsrc = 100;
 constexpr int kSequenceNumber = 100;
 constexpr int kMinPlayoutDelay = 0;
 constexpr int kMaxPlayoutDelay = 150;
 }  // namespace
 
-class PlayoutDelayOracleTest : public ::testing::Test {
+TEST(PlayoutDelayOracleTest, DisabledByDefault) {
- protected:
+  PlayoutDelayOracle playout_delay_oracle;
-  void ReportRTCPFeedback(int ssrc, int seq_num) {
+  EXPECT_FALSE(playout_delay_oracle.PlayoutDelayToSend({-1, -1}));
-    RTCPReportBlock report_block;
-    report_block.source_ssrc = ssrc;
-    report_block.extended_highest_sequence_number = seq_num;
-    report_blocks_.push_back(report_block);
-    playout_delay_oracle_.OnReceivedRtcpReportBlocks(report_blocks_);
-  }
-
-  ReportBlockList report_blocks_;
-  PlayoutDelayOracle playout_delay_oracle_;
-};
-
-TEST_F(PlayoutDelayOracleTest, DisabledByDefault) {
-  EXPECT_FALSE(playout_delay_oracle_.send_playout_delay());
-  EXPECT_EQ(-1, playout_delay_oracle_.playout_delay().min_ms);
-  EXPECT_EQ(-1, playout_delay_oracle_.playout_delay().max_ms);
 }
 
-TEST_F(PlayoutDelayOracleTest, SendPlayoutDelayUntilSeqNumberExceeds) {
+TEST(PlayoutDelayOracleTest, SendPlayoutDelayUntilSeqNumberExceeds) {
+  PlayoutDelayOracle playout_delay_oracle;
   PlayoutDelay playout_delay = {kMinPlayoutDelay, kMaxPlayoutDelay};
-  playout_delay_oracle_.UpdateRequest(kSsrc, playout_delay, kSequenceNumber);
+  playout_delay_oracle.OnSentPacket(kSequenceNumber, playout_delay);
-  EXPECT_TRUE(playout_delay_oracle_.send_playout_delay());
+  absl::optional<PlayoutDelay> delay_to_send =
-  EXPECT_EQ(kMinPlayoutDelay, playout_delay_oracle_.playout_delay().min_ms);
+      playout_delay_oracle.PlayoutDelayToSend({-1, -1});
-  EXPECT_EQ(kMaxPlayoutDelay, playout_delay_oracle_.playout_delay().max_ms);
+  ASSERT_TRUE(delay_to_send.has_value());
+  EXPECT_EQ(kMinPlayoutDelay, delay_to_send->min_ms);
+  EXPECT_EQ(kMaxPlayoutDelay, delay_to_send->max_ms);
 
   // Oracle indicates playout delay should be sent if highest sequence number
   // acked is lower than the sequence number of the first packet containing
   // playout delay.
-  ReportRTCPFeedback(kSsrc, kSequenceNumber - 1);
+  playout_delay_oracle.OnReceivedAck(kSequenceNumber - 1);
-  EXPECT_TRUE(playout_delay_oracle_.send_playout_delay());
+  EXPECT_TRUE(playout_delay_oracle.PlayoutDelayToSend({-1, -1}));
-
-  // An invalid ssrc feedback report is dropped by the oracle.
-  ReportRTCPFeedback(kSsrc + 1, kSequenceNumber + 1);
-  EXPECT_TRUE(playout_delay_oracle_.send_playout_delay());
 
   // Oracle indicates playout delay should not be sent if sequence number
   // acked on a matching ssrc indicates the receiver has received the playout
   // delay values.
-  ReportRTCPFeedback(kSsrc, kSequenceNumber + 1);
+  playout_delay_oracle.OnReceivedAck(kSequenceNumber + 1);
-  EXPECT_FALSE(playout_delay_oracle_.send_playout_delay());
+  EXPECT_FALSE(playout_delay_oracle.PlayoutDelayToSend({-1, -1}));
 }
 
 }  // namespace webrtc

modules/rtp_rtcp/source/rtp_sender.cc

@@ -389,8 +389,17 @@ bool RTPSender::SendOutgoingData(FrameType frame_type,
       return true;
 
     if (rtp_header) {
-      playout_delay_oracle_.UpdateRequest(ssrc, rtp_header->playout_delay,
+      // TODO(nisse): This way of using PlayoutDelayOracle is a bit awkward. The
-                                          sequence_number);
+      // intended way to use it is to call PlayoutDelayToSend at the place where
+      // the extension is written into the packet, and OnSentPacket later, after
+      // the final allocation of the sequence number. But currently the
+      // extension is set in AllocatePacket, where the RTPVideoHeader isn't
+      // available, so it always calls PlayoutDelayToSend with {-1,-1}. Hence,
+      // we have to use OnSentPacket early, and record both contents of the
+      // extension and the sequence number.
+      playout_delay_oracle_.OnSentPacket(
+          sequence_number,
+          playout_delay_oracle_.PlayoutDelayToSend(rtp_header->playout_delay));
     }
 
     result = video_->SendVideo(frame_type, payload_type, rtp_timestamp,
@@ -653,7 +662,23 @@ void RTPSender::OnReceivedNack(
 
 void RTPSender::OnReceivedRtcpReportBlocks(
     const ReportBlockList& report_blocks) {
-  playout_delay_oracle_.OnReceivedRtcpReportBlocks(report_blocks);
+  if (!video_) {
+    return;
+  }
+  uint32_t ssrc;
+  {
+    rtc::CritScope lock(&send_critsect_);
+    if (!ssrc_)
+      return;
+    ssrc = *ssrc_;
+  }
+
+  for (const RTCPReportBlock& report_block : report_blocks) {
+    if (ssrc == report_block.source_ssrc) {
+      playout_delay_oracle_.OnReceivedAck(
+          report_block.extended_highest_sequence_number);
+    }
+  }
 }
 
 // Called from pacer when we can send the packet.
@@ -1050,9 +1075,10 @@ std::unique_ptr<RtpPacketToSend> RTPSender::AllocatePacket() const {
   packet->ReserveExtension<AbsoluteSendTime>();
   packet->ReserveExtension<TransmissionOffset>();
   packet->ReserveExtension<TransportSequenceNumber>();
-  if (playout_delay_oracle_.send_playout_delay()) {
+  absl::optional<PlayoutDelay> playout_delay =
-    packet->SetExtension<PlayoutDelayLimits>(
+      playout_delay_oracle_.PlayoutDelayToSend({-1, -1});
-        playout_delay_oracle_.playout_delay());
+  if (playout_delay) {
+    packet->SetExtension<PlayoutDelayLimits>(*playout_delay);
   }
   if (!mid_.empty()) {
     // This is a no-op if the MID header extension is not registered.