Reland "Remove PlayoutDelayOracle and make RtpSenderVideo guarantee delivery"
This is a reland of 4f68f5398d7fa3d47c449e99893c9bea07bf7ca2
Original change's description:
> 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}
TBR=stefan@webrtc.org
Bug: webrtc:11340
Change-Id: I2fdd0004121b13b96497b21e052359e31d0c477a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/168305
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30479}
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 25a9d29..b2bda62 100644
--- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h
+++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h
@@ -402,19 +402,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 69a09d8..c3ae539 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);
@@ -1150,12 +1148,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};
@@ -1194,12 +1190,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);
@@ -1241,13 +1235,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);
@@ -1327,13 +1319,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);
@@ -1601,13 +1591,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.
@@ -1677,12 +1665,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;
@@ -1735,12 +1721,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};
@@ -1792,12 +1776,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 e2ef16d..99fb822 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() {}
@@ -519,8 +522,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/
@@ -649,6 +660,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) {
@@ -743,10 +763,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;
@@ -940,4 +956,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..5f01803 100644
--- a/modules/rtp_rtcp/source/rtp_sender_video.h
+++ b/modules/rtp_rtcp/source/rtp_sender_video.h
@@ -70,6 +70,7 @@
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 @@
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 @@
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);