From eabaf8d7fe5fbc99cbc9451a3bd43b1d8f64ec29 Mon Sep 17 00:00:00 2001 From: Evan Shrubsole Date: Mon, 23 May 2022 17:45:58 +0200 Subject: [PATCH] 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 Commit-Queue: Evan Shrubsole Cr-Commit-Position: refs/heads/main@{#36977} --- modules/video_coding/timing.cc | 19 ++++++++++++++++++- modules/video_coding/timing.h | 1 + video/video_receive_stream2.cc | 24 +++++++++++++++++++++--- 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/modules/video_coding/timing.cc b/modules/video_coding/timing.cc index af421c7c79..b7103c4c15 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 @@ void VCMTiming::set_render_delay(TimeDelta render_delay) { 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_min_playout_delay(TimeDelta min_playout_delay) { 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 8c09451485..69efc7aba6 100644 --- a/modules/video_coding/timing.h +++ b/modules/video_coding/timing.h @@ -47,6 +47,7 @@ class VCMTiming { 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 40abc134bc..4610bb5831 100644 --- a/video/video_receive_stream2.cc +++ b/video/video_receive_stream2.cc @@ -181,6 +181,10 @@ bool IsKeyFrameAndUnspecifiedResolution(const EncodedFrame& frame) { // timestamps wraparound to affect FrameBuffer. constexpr TimeDelta kInactiveStreamThreshold = TimeDelta::Minutes(10); +std::string OptionalDelayToLogString(const absl::optional opt) { + return opt.has_value() ? ToLogString(*opt) : ""; +} + } // namespace TimeDelta DetermineMaxWaitForFrame( @@ -977,12 +981,26 @@ bool VideoReceiveStream2::IsReceivingKeyFrame(Timestamp now) const { void VideoReceiveStream2::UpdatePlayoutDelays() const { // Running on worker_sequence_checker_. + const std::initializer_list> 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 minimum_delay = - std::max({frame_minimum_playout_delay_, base_minimum_playout_delay_, - syncable_minimum_playout_delay_}); + absl::optional 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()) {