Remove PlayoutDelayOracle and make RtpSenderVideo guarantee delivery
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 <stefan@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30473}
diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc
index 413171f..3ae0794 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_internal_rtp_video_sender {
-RtpStreamSender::RtpStreamSender(
- std::unique_ptr<PlayoutDelayOracle> playout_delay_oracle,
- std::unique_ptr<RtpRtcp> rtp_rtcp,
- std::unique_ptr<RTPSenderVideo> 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<RtpRtcp> rtp_rtcp,
+ std::unique_ptr<RTPSenderVideo> sender_video)
+ : rtp_rtcp(std::move(rtp_rtcp)), sender_video(std::move(sender_video)) {}
RtpStreamSender::~RtpStreamSender() = default;
@@ -177,9 +172,7 @@
configuration.local_media_ssrc) !=
flexfec_protected_ssrcs.end();
configuration.flexfec_sender = enable_flexfec ? flexfec_sender : nullptr;
- auto playout_delay_oracle = std::make_unique<PlayoutDelayOracle>();
- 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 @@
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 @@
video_config.ulpfec_payload_type = rtp_config.ulpfec.ulpfec_payload_type;
}
auto sender_video = std::make_unique<RTPSenderVideo>(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 eb7e431..620c975 100644
--- a/call/rtp_video_sender.h
+++ b/call/rtp_video_sender.h
@@ -50,8 +50,7 @@
// RTP state for a single simulcast stream. Internal to the implementation of
// RtpVideoSender.
struct RtpStreamSender {
- RtpStreamSender(std::unique_ptr<PlayoutDelayOracle> playout_delay_oracle,
- std::unique_ptr<RtpRtcp> rtp_rtcp,
+ RtpStreamSender(std::unique_ptr<RtpRtcp> rtp_rtcp,
std::unique_ptr<RTPSenderVideo> sender_video);
~RtpStreamSender();
@@ -59,7 +58,6 @@
RtpStreamSender& operator=(RtpStreamSender&&) = default;
// Note: Needs pointer stability.
- std::unique_ptr<PlayoutDelayOracle> playout_delay_oracle;
std::unique_ptr<RtpRtcp> rtp_rtcp;
std::unique_ptr<RTPSenderVideo> sender_video;
};
diff --git a/common_types.h b/common_types.h
index aadda4f..dedcbd5 100644
--- a/common_types.h
+++ b/common_types.h
@@ -89,8 +89,16 @@
// 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 099c066..b8dd23e 100644
--- a/modules/rtp_rtcp/BUILD.gn
+++ b/modules/rtp_rtcp/BUILD.gn
@@ -156,7 +156,6 @@
"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 @@
"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 b3cd8f6..fbb3bb3 100644
--- a/modules/rtp_rtcp/include/rtp_rtcp.h
+++ b/modules/rtp_rtcp/include/rtp_rtcp.h
@@ -101,7 +101,6 @@
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 8cd402e..bdee7b4 100644
--- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h
+++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h
@@ -392,19 +392,6 @@
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 17601dd..55e1e44 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 @@
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<RTPSenderVideo>(video_config);
rtp_rtcp_module_->SetRTCPStatus(RtcpMode::kCompound);
@@ -227,7 +225,6 @@
std::unique_ptr<ReceiveStatistics> receive_statistics_;
std::unique_ptr<RtpRtcp> rtp_rtcp_module_;
- PlayoutDelayOracle playout_delay_oracle_;
std::unique_ptr<RTPSenderVideo> rtp_sender_video_;
RtxLoopBackTransport transport_;
const std::map<int, int> 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 f234759..0000000
--- 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 <algorithm>
-
-#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<PlayoutDelay> 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<PlayoutDelay> 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 6451be4..04465e3 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 <stdint.h>
-
-#include "absl/types/optional.h"
-#include "common_types.h" // NOLINT(build/include)
-#include "modules/include/module_common_types_public.h"
-#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
-#include "rtc_base/constructor_magic.h"
-#include "rtc_base/critical_section.h"
-#include "rtc_base/thread_annotations.h"
-
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<PlayoutDelay> PlayoutDelayToSend(
- PlayoutDelay requested_delay) const;
-
- void OnSentPacket(uint16_t sequence_number,
- absl::optional<PlayoutDelay> 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<int64_t> unacked_sequence_number_ RTC_GUARDED_BY(crit_sect_);
- // Sequence number unwrapper for sent packets.
-
- // TODO(nisse): Could potentially get out of sync with the unwrapper used by
- // the caller of OnReceivedAck.
- SequenceNumberUnwrapper unwrapper_ RTC_GUARDED_BY(crit_sect_);
- // Playout delay values on the next frame if |send_playout_delay_| is set.
- PlayoutDelay 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 3857e9b..0000000
--- 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<PlayoutDelay> delay_to_send =
- playout_delay_oracle.PlayoutDelayToSend({-1, -1});
- ASSERT_TRUE(delay_to_send.has_value());
- EXPECT_EQ(kMinPlayoutDelay, delay_to_send->min_ms);
- EXPECT_EQ(kMaxPlayoutDelay, delay_to_send->max_ms);
-
- // Oracle indicates playout delay should be sent if highest sequence number
- // acked is lower than the sequence number of the first packet containing
- // playout delay.
- 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 987ae0e..dfbac29 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
@@ -68,7 +68,6 @@
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::OnReceivedRtcpReportBlocks(
const ReportBlockList& report_blocks) {
- if (ack_observer_) {
+ if (rtp_sender_) {
uint32_t ssrc = SSRC();
absl::optional<uint32_t> rtx_ssrc;
if (rtp_sender_->packet_generator.RtxStatus() != kRtxOff) {
@@ -747,8 +746,6 @@
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 976653a..c03683f 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h
@@ -340,8 +340,6 @@
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 0b681cf..5e4cce9 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 @@
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<RTPSenderVideo>(video_config);
@@ -201,7 +199,6 @@
SimulatedClock clock_;
RtpRtcpModule sender_;
- PlayoutDelayOracle playout_delay_oracle_;
std::unique_ptr<RTPSenderVideo> 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 5ca4e70..458d3e7 100644
--- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
@@ -649,12 +649,10 @@
config.event_log = &mock_rtc_event_log_;
rtp_sender_context_ = std::make_unique<RtpSenderContext>(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(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 @@
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 @@
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 @@
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 @@
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 @@
config.retransmission_rate_limiter = &retransmission_rate_limiter_;
rtp_sender_context_ = std::make_unique<RtpSenderContext>(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(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 @@
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 fc176c9..6c171c6 100644
--- a/modules/rtp_rtcp/source/rtp_sender_video.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -13,6 +13,7 @@
#include <stdlib.h>
#include <string.h>
+#include <algorithm>
#include <limits>
#include <memory>
#include <string>
@@ -240,6 +241,10 @@
}
#endif
+bool IsNoopDelay(const PlayoutDelay& delay) {
+ return delay.min_ms == -1 && delay.max_ms == -1;
+}
+
} // namespace
RTPSenderVideo::RTPSenderVideo(Clock* clock,
@@ -256,7 +261,6 @@
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 @@
: (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<RtpSequenceNumberMap>(
kRtpSequenceNumberMapMaxEntries)
@@ -296,9 +301,7 @@
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 @@
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<PlayoutDelay> playout_delay =
- playout_delay_oracle_->PlayoutDelayToSend(video_header.playout_delay);
+ playout_delay_pending_
+ ? absl::optional<PlayoutDelay>(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 @@
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 @@
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 @@
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 053877e..0f42d25 100644
--- a/modules/rtp_rtcp/source/rtp_sender_video.h
+++ b/modules/rtp_rtcp/source/rtp_sender_video.h
@@ -70,7 +70,6 @@
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 @@
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 @@
std::unique_ptr<FrameDependencyStructure> 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 867e05b..af235af 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 @@
kVideoRotationExtensionId,
kVideoTimingExtensionId,
kAbsoluteCaptureTimeExtensionId,
+ kPlayoutDelayExtensionId
};
constexpr int kPayload = 100;
@@ -87,6 +88,8 @@
kFrameMarkingExtensionId);
receivers_extensions_.Register<AbsoluteCaptureTimeExtension>(
kAbsoluteCaptureTimeExtensionId);
+ receivers_extensions_.Register<PlayoutDelayLimits>(
+ kPlayoutDelayExtensionId);
}
bool SendRtp(const uint8_t* data,
@@ -121,7 +124,6 @@
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 @@
retransmission_settings,
expected_retransmission_time_ms);
}
- PlayoutDelayOracle playout_delay_oracle_;
};
class FieldTrials : public WebRtcKeyValueConfig {
@@ -792,6 +793,63 @@
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<RTPVideoHeaderVP8>();
+ vp8_header.temporalIdx = 0;
+
+ rtp_sender_video_.SendVideo(kPayload, kType, kTimestamp, 0, kFrame, nullptr,
+ hdr, kDefaultExpectedRetransmissionTimeMs);
+ EXPECT_FALSE(
+ transport_.last_sent_packet().HasExtension<PlayoutDelayLimits>());
+
+ // 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<PlayoutDelayLimits>(
+ &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<PlayoutDelayLimits>(
+ &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<PlayoutDelayLimits>());
+
+ // 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<PlayoutDelayLimits>(
+ &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 25fec2c..774be08 100644
--- a/test/fuzzers/rtp_packet_fuzzer.cc
+++ b/test/fuzzers/rtp_packet_fuzzer.cc
@@ -99,10 +99,11 @@
&feedback_request);
break;
}
- case kRtpExtensionPlayoutDelay:
- PlayoutDelay playout;
+ case kRtpExtensionPlayoutDelay: {
+ PlayoutDelay playout = PlayoutDelay::Noop();
packet.GetExtension<PlayoutDelayLimits>(&playout);
break;
+ }
case kRtpExtensionVideoContentType:
VideoContentType content_type;
packet.GetExtension<VideoContentTypeExtension>(&content_type);