Log when conflicting recv video playout delays are set
There are two cases that can be confusing for applications developers
which may result in the playout delay not being set as intended.
First, it is not well defined which min playout delay should be used
when multiple are set. This changes adds a warning to alert application
developers that they are setting multiple playout delays.
Second, if the playout delay header extension is used, developers must
be careful that the max playout delay is always larger than the min
playout delay, otherwise the behaviour is undefined. This change logs an
error when this case is detected.
Bug: None
Change-Id: I8477d48ef64636da080792362fa898e42f038bef
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/263202
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Commit-Queue: Evan Shrubsole <eshr@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36977}
diff --git a/modules/video_coding/timing.cc b/modules/video_coding/timing.cc
index af421c7..b7103c4 100644
--- a/modules/video_coding/timing.cc
+++ b/modules/video_coding/timing.cc
@@ -14,6 +14,7 @@
#include "api/units/time_delta.h"
#include "rtc_base/experiments/field_trial_parser.h"
+#include "rtc_base/logging.h"
#include "rtc_base/time/timestamp_extrapolator.h"
#include "system_wrappers/include/clock.h"
@@ -61,6 +62,11 @@
render_delay_ = render_delay;
}
+TimeDelta VCMTiming::min_playout_delay() const {
+ MutexLock lock(&mutex_);
+ return min_playout_delay_;
+}
+
void VCMTiming::set_min_playout_delay(TimeDelta min_playout_delay) {
MutexLock lock(&mutex_);
min_playout_delay_ = min_playout_delay;
@@ -68,7 +74,18 @@
void VCMTiming::set_max_playout_delay(TimeDelta max_playout_delay) {
MutexLock lock(&mutex_);
- max_playout_delay_ = max_playout_delay;
+ if (max_playout_delay_ != max_playout_delay) {
+ if (min_playout_delay_ > max_playout_delay) {
+ RTC_LOG(LS_ERROR)
+ << "Playout delays set incorrectly: min playout delay ("
+ << min_playout_delay_ << ") > max playout delay ("
+ << max_playout_delay
+ << "). This is undefined behaviour. Application writers should "
+ "ensure that the min delay is always less than or equals max "
+ "delay.";
+ }
+ max_playout_delay_ = max_playout_delay;
+ }
}
void VCMTiming::SetJitterDelay(TimeDelta jitter_delay) {
diff --git a/modules/video_coding/timing.h b/modules/video_coding/timing.h
index 8c09451..69efc7a 100644
--- a/modules/video_coding/timing.h
+++ b/modules/video_coding/timing.h
@@ -47,6 +47,7 @@
void SetJitterDelay(TimeDelta required_delay);
// Set/get the minimum playout delay from capture to render.
+ TimeDelta min_playout_delay() const;
void set_min_playout_delay(TimeDelta min_playout_delay);
// Set/get the maximum playout delay from capture to render in ms.
diff --git a/video/video_receive_stream2.cc b/video/video_receive_stream2.cc
index 40abc13..4610bb5 100644
--- a/video/video_receive_stream2.cc
+++ b/video/video_receive_stream2.cc
@@ -181,6 +181,10 @@
// timestamps wraparound to affect FrameBuffer.
constexpr TimeDelta kInactiveStreamThreshold = TimeDelta::Minutes(10);
+std::string OptionalDelayToLogString(const absl::optional<TimeDelta> opt) {
+ return opt.has_value() ? ToLogString(*opt) : "<unset>";
+}
+
} // namespace
TimeDelta DetermineMaxWaitForFrame(
@@ -977,12 +981,26 @@
void VideoReceiveStream2::UpdatePlayoutDelays() const {
// Running on worker_sequence_checker_.
+ const std::initializer_list<absl::optional<TimeDelta>> min_delays = {
+ frame_minimum_playout_delay_, base_minimum_playout_delay_,
+ syncable_minimum_playout_delay_};
// Since nullopt < anything, this will return the largest of the minumum
// delays, or nullopt if all are nullopt.
- absl::optional<TimeDelta> minimum_delay =
- std::max({frame_minimum_playout_delay_, base_minimum_playout_delay_,
- syncable_minimum_playout_delay_});
+ absl::optional<TimeDelta> minimum_delay = std::max(min_delays);
if (minimum_delay) {
+ auto num_playout_delays_set =
+ absl::c_count_if(min_delays, [](auto opt) { return opt.has_value(); });
+ if (num_playout_delays_set > 1 &&
+ timing_->min_playout_delay() != minimum_delay) {
+ RTC_LOG(LS_WARNING)
+ << "Multiple playout delays set. Actual delay value set to "
+ << *minimum_delay << " frame min delay="
+ << OptionalDelayToLogString(frame_maximum_playout_delay_)
+ << " base min delay="
+ << OptionalDelayToLogString(base_minimum_playout_delay_)
+ << " sync min delay="
+ << OptionalDelayToLogString(syncable_minimum_playout_delay_);
+ }
timing_->set_min_playout_delay(*minimum_delay);
if (frame_minimum_playout_delay_ == TimeDelta::Zero() &&
frame_maximum_playout_delay_ > TimeDelta::Zero()) {
