From 71f94c93a63d8a18f8dc0b71ac3cffc005d789ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Wed, 30 Jan 2019 16:56:10 +0100 Subject: [PATCH] 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 Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#26474} --- .../rtp_rtcp/source/playout_delay_oracle.cc | 91 ++++++++++++------- .../rtp_rtcp/source/playout_delay_oracle.h | 46 ++++------ .../source/playout_delay_oracle_unittest.cc | 47 +++------- modules/rtp_rtcp/source/rtp_sender.cc | 38 ++++++-- 4 files changed, 124 insertions(+), 98 deletions(-) diff --git a/modules/rtp_rtcp/source/playout_delay_oracle.cc b/modules/rtp_rtcp/source/playout_delay_oracle.cc index dc33fad536..e3e13fdabc 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 + #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 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 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 32f7d403ff..78f8747879 100644 --- a/modules/rtp_rtcp/source/playout_delay_oracle.h +++ b/modules/rtp_rtcp/source/playout_delay_oracle.h @@ -13,9 +13,9 @@ #include +#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 - // 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 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 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 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 099339d6d4..3857e9b211 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 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 2d0a7e6f5c..b2b6e7cd26 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/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, - 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::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 RTPSender::AllocatePacket() const { packet->ReserveExtension(); packet->ReserveExtension(); packet->ReserveExtension(); - if (playout_delay_oracle_.send_playout_delay()) { - packet->SetExtension( - playout_delay_oracle_.playout_delay()); + absl::optional playout_delay = + playout_delay_oracle_.PlayoutDelayToSend({-1, -1}); + if (playout_delay) { + packet->SetExtension(*playout_delay); } if (!mid_.empty()) { // This is a no-op if the MID header extension is not registered.