diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 413171fa67..3ae0794631 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -24,7 +24,6 @@ #include "modules/pacing/packet_router.h" #include "modules/rtp_rtcp/include/rtp_rtcp.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" -#include "modules/rtp_rtcp/source/playout_delay_oracle.h" #include "modules/rtp_rtcp/source/rtp_sender.h" #include "modules/utility/include/process_thread.h" #include "modules/video_coding/include/video_codec_interface.h" @@ -37,13 +36,9 @@ namespace webrtc { namespace webrtc_internal_rtp_video_sender { -RtpStreamSender::RtpStreamSender( - std::unique_ptr playout_delay_oracle, - std::unique_ptr rtp_rtcp, - std::unique_ptr sender_video) - : playout_delay_oracle(std::move(playout_delay_oracle)), - rtp_rtcp(std::move(rtp_rtcp)), - sender_video(std::move(sender_video)) {} +RtpStreamSender::RtpStreamSender(std::unique_ptr rtp_rtcp, + std::unique_ptr sender_video) + : rtp_rtcp(std::move(rtp_rtcp)), sender_video(std::move(sender_video)) {} RtpStreamSender::~RtpStreamSender() = default; @@ -177,9 +172,7 @@ std::vector CreateRtpStreamSenders( configuration.local_media_ssrc) != flexfec_protected_ssrcs.end(); configuration.flexfec_sender = enable_flexfec ? flexfec_sender : nullptr; - auto playout_delay_oracle = std::make_unique(); - configuration.ack_observer = playout_delay_oracle.get(); if (rtp_config.rtx.ssrcs.size() > i) { configuration.rtx_send_ssrc = rtp_config.rtx.ssrcs[i]; } @@ -196,7 +189,6 @@ std::vector CreateRtpStreamSenders( video_config.clock = configuration.clock; video_config.rtp_sender = rtp_rtcp->RtpSender(); video_config.flexfec_sender = configuration.flexfec_sender; - video_config.playout_delay_oracle = playout_delay_oracle.get(); video_config.frame_encryptor = frame_encryptor; video_config.require_frame_encryption = crypto_options.sframe.require_frame_encryption; @@ -214,8 +206,7 @@ std::vector CreateRtpStreamSenders( video_config.ulpfec_payload_type = rtp_config.ulpfec.ulpfec_payload_type; } auto sender_video = std::make_unique(video_config); - rtp_streams.emplace_back(std::move(playout_delay_oracle), - std::move(rtp_rtcp), std::move(sender_video)); + rtp_streams.emplace_back(std::move(rtp_rtcp), std::move(sender_video)); } return rtp_streams; } diff --git a/call/rtp_video_sender.h b/call/rtp_video_sender.h index eb7e4315be..620c975810 100644 --- a/call/rtp_video_sender.h +++ b/call/rtp_video_sender.h @@ -50,8 +50,7 @@ namespace webrtc_internal_rtp_video_sender { // RTP state for a single simulcast stream. Internal to the implementation of // RtpVideoSender. struct RtpStreamSender { - RtpStreamSender(std::unique_ptr playout_delay_oracle, - std::unique_ptr rtp_rtcp, + RtpStreamSender(std::unique_ptr rtp_rtcp, std::unique_ptr sender_video); ~RtpStreamSender(); @@ -59,7 +58,6 @@ struct RtpStreamSender { RtpStreamSender& operator=(RtpStreamSender&&) = default; // Note: Needs pointer stability. - std::unique_ptr playout_delay_oracle; std::unique_ptr rtp_rtcp; std::unique_ptr sender_video; }; diff --git a/common_types.h b/common_types.h index aadda4fb99..dedcbd5460 100644 --- a/common_types.h +++ b/common_types.h @@ -89,8 +89,16 @@ typedef SpatialLayer SimulcastStream; // Note: Given that this gets embedded in a union, it is up-to the owner to // initialize these values. struct PlayoutDelay { + PlayoutDelay(int min_ms, int max_ms) : min_ms(min_ms), max_ms(max_ms) {} int min_ms; int max_ms; + + static PlayoutDelay Noop() { return PlayoutDelay(-1, -1); } + + bool IsNoop() const { return min_ms == -1 && max_ms == -1; } + bool operator==(const PlayoutDelay& rhs) const { + return min_ms == rhs.min_ms && max_ms == rhs.max_ms; + } }; } // namespace webrtc diff --git a/modules/rtp_rtcp/BUILD.gn b/modules/rtp_rtcp/BUILD.gn index 099c0663d2..b8dd23ed86 100644 --- a/modules/rtp_rtcp/BUILD.gn +++ b/modules/rtp_rtcp/BUILD.gn @@ -156,7 +156,6 @@ rtc_library("rtp_rtcp") { "source/forward_error_correction_internal.h", "source/packet_loss_stats.cc", "source/packet_loss_stats.h", - "source/playout_delay_oracle.cc", "source/playout_delay_oracle.h", "source/receive_statistics_impl.cc", "source/receive_statistics_impl.h", @@ -429,7 +428,6 @@ if (rtc_include_tests) { "source/flexfec_sender_unittest.cc", "source/nack_rtx_unittest.cc", "source/packet_loss_stats_unittest.cc", - "source/playout_delay_oracle_unittest.cc", "source/receive_statistics_unittest.cc", "source/remote_ntp_time_estimator_unittest.cc", "source/rtcp_nack_stats_unittest.cc", diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index b3cd8f6418..fbb3bb3241 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -101,7 +101,6 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { SendPacketObserver* send_packet_observer = nullptr; RateLimiter* retransmission_rate_limiter = nullptr; OverheadObserver* overhead_observer = nullptr; - RtcpAckObserver* ack_observer = nullptr; StreamDataCountersCallback* rtp_stats_callback = nullptr; int rtcp_report_interval_ms = 0; diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index 25a9d29077..b2bda626ce 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -402,19 +402,6 @@ struct RtpReceiveStats { RtpPacketCounter packet_counter; }; -class RtcpAckObserver { - public: - // This method is called on received report blocks matching the sender ssrc. - // TODO(nisse): Use of "extended" sequence number is a bit brittle, since the - // observer for this callback typically has its own sequence number unwrapper, - // and there's no guarantee that they are in sync. Change to pass raw sequence - // number, possibly augmented with timestamp (if available) to aid - // disambiguation. - virtual void OnReceivedAck(int64_t extended_highest_sequence_number) = 0; - - virtual ~RtcpAckObserver() = default; -}; - // Callback, used to notify an observer whenever new rates have been estimated. class BitrateStatisticsObserver { public: diff --git a/modules/rtp_rtcp/source/nack_rtx_unittest.cc b/modules/rtp_rtcp/source/nack_rtx_unittest.cc index 17601dd966..55e1e44ebe 100644 --- a/modules/rtp_rtcp/source/nack_rtx_unittest.cc +++ b/modules/rtp_rtcp/source/nack_rtx_unittest.cc @@ -21,7 +21,6 @@ #include "modules/rtp_rtcp/include/receive_statistics.h" #include "modules/rtp_rtcp/include/rtp_rtcp.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" -#include "modules/rtp_rtcp/source/playout_delay_oracle.h" #include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "modules/rtp_rtcp/source/rtp_sender_video.h" #include "rtc_base/rate_limiter.h" @@ -140,7 +139,6 @@ class RtpRtcpRtxNackTest : public ::testing::Test { RTPSenderVideo::Config video_config; video_config.clock = &fake_clock; video_config.rtp_sender = rtp_rtcp_module_->RtpSender(); - video_config.playout_delay_oracle = &playout_delay_oracle_; video_config.field_trials = &field_trials; rtp_sender_video_ = std::make_unique(video_config); rtp_rtcp_module_->SetRTCPStatus(RtcpMode::kCompound); @@ -227,7 +225,6 @@ class RtpRtcpRtxNackTest : public ::testing::Test { std::unique_ptr receive_statistics_; std::unique_ptr rtp_rtcp_module_; - PlayoutDelayOracle playout_delay_oracle_; std::unique_ptr rtp_sender_video_; RtxLoopBackTransport transport_; const std::map rtx_associated_payload_types_ = { diff --git a/modules/rtp_rtcp/source/playout_delay_oracle.cc b/modules/rtp_rtcp/source/playout_delay_oracle.cc deleted file mode 100644 index f234759678..0000000000 --- a/modules/rtp_rtcp/source/playout_delay_oracle.cc +++ /dev/null @@ -1,90 +0,0 @@ -/* - * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "modules/rtp_rtcp/source/playout_delay_oracle.h" - -#include - -#include "modules/rtp_rtcp/source/rtp_header_extensions.h" -#include "rtc_base/checks.h" -#include "rtc_base/logging.h" - -namespace webrtc { - -PlayoutDelayOracle::PlayoutDelayOracle() = default; - -PlayoutDelayOracle::~PlayoutDelayOracle() = default; - -absl::optional PlayoutDelayOracle::PlayoutDelayToSend( - PlayoutDelay requested_delay) const { - rtc::CritScope lock(&crit_sect_); - 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; - } - - 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; - } -} - -// If an ACK is received on the packet containing the playout delay extension, -// we stop sending the extension on future packets. -void PlayoutDelayOracle::OnReceivedAck( - int64_t extended_highest_sequence_number) { - rtc::CritScope lock(&crit_sect_); - if (unacked_sequence_number_ && - extended_highest_sequence_number > *unacked_sequence_number_) { - unacked_sequence_number_ = absl::nullopt; - } -} - -} // namespace webrtc diff --git a/modules/rtp_rtcp/source/playout_delay_oracle.h b/modules/rtp_rtcp/source/playout_delay_oracle.h index 6451be4cdc..04465e3cfc 100644 --- a/modules/rtp_rtcp/source/playout_delay_oracle.h +++ b/modules/rtp_rtcp/source/playout_delay_oracle.h @@ -11,64 +11,12 @@ #ifndef MODULES_RTP_RTCP_SOURCE_PLAYOUT_DELAY_ORACLE_H_ #define MODULES_RTP_RTCP_SOURCE_PLAYOUT_DELAY_ORACLE_H_ -#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" - namespace webrtc { -// This class tracks the application requests to limit minimum and maximum -// playout delay and makes a decision on whether the current RTP frame -// should include the playout out delay extension header. -// -// Playout delay can be defined in terms of capture and render time as follows: -// -// Render time = Capture time in receiver time + playout delay -// -// The application specifies a minimum and maximum limit for the playout delay -// which are both communicated to the receiver and the receiver can adapt -// the playout delay within this range based on observed network jitter. -class PlayoutDelayOracle : public RtcpAckObserver { +// TODO(sprang): Remove once downstream usage is gone. +class PlayoutDelayOracle { public: - PlayoutDelayOracle(); - ~PlayoutDelayOracle() override; - - // 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; - - void OnSentPacket(uint16_t sequence_number, - absl::optional playout_delay); - - void OnReceivedAck(int64_t extended_highest_sequence_number) override; - - 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 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 latest_delay_ RTC_GUARDED_BY(crit_sect_) = {-1, -1}; - - RTC_DISALLOW_COPY_AND_ASSIGN(PlayoutDelayOracle); + PlayoutDelayOracle() = default; }; } // namespace webrtc diff --git a/modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc b/modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc deleted file mode 100644 index 3857e9b211..0000000000 --- a/modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc +++ /dev/null @@ -1,52 +0,0 @@ -/* - * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "modules/rtp_rtcp/source/playout_delay_oracle.h" - -#include "rtc_base/logging.h" -#include "test/gtest.h" - -namespace webrtc { - -namespace { -constexpr int kSequenceNumber = 100; -constexpr int kMinPlayoutDelay = 0; -constexpr int kMaxPlayoutDelay = 150; -} // namespace - -TEST(PlayoutDelayOracleTest, DisabledByDefault) { - PlayoutDelayOracle playout_delay_oracle; - EXPECT_FALSE(playout_delay_oracle.PlayoutDelayToSend({-1, -1})); -} - -TEST(PlayoutDelayOracleTest, SendPlayoutDelayUntilSeqNumberExceeds) { - PlayoutDelayOracle playout_delay_oracle; - PlayoutDelay playout_delay = {kMinPlayoutDelay, kMaxPlayoutDelay}; - 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. - 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. - playout_delay_oracle.OnReceivedAck(kSequenceNumber + 1); - EXPECT_FALSE(playout_delay_oracle.PlayoutDelayToSend({-1, -1})); -} - -} // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 987ae0ec59..dfbac29d03 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -68,7 +68,6 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration) nack_last_time_sent_full_ms_(0), nack_last_seq_number_sent_(0), remote_bitrate_(configuration.remote_bitrate_estimator), - ack_observer_(configuration.ack_observer), rtt_stats_(configuration.rtt_stats), rtt_ms_(0) { if (!configuration.receiver_only) { @@ -736,7 +735,7 @@ void ModuleRtpRtcpImpl::OnReceivedNack( void ModuleRtpRtcpImpl::OnReceivedRtcpReportBlocks( const ReportBlockList& report_blocks) { - if (ack_observer_) { + if (rtp_sender_) { uint32_t ssrc = SSRC(); absl::optional rtx_ssrc; if (rtp_sender_->packet_generator.RtxStatus() != kRtxOff) { @@ -747,8 +746,6 @@ void ModuleRtpRtcpImpl::OnReceivedRtcpReportBlocks( if (ssrc == report_block.source_ssrc) { rtp_sender_->packet_generator.OnReceivedAckOnSsrc( report_block.extended_highest_sequence_number); - ack_observer_->OnReceivedAck( - report_block.extended_highest_sequence_number); } else if (rtx_ssrc && *rtx_ssrc == report_block.source_ssrc) { rtp_sender_->packet_generator.OnReceivedAckOnRtxSsrc( report_block.extended_highest_sequence_number); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 976653a458..c03683f48e 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -340,8 +340,6 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { RemoteBitrateEstimator* const remote_bitrate_; - RtcpAckObserver* const ack_observer_; - RtcpRttStats* const rtt_stats_; // The processed RTT from RtcpRttStats. diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc index 0b681cf183..5e4cce99a7 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc @@ -17,7 +17,6 @@ #include "api/transport/field_trial_based_config.h" #include "api/video_codecs/video_codec.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" -#include "modules/rtp_rtcp/source/playout_delay_oracle.h" #include "modules/rtp_rtcp/source/rtcp_packet.h" #include "modules/rtp_rtcp/source/rtcp_packet/nack.h" #include "modules/rtp_rtcp/source/rtp_packet_received.h" @@ -182,7 +181,6 @@ class RtpRtcpImplTest : public ::testing::Test { RTPSenderVideo::Config video_config; video_config.clock = &clock_; video_config.rtp_sender = sender_.impl_->RtpSender(); - video_config.playout_delay_oracle = &playout_delay_oracle_; video_config.field_trials = &field_trials; sender_video_ = std::make_unique(video_config); @@ -201,7 +199,6 @@ class RtpRtcpImplTest : public ::testing::Test { SimulatedClock clock_; RtpRtcpModule sender_; - PlayoutDelayOracle playout_delay_oracle_; std::unique_ptr sender_video_; RtpRtcpModule receiver_; VideoCodec codec_; diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 69a09d8183..c3ae539071 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -649,12 +649,10 @@ TEST_P(RtpSenderTestWithoutPacer, OnSendSideDelayUpdated) { config.event_log = &mock_rtc_event_log_; rtp_sender_context_ = std::make_unique(config); - PlayoutDelayOracle playout_delay_oracle; FieldTrialBasedConfig field_trials; RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); - video_config.playout_delay_oracle = &playout_delay_oracle; video_config.field_trials = &field_trials; RTPSenderVideo rtp_sender_video(video_config); @@ -1150,12 +1148,10 @@ TEST_P(RtpSenderTest, OnSendPacketNotUpdatedForRetransmits) { TEST_P(RtpSenderTestWithoutPacer, SendGenericVideo) { const uint8_t kPayloadType = 127; const VideoCodecType kCodecType = VideoCodecType::kVideoCodecGeneric; - PlayoutDelayOracle playout_delay_oracle; FieldTrialBasedConfig field_trials; RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); - video_config.playout_delay_oracle = &playout_delay_oracle; video_config.field_trials = &field_trials; RTPSenderVideo rtp_sender_video(video_config); uint8_t payload[] = {47, 11, 32, 93, 89}; @@ -1194,12 +1190,10 @@ TEST_P(RtpSenderTestWithoutPacer, SendRawVideo) { const uint8_t kPayloadType = 111; const uint8_t payload[] = {11, 22, 33, 44, 55}; - PlayoutDelayOracle playout_delay_oracle; FieldTrialBasedConfig field_trials; RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); - video_config.playout_delay_oracle = &playout_delay_oracle; video_config.field_trials = &field_trials; RTPSenderVideo rtp_sender_video(video_config); @@ -1241,13 +1235,11 @@ TEST_P(RtpSenderTest, SendFlexfecPackets) { rtp_sender_context_->packet_history_.SetStorePacketsStatus( RtpPacketHistory::StorageMode::kStoreAndCull, 10); - PlayoutDelayOracle playout_delay_oracle; FieldTrialBasedConfig field_trials; RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); video_config.flexfec_sender = &flexfec_sender; - video_config.playout_delay_oracle = &playout_delay_oracle; video_config.field_trials = &field_trials; RTPSenderVideo rtp_sender_video(video_config); @@ -1327,13 +1319,11 @@ TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) { rtp_sender()->SetSequenceNumber(kSeqNum); - PlayoutDelayOracle playout_delay_oracle; FieldTrialBasedConfig field_trials; RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); video_config.flexfec_sender = &flexfec_sender; - video_config.playout_delay_oracle = &playout_delay_oracle; video_config.field_trials = &field_trials; RTPSenderVideo rtp_sender_video(video_config); @@ -1601,13 +1591,11 @@ TEST_P(RtpSenderTest, FecOverheadRate) { rtp_sender()->SetSequenceNumber(kSeqNum); - PlayoutDelayOracle playout_delay_oracle; FieldTrialBasedConfig field_trials; RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); video_config.flexfec_sender = &flexfec_sender; - video_config.playout_delay_oracle = &playout_delay_oracle; video_config.field_trials = &field_trials; RTPSenderVideo rtp_sender_video(video_config); // Parameters selected to generate a single FEC packet per media packet. @@ -1677,12 +1665,10 @@ TEST_P(RtpSenderTest, BitrateCallbacks) { config.retransmission_rate_limiter = &retransmission_rate_limiter_; rtp_sender_context_ = std::make_unique(config); - PlayoutDelayOracle playout_delay_oracle; FieldTrialBasedConfig field_trials; RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); - video_config.playout_delay_oracle = &playout_delay_oracle; video_config.field_trials = &field_trials; RTPSenderVideo rtp_sender_video(video_config); const VideoCodecType kCodecType = VideoCodecType::kVideoCodecGeneric; @@ -1735,12 +1721,10 @@ TEST_P(RtpSenderTest, BitrateCallbacks) { TEST_P(RtpSenderTestWithoutPacer, StreamDataCountersCallbacks) { const uint8_t kPayloadType = 127; const VideoCodecType kCodecType = VideoCodecType::kVideoCodecGeneric; - PlayoutDelayOracle playout_delay_oracle; FieldTrialBasedConfig field_trials; RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); - video_config.playout_delay_oracle = &playout_delay_oracle; video_config.field_trials = &field_trials; RTPSenderVideo rtp_sender_video(video_config); uint8_t payload[] = {47, 11, 32, 93, 89}; @@ -1792,12 +1776,10 @@ TEST_P(RtpSenderTestWithoutPacer, StreamDataCountersCallbacksUlpfec) { const uint8_t kUlpfecPayloadType = 97; const uint8_t kPayloadType = 127; const VideoCodecType kCodecType = VideoCodecType::kVideoCodecGeneric; - PlayoutDelayOracle playout_delay_oracle; FieldTrialBasedConfig field_trials; RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); - video_config.playout_delay_oracle = &playout_delay_oracle; video_config.field_trials = &field_trials; video_config.red_payload_type = kRedPayloadType; video_config.ulpfec_payload_type = kUlpfecPayloadType; diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index e2ef16de51..99fb822cc5 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -240,6 +241,10 @@ const char* FrameTypeToString(VideoFrameType frame_type) { } #endif +bool IsNoopDelay(const PlayoutDelay& delay) { + return delay.min_ms == -1 && delay.max_ms == -1; +} + } // namespace RTPSenderVideo::RTPSenderVideo(Clock* clock, @@ -256,7 +261,6 @@ RTPSenderVideo::RTPSenderVideo(Clock* clock, config.clock = clock; config.rtp_sender = rtp_sender; config.flexfec_sender = flexfec_sender; - config.playout_delay_oracle = playout_delay_oracle; config.frame_encryptor = frame_encryptor; config.require_frame_encryption = require_frame_encryption; config.need_rtp_packet_infos = need_rtp_packet_infos; @@ -274,7 +278,8 @@ RTPSenderVideo::RTPSenderVideo(const Config& config) : (kRetransmitBaseLayer | kConditionallyRetransmitHigherLayers)), last_rotation_(kVideoRotation_0), transmit_color_space_next_frame_(false), - playout_delay_oracle_(config.playout_delay_oracle), + current_playout_delay_{-1, -1}, + playout_delay_pending_(false), rtp_sequence_number_map_(config.need_rtp_packet_infos ? std::make_unique( kRtpSequenceNumberMapMaxEntries) @@ -296,9 +301,7 @@ RTPSenderVideo::RTPSenderVideo(const Config& config) config.field_trials ->Lookup(kExcludeTransportSequenceNumberFromFecFieldTrial) .find("Enabled") == 0), - absolute_capture_time_sender_(config.clock) { - RTC_DCHECK(playout_delay_oracle_); -} + absolute_capture_time_sender_(config.clock) {} RTPSenderVideo::~RTPSenderVideo() {} @@ -519,8 +522,16 @@ bool RTPSenderVideo::SendVideo( video_header.codec == kVideoCodecH264 && video_header.frame_marking.temporal_id != kNoTemporalIdx; + MaybeUpdateCurrentPlayoutDelay(video_header); + if (video_header.frame_type == VideoFrameType::kVideoFrameKey && + !IsNoopDelay(current_playout_delay_)) { + // Force playout delay on key-frames, if set. + playout_delay_pending_ = true; + } const absl::optional playout_delay = - playout_delay_oracle_->PlayoutDelayToSend(video_header.playout_delay); + playout_delay_pending_ + ? absl::optional(current_playout_delay_) + : absl::nullopt; // According to // http://www.etsi.org/deliver/etsi_ts/126100_126199/126114/12.07.00_60/ @@ -649,6 +660,15 @@ bool RTPSenderVideo::SendVideo( MinimizeDescriptor(&video_header); } + if (video_header.frame_type == VideoFrameType::kVideoFrameKey || + (IsBaseLayer(video_header) && + !(video_header.generic.has_value() ? video_header.generic->discardable + : false))) { + // This frame has guaranteed delivery, no need to populate playout + // delay extensions until it changes again. + playout_delay_pending_ = false; + } + // TODO(benwright@webrtc.org) - Allocate enough to always encrypt inline. rtc::Buffer encrypted_video_payload; if (frame_encryptor_ != nullptr) { @@ -743,10 +763,6 @@ bool RTPSenderVideo::SendVideo( first_sequence_number = packet->SequenceNumber(); } - if (i == 0) { - playout_delay_oracle_->OnSentPacket(packet->SequenceNumber(), - playout_delay); - } // No FEC protection for upper temporal layers, if used. bool protect_packet = temporal_id == 0 || temporal_id == kNoTemporalIdx; @@ -940,4 +956,52 @@ bool RTPSenderVideo::UpdateConditionalRetransmit( return false; } +void RTPSenderVideo::MaybeUpdateCurrentPlayoutDelay( + const RTPVideoHeader& header) { + if (IsNoopDelay(header.playout_delay)) { + return; + } + + PlayoutDelay requested_delay = header.playout_delay; + + 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; + } + 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; + } + + if (!playout_delay_pending_) { + current_playout_delay_ = requested_delay; + playout_delay_pending_ = true; + return; + } + + if ((requested_delay.min_ms == -1 || + requested_delay.min_ms == current_playout_delay_.min_ms) && + (requested_delay.max_ms == -1 || + requested_delay.max_ms == current_playout_delay_.max_ms)) { + // No change, ignore. + return; + } + + if (requested_delay.min_ms == -1) { + RTC_DCHECK_GE(requested_delay.max_ms, 0); + requested_delay.min_ms = + std::min(current_playout_delay_.min_ms, requested_delay.max_ms); + } + if (requested_delay.max_ms == -1) { + requested_delay.max_ms = + std::max(current_playout_delay_.max_ms, requested_delay.min_ms); + } + + current_playout_delay_ = requested_delay; + playout_delay_pending_ = true; +} + } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_sender_video.h b/modules/rtp_rtcp/source/rtp_sender_video.h index 053877ef28..5f01803055 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.h +++ b/modules/rtp_rtcp/source/rtp_sender_video.h @@ -70,6 +70,7 @@ class RTPSenderVideo { Clock* clock = nullptr; RTPSender* rtp_sender = nullptr; FlexfecSender* flexfec_sender = nullptr; + // TODO(sprang): Remove when downstream usage is gone. PlayoutDelayOracle* playout_delay_oracle = nullptr; FrameEncryptorInterface* frame_encryptor = nullptr; bool require_frame_encryption = false; @@ -181,6 +182,9 @@ class RTPSenderVideo { int64_t expected_retransmission_time_ms) RTC_EXCLUSIVE_LOCKS_REQUIRED(stats_crit_); + void MaybeUpdateCurrentPlayoutDelay(const RTPVideoHeader& header) + RTC_EXCLUSIVE_LOCKS_REQUIRED(send_checker_); + RTPSender* const rtp_sender_; Clock* const clock_; @@ -195,10 +199,11 @@ class RTPSenderVideo { std::unique_ptr video_structure_ RTC_GUARDED_BY(send_checker_); - // Tracks the current request for playout delay limits from application - // and decides whether the current RTP frame should include the playout - // delay extension on header. - PlayoutDelayOracle* const playout_delay_oracle_; + // Current target playout delay. + PlayoutDelay current_playout_delay_ RTC_GUARDED_BY(send_checker_); + // Flag indicating if we need to propagate |current_playout_delay_| in order + // to guarantee it gets delivered. + bool playout_delay_pending_; // Should never be held when calling out of this class. rtc::CriticalSection crit_; diff --git a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc index 867e05b60d..af235afe2a 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc @@ -54,6 +54,7 @@ enum : int { // The first valid value is 1. kVideoRotationExtensionId, kVideoTimingExtensionId, kAbsoluteCaptureTimeExtensionId, + kPlayoutDelayExtensionId }; constexpr int kPayload = 100; @@ -87,6 +88,8 @@ class LoopbackTransportTest : public webrtc::Transport { kFrameMarkingExtensionId); receivers_extensions_.Register( kAbsoluteCaptureTimeExtensionId); + receivers_extensions_.Register( + kPlayoutDelayExtensionId); } bool SendRtp(const uint8_t* data, @@ -121,7 +124,6 @@ class TestRtpSenderVideo : public RTPSenderVideo { config.clock = clock; config.rtp_sender = rtp_sender; config.flexfec_sender = flexfec_sender; - config.playout_delay_oracle = &playout_delay_oracle_; config.field_trials = &field_trials; return config; }()) {} @@ -134,7 +136,6 @@ class TestRtpSenderVideo : public RTPSenderVideo { retransmission_settings, expected_retransmission_time_ms); } - PlayoutDelayOracle playout_delay_oracle_; }; class FieldTrials : public WebRtcKeyValueConfig { @@ -792,6 +793,63 @@ TEST_P(RtpSenderVideoTest, AbsoluteCaptureTime) { EXPECT_EQ(packets_with_abs_capture_time, 1); } +TEST_P(RtpSenderVideoTest, PopulatesPlayoutDelay) { + // Single packet frames. + constexpr size_t kPacketSize = 123; + uint8_t kFrame[kPacketSize]; + rtp_module_->RegisterRtpHeaderExtension(PlayoutDelayLimits::kUri, + kPlayoutDelayExtensionId); + const PlayoutDelay kExpectedDelay = {10, 20}; + + // Send initial key-frame without playout delay. + RTPVideoHeader hdr; + hdr.frame_type = VideoFrameType::kVideoFrameKey; + hdr.codec = VideoCodecType::kVideoCodecVP8; + auto& vp8_header = hdr.video_type_header.emplace(); + vp8_header.temporalIdx = 0; + + rtp_sender_video_.SendVideo(kPayload, kType, kTimestamp, 0, kFrame, nullptr, + hdr, kDefaultExpectedRetransmissionTimeMs); + EXPECT_FALSE( + transport_.last_sent_packet().HasExtension()); + + // Set playout delay on a discardable frame. + hdr.playout_delay = kExpectedDelay; + hdr.frame_type = VideoFrameType::kVideoFrameDelta; + vp8_header.temporalIdx = 1; + rtp_sender_video_.SendVideo(kPayload, kType, kTimestamp, 0, kFrame, nullptr, + hdr, kDefaultExpectedRetransmissionTimeMs); + PlayoutDelay received_delay = PlayoutDelay::Noop(); + ASSERT_TRUE(transport_.last_sent_packet().GetExtension( + &received_delay)); + EXPECT_EQ(received_delay, kExpectedDelay); + + // Set playout delay on a non-discardable frame, the extension should still + // be populated since dilvery wasn't guaranteed on the last one. + hdr.playout_delay = PlayoutDelay::Noop(); // Inidcates "no change". + vp8_header.temporalIdx = 0; + rtp_sender_video_.SendVideo(kPayload, kType, kTimestamp, 0, kFrame, nullptr, + hdr, kDefaultExpectedRetransmissionTimeMs); + ASSERT_TRUE(transport_.last_sent_packet().GetExtension( + &received_delay)); + EXPECT_EQ(received_delay, kExpectedDelay); + + // The next frame does not need the extensions since it's delivery has + // already been guaranteed. + rtp_sender_video_.SendVideo(kPayload, kType, kTimestamp, 0, kFrame, nullptr, + hdr, kDefaultExpectedRetransmissionTimeMs); + EXPECT_FALSE( + transport_.last_sent_packet().HasExtension()); + + // Insert key-frame, we need to refresh the state here. + hdr.frame_type = VideoFrameType::kVideoFrameKey; + rtp_sender_video_.SendVideo(kPayload, kType, kTimestamp, 0, kFrame, nullptr, + hdr, kDefaultExpectedRetransmissionTimeMs); + ASSERT_TRUE(transport_.last_sent_packet().GetExtension( + &received_delay)); + EXPECT_EQ(received_delay, kExpectedDelay); +} + INSTANTIATE_TEST_SUITE_P(WithAndWithoutOverhead, RtpSenderVideoTest, ::testing::Bool()); diff --git a/test/fuzzers/rtp_packet_fuzzer.cc b/test/fuzzers/rtp_packet_fuzzer.cc index 25fec2c094..774be0871e 100644 --- a/test/fuzzers/rtp_packet_fuzzer.cc +++ b/test/fuzzers/rtp_packet_fuzzer.cc @@ -99,10 +99,11 @@ void FuzzOneInput(const uint8_t* data, size_t size) { &feedback_request); break; } - case kRtpExtensionPlayoutDelay: - PlayoutDelay playout; + case kRtpExtensionPlayoutDelay: { + PlayoutDelay playout = PlayoutDelay::Noop(); packet.GetExtension(&playout); break; + } case kRtpExtensionVideoContentType: VideoContentType content_type; packet.GetExtension(&content_type);