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.