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}diff --git a/modules/rtp_rtcp/source/playout_delay_oracle.cc b/modules/rtp_rtcp/source/playout_delay_oracle.cc
index dc33fad..e3e13fd 100644
--- a/modules/rtp_rtcp/source/playout_delay_oracle.cc
+++ b/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()
- : high_sequence_number_(0),
- send_playout_delay_(false),
- ssrc_(0),
- playout_delay_{-1, -1} {}
+PlayoutDelayOracle::PlayoutDelayOracle() = default;
-PlayoutDelayOracle::~PlayoutDelayOracle() {}
+PlayoutDelayOracle::~PlayoutDelayOracle() = default;
-void PlayoutDelayOracle::UpdateRequest(uint32_t ssrc,
- PlayoutDelay playout_delay,
- uint16_t seq_num) {
+absl::optional<PlayoutDelay> PlayoutDelayOracle::PlayoutDelayToSend(
+ PlayoutDelay requested_delay) const {
rtc::CritScope lock(&crit_sect_);
- RTC_DCHECK_LE(playout_delay.min_ms, PlayoutDelayLimits::kMaxMs);
- RTC_DCHECK_LE(playout_delay.max_ms, PlayoutDelayLimits::kMaxMs);
- RTC_DCHECK_LE(playout_delay.min_ms, playout_delay.max_ms);
- int64_t unwrapped_seq_num = unwrapper_.Unwrap(seq_num);
- if (playout_delay.min_ms >= 0 &&
- playout_delay.min_ms != playout_delay_.min_ms) {
- send_playout_delay_ = true;
- playout_delay_.min_ms = playout_delay.min_ms;
- high_sequence_number_ = unwrapped_seq_num;
+ if (requested_delay.min_ms > PlayoutDelayLimits::kMaxMs ||
+ requested_delay.max_ms > PlayoutDelayLimits::kMaxMs) {
+ RTC_DLOG(LS_ERROR)
+ << "Requested playout delay values out of range, ignored";
+ return absl::nullopt;
+ }
+ if (requested_delay.max_ms != -1 &&
+ requested_delay.min_ms > requested_delay.max_ms) {
+ 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 &&
- playout_delay.max_ms != playout_delay_.max_ms) {
- send_playout_delay_ = true;
- playout_delay_.max_ms = playout_delay.max_ms;
- high_sequence_number_ = unwrapped_seq_num;
+ RTC_DCHECK_LE(0, delay->min_ms);
+ RTC_DCHECK_LE(delay->max_ms, PlayoutDelayLimits::kMaxMs);
+ RTC_DCHECK_LE(delay->min_ms, delay->max_ms);
+
+ 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(
- const ReportBlockList& report_blocks) {
+void PlayoutDelayOracle::OnReceivedAck(
+ int64_t extended_highest_sequence_number) {
rtc::CritScope lock(&crit_sect_);
- for (const RTCPReportBlock& report_block : report_blocks) {
- if ((ssrc_ == report_block.source_ssrc) && send_playout_delay_ &&
- (report_block.extended_highest_sequence_number >
- high_sequence_number_)) {
- send_playout_delay_ = false;
- }
+ if (unacked_sequence_number_ &&
+ extended_highest_sequence_number > *unacked_sequence_number_) {
+ unacked_sequence_number_ = absl::nullopt;
}
}
diff --git a/modules/rtp_rtcp/source/playout_delay_oracle.h b/modules/rtp_rtcp/source/playout_delay_oracle.h
index 32f7d40..78f8747 100644
--- a/modules/rtp_rtcp/source/playout_delay_oracle.h
+++ b/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 @@
PlayoutDelayOracle();
~PlayoutDelayOracle();
- // Returns true if the current frame should include the playout delay
- // extension
- bool send_playout_delay() const {
- rtc::CritScope lock(&crit_sect_);
- return send_playout_delay_;
- }
+ // The playout delay to be added to a packet. The input delays are provided by
+ // the application, with -1 meaning unchanged/unspecified. The output delay
+ // are the values to be attached to packets on the wire. Presence and value
+ // depends on the current input, previous inputs, and received acks from the
+ // remote end.
+ absl::optional<PlayoutDelay> PlayoutDelayToSend(
+ PlayoutDelay requested_delay) const;
- // Returns current playout delay.
- PlayoutDelay playout_delay() const {
- rtc::CritScope lock(&crit_sect_);
- return playout_delay_;
- }
+ void OnSentPacket(uint16_t sequence_number,
+ absl::optional<PlayoutDelay> playout_delay);
- // Updates the application requested playout delay, current ssrc
- // and the current sequence number.
- void UpdateRequest(uint32_t ssrc,
- PlayoutDelay playout_delay,
- uint16_t seq_num);
-
- void OnReceivedRtcpReportBlocks(const ReportBlockList& report_blocks);
+ void OnReceivedAck(int64_t extended_highest_sequence_number);
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.
- int64_t high_sequence_number_ RTC_GUARDED_BY(crit_sect_);
- // Indicates whether the playout delay should go on the next frame.
- bool send_playout_delay_ RTC_GUARDED_BY(crit_sect_);
- // Sender ssrc.
- uint32_t ssrc_ RTC_GUARDED_BY(crit_sect_);
- // Sequence number unwrapper.
+ // The oldest sequence number on which the current playout delay values have
+ // been sent. When set, it means we need to attach extension to sent packets.
+ absl::optional<int64_t> unacked_sequence_number_ RTC_GUARDED_BY(crit_sect_);
+ // Sequence number unwrapper for sent packets.
+
+ // TODO(nisse): Could potentially get out of sync with the unwrapper used by
+ // 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);
};
diff --git a/modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc b/modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc
index 099339d..3857e9b 100644
--- a/modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc
+++ b/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 {
- protected:
- void ReportRTCPFeedback(int ssrc, int seq_num) {
- 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(PlayoutDelayOracleTest, DisabledByDefault) {
+ PlayoutDelayOracle playout_delay_oracle;
+ EXPECT_FALSE(playout_delay_oracle.PlayoutDelayToSend({-1, -1}));
}
-TEST_F(PlayoutDelayOracleTest, SendPlayoutDelayUntilSeqNumberExceeds) {
+TEST(PlayoutDelayOracleTest, SendPlayoutDelayUntilSeqNumberExceeds) {
+ PlayoutDelayOracle playout_delay_oracle;
PlayoutDelay playout_delay = {kMinPlayoutDelay, kMaxPlayoutDelay};
- playout_delay_oracle_.UpdateRequest(kSsrc, playout_delay, kSequenceNumber);
- EXPECT_TRUE(playout_delay_oracle_.send_playout_delay());
- EXPECT_EQ(kMinPlayoutDelay, playout_delay_oracle_.playout_delay().min_ms);
- EXPECT_EQ(kMaxPlayoutDelay, playout_delay_oracle_.playout_delay().max_ms);
+ playout_delay_oracle.OnSentPacket(kSequenceNumber, playout_delay);
+ absl::optional<PlayoutDelay> delay_to_send =
+ playout_delay_oracle.PlayoutDelayToSend({-1, -1});
+ 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);
- EXPECT_TRUE(playout_delay_oracle_.send_playout_delay());
-
- // An invalid ssrc feedback report is dropped by the oracle.
- ReportRTCPFeedback(kSsrc + 1, kSequenceNumber + 1);
- EXPECT_TRUE(playout_delay_oracle_.send_playout_delay());
+ playout_delay_oracle.OnReceivedAck(kSequenceNumber - 1);
+ EXPECT_TRUE(playout_delay_oracle.PlayoutDelayToSend({-1, -1}));
// 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);
- EXPECT_FALSE(playout_delay_oracle_.send_playout_delay());
+ playout_delay_oracle.OnReceivedAck(kSequenceNumber + 1);
+ EXPECT_FALSE(playout_delay_oracle.PlayoutDelayToSend({-1, -1}));
}
} // namespace webrtc
diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc
index 2d0a7e6..b2b6e7c 100644
--- a/modules/rtp_rtcp/source/rtp_sender.cc
+++ b/modules/rtp_rtcp/source/rtp_sender.cc
@@ -389,8 +389,17 @@
return true;
if (rtp_header) {
- playout_delay_oracle_.UpdateRequest(ssrc, rtp_header->playout_delay,
- sequence_number);
+ // TODO(nisse): This way of using PlayoutDelayOracle is a bit awkward. The
+ // 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::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 @@
packet->ReserveExtension<AbsoluteSendTime>();
packet->ReserveExtension<TransmissionOffset>();
packet->ReserveExtension<TransportSequenceNumber>();
- if (playout_delay_oracle_.send_playout_delay()) {
- packet->SetExtension<PlayoutDelayLimits>(
- playout_delay_oracle_.playout_delay());
+ absl::optional<PlayoutDelay> playout_delay =
+ playout_delay_oracle_.PlayoutDelayToSend({-1, -1});
+ if (playout_delay) {
+ packet->SetExtension<PlayoutDelayLimits>(*playout_delay);
}
if (!mid_.empty()) {
// This is a no-op if the MID header extension is not registered.
