From 4f68f5398d7fa3d47c449e99893c9bea07bf7ca2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Thu, 6 Feb 2020 13:43:51 +0100 Subject: [PATCH] Remove PlayoutDelayOracle and make RtpSenderVideo guarantee delivery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PlayoutDelayOracle was responsible for making sure the PlayoutDelay header extension was successfully propagated to the receiving side. Once it was determined that the receiver had received a frame with the new delay tag, it's no longer necessary to propagate. The issue with this implementation is that it is based on max extended sequence number reported via RTCP, which makes it often slow to react, could theoretically fail to produce desired outcome (max received > X does not guarantee X was fully received and decoded), and added a lot of code complexity. The guarantee of delivery can in fact be accomplished more reliably and with less code by making sure to tag each frame until an undiscardable frame is sent. This allows containing the logic fully within RTPSenderVideo. Bug: webrtc:11340 Change-Id: I2d1d2b6b67f4f07b8b33336f8fcfcde724243eef Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/168221 Reviewed-by: Stefan Holmer Reviewed-by: Sebastian Jansson Commit-Queue: Erik Språng Cr-Commit-Position: refs/heads/master@{#30473} --- call/rtp_video_sender.cc | 17 +--- call/rtp_video_sender.h | 4 +- common_types.h | 8 ++ modules/rtp_rtcp/BUILD.gn | 2 - modules/rtp_rtcp/include/rtp_rtcp.h | 1 - modules/rtp_rtcp/include/rtp_rtcp_defines.h | 13 --- modules/rtp_rtcp/source/nack_rtx_unittest.cc | 3 - .../rtp_rtcp/source/playout_delay_oracle.cc | 90 ------------------- .../rtp_rtcp/source/playout_delay_oracle.h | 58 +----------- .../source/playout_delay_oracle_unittest.cc | 52 ----------- modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 5 +- modules/rtp_rtcp/source/rtp_rtcp_impl.h | 2 - .../rtp_rtcp/source/rtp_rtcp_impl_unittest.cc | 3 - .../rtp_rtcp/source/rtp_sender_unittest.cc | 18 ---- modules/rtp_rtcp/source/rtp_sender_video.cc | 84 ++++++++++++++--- modules/rtp_rtcp/source/rtp_sender_video.h | 13 +-- .../source/rtp_sender_video_unittest.cc | 62 ++++++++++++- test/fuzzers/rtp_packet_fuzzer.cc | 5 +- 18 files changed, 162 insertions(+), 278 deletions(-) delete mode 100644 modules/rtp_rtcp/source/playout_delay_oracle.cc delete mode 100644 modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc 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 8cd402e227..bdee7b45ed 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -392,19 +392,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 5ca4e70de8..458d3e7eb6 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); @@ -1153,12 +1151,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}; @@ -1197,12 +1193,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); @@ -1244,13 +1238,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); @@ -1330,13 +1322,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); @@ -1604,13 +1594,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. @@ -1680,12 +1668,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; @@ -1738,12 +1724,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}; @@ -1795,12 +1779,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 fc176c96cd..6c171c6d99 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() {} @@ -521,8 +524,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/ @@ -651,6 +662,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) { @@ -745,10 +765,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; @@ -942,4 +958,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..0f42d25a76 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.h +++ b/modules/rtp_rtcp/source/rtp_sender_video.h @@ -70,7 +70,6 @@ class RTPSenderVideo { Clock* clock = nullptr; RTPSender* rtp_sender = nullptr; FlexfecSender* flexfec_sender = nullptr; - PlayoutDelayOracle* playout_delay_oracle = nullptr; FrameEncryptorInterface* frame_encryptor = nullptr; bool require_frame_encryption = false; bool need_rtp_packet_infos = false; @@ -181,6 +180,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 +197,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);