diff --git a/video/overuse_frame_detector_resource_adaptation_module.cc b/video/overuse_frame_detector_resource_adaptation_module.cc index 673eebd09e..1e449b99be 100644 --- a/video/overuse_frame_detector_resource_adaptation_module.cc +++ b/video/overuse_frame_detector_resource_adaptation_module.cc @@ -41,6 +41,29 @@ bool IsFramerateScalingEnabled(DegradationPreference degradation_preference) { degradation_preference == DegradationPreference::BALANCED; } +// Returns modified restrictions where any constraints that don't apply to the +// degradation preference are cleared. +VideoSourceRestrictions ApplyDegradationPreference( + VideoSourceRestrictions source_restrictions, + DegradationPreference degradation_preference) { + switch (degradation_preference) { + case DegradationPreference::BALANCED: + break; + case DegradationPreference::MAINTAIN_FRAMERATE: + source_restrictions.set_max_frame_rate(absl::nullopt); + break; + case DegradationPreference::MAINTAIN_RESOLUTION: + source_restrictions.set_max_pixels_per_frame(absl::nullopt); + source_restrictions.set_target_pixels_per_frame(absl::nullopt); + break; + case DegradationPreference::DISABLED: + source_restrictions.set_max_pixels_per_frame(absl::nullopt); + source_restrictions.set_target_pixels_per_frame(absl::nullopt); + source_restrictions.set_max_frame_rate(absl::nullopt); + } + return source_restrictions; +} + } // namespace // VideoSourceRestrictor is responsible for keeping track of current @@ -56,10 +79,8 @@ bool IsFramerateScalingEnabled(DegradationPreference degradation_preference) { // using a lock. class OveruseFrameDetectorResourceAdaptationModule::VideoSourceRestrictor { public: - explicit VideoSourceRestrictor( - VideoSourceSinkController* video_source_sink_controller) - : video_source_sink_controller_(video_source_sink_controller), - has_input_video_(false), + VideoSourceRestrictor() + : has_input_video_(false), degradation_preference_(DegradationPreference::DISABLED) {} VideoSourceRestrictions source_restrictions() { @@ -80,21 +101,9 @@ class OveruseFrameDetectorResourceAdaptationModule::VideoSourceRestrictor { degradation_preference_ = degradation_preference; } - // Informs the sink of the new source settings. - // TODO(https://crbug.com/webrtc/11222): Handle all sink updates in - // video_stream_encoder.cc. This method is only used when setting the - // degradation preference such that it moves in or out of the "balanced" - // state, or when clearing all counters. When moving the remaining degradation - // preference logic inside the VideoSourceSinkController to here, stop - // explicitly setting the controller's restrictions and instead inform the - // VideoStreamEncoder of updated restrictions using - // OnVideoSourceRestrictionsUpdated(). - void ResetPixelFpsCount() { + void ClearRestrictions() { rtc::CritScope lock(&crit_); - // Clear all restrictions. source_restrictions_ = VideoSourceRestrictions(); - video_source_sink_controller_->SetRestrictions(source_restrictions_); - video_source_sink_controller_->PushSourceSinkSettings(); } // Updates the source_restrictions(). The source/sink has to be informed of @@ -252,7 +261,6 @@ class OveruseFrameDetectorResourceAdaptationModule::VideoSourceRestrictor { private: rtc::CriticalSection crit_; SequenceChecker main_checker_; - VideoSourceSinkController* const video_source_sink_controller_; VideoSourceRestrictions source_restrictions_ RTC_GUARDED_BY(&crit_); bool has_input_video_ RTC_GUARDED_BY(&crit_); DegradationPreference degradation_preference_ RTC_GUARDED_BY(&crit_); @@ -386,21 +394,18 @@ OveruseFrameDetectorResourceAdaptationModule::AdaptCounter::ToString( OveruseFrameDetectorResourceAdaptationModule:: OveruseFrameDetectorResourceAdaptationModule( VideoStreamEncoder* video_stream_encoder, - VideoSourceSinkController* video_source_sink_controller, std::unique_ptr overuse_detector, VideoStreamEncoderObserver* encoder_stats_observer, ResourceAdaptationModuleListener* adaptation_listener) : encoder_queue_(nullptr), adaptation_listener_(adaptation_listener), video_stream_encoder_(video_stream_encoder), - video_source_sink_controller_(video_source_sink_controller), degradation_preference_(DegradationPreference::DISABLED), adapt_counters_(), balanced_settings_(), last_adaptation_request_(absl::nullopt), last_frame_pixel_count_(absl::nullopt), - source_restrictor_(std::make_unique( - video_source_sink_controller)), + source_restrictor_(std::make_unique()), overuse_detector_(std::move(overuse_detector)), codec_max_framerate_(-1), encoder_start_bitrate_bps_(0), @@ -511,8 +516,12 @@ void OveruseFrameDetectorResourceAdaptationModule:: SetHasInputVideoAndDegradationPreference( bool has_input_video, DegradationPreference degradation_preference) { + // TODO(https://crbug.com/webrtc/11222): Move this call to the encoder queue, + // making VideoSourceRestrictor single-threaded and removing the only call to + // MaybeUpdateVideoSourceRestrictions() that isn't on the |encoder_queue_|. source_restrictor_->SetHasInputVideoAndDegradationPreference( has_input_video, degradation_preference); + MaybeUpdateVideoSourceRestrictions(degradation_preference); encoder_queue_->PostTask([this, degradation_preference] { RTC_DCHECK_RUN_ON(encoder_queue_); if (degradation_preference_ != degradation_preference) { @@ -523,32 +532,41 @@ void OveruseFrameDetectorResourceAdaptationModule:: degradation_preference_ == DegradationPreference::BALANCED) { // TODO(asapersson): Consider removing |adapt_counters_| map and use one // AdaptCounter for all modes. - source_restrictor_->ResetPixelFpsCount(); + source_restrictor_->ClearRestrictions(); adapt_counters_.clear(); } } degradation_preference_ = degradation_preference; + // This is the second time we're invoking + // MaybeUpdateVideoSourceRestrictions() in this method. This is because + // current tests expect the changes to the source restrictions to be + // immediate (outside of the encoder queue) while it is possible that they + // change again after ClearRestrictions() on the encoder queue. + // TODO(https://crbug.com/webrtc/11222): Change the expectations to allow + // source restrictions only to change on the encoder queue. This unblocks + // making OveruseFrameDetectorResourceAdaptationModule and + // VideoSourceRestrictor single-threaded. + MaybeUpdateVideoSourceRestrictions(degradation_preference_); }); } void OveruseFrameDetectorResourceAdaptationModule::RefreshTargetFramerate() { RTC_DCHECK(encoder_queue_); RTC_DCHECK_RUN_ON(encoder_queue_); - // We need the "sink wants" from the |video_source_sink_controller_| because - // the controller filters its current settings as "sink wants" differently - // depending degradation preferences. - // TODO(https://crbug.com/webrtc/11222): When degradation preference-related - // changes to settings are handled by this class instead, we can remove the - // dependency on the controller; the VideoSourceRestrictions outputted by this - // module will then be the "final" settings, including the max frame rate. - auto sink_wants = video_source_sink_controller_->CurrentSettingsToSinkWants(); + absl::optional restricted_frame_rate = + ApplyDegradationPreference(source_restrictor_->source_restrictions(), + degradation_preference_) + .max_frame_rate(); // Get the current target framerate, ie the maximum framerate as specified by // the current codec configuration, or any limit imposed by cpu adaption in // maintain-resolution or balanced mode. This is used to make sure overuse // detection doesn't needlessly trigger in low and/or variable framerate // scenarios. int target_framerate = - std::min(codec_max_framerate_, sink_wants.max_framerate_fps); + std::min(codec_max_framerate_, + restricted_frame_rate.has_value() + ? static_cast(restricted_frame_rate.value()) + : std::numeric_limits::max()); overuse_detector_->OnTargetFramerateUpdated(target_framerate); } @@ -556,8 +574,9 @@ void OveruseFrameDetectorResourceAdaptationModule::ResetAdaptationCounters() { RTC_DCHECK(encoder_queue_); RTC_DCHECK_RUN_ON(encoder_queue_); last_adaptation_request_.reset(); - source_restrictor_->ResetPixelFpsCount(); + source_restrictor_->ClearRestrictions(); adapt_counters_.clear(); + MaybeUpdateVideoSourceRestrictions(degradation_preference_); } void OveruseFrameDetectorResourceAdaptationModule::AdaptUp(AdaptReason reason) { @@ -666,8 +685,7 @@ void OveruseFrameDetectorResourceAdaptationModule::AdaptUp(AdaptReason reason) { // Tell the adaptation listener to reconfigure the source for us according to // the latest adaptation. - adaptation_listener_->OnVideoSourceRestrictionsUpdated( - source_restrictor_->source_restrictions()); + MaybeUpdateVideoSourceRestrictions(degradation_preference_); last_adaptation_request_.emplace(adaptation_request); @@ -773,8 +791,7 @@ bool OveruseFrameDetectorResourceAdaptationModule::AdaptDown( // Tell the adaptation listener to reconfigure the source for us according to // the latest adaptation. - adaptation_listener_->OnVideoSourceRestrictionsUpdated( - source_restrictor_->source_restrictions()); + MaybeUpdateVideoSourceRestrictions(degradation_preference_); last_adaptation_request_.emplace(adaptation_request); @@ -784,6 +801,25 @@ bool OveruseFrameDetectorResourceAdaptationModule::AdaptDown( return did_adapt; } +void OveruseFrameDetectorResourceAdaptationModule:: + MaybeUpdateVideoSourceRestrictions( + DegradationPreference degradation_preference) { + absl::optional updated_restrictions; + { + rtc::CritScope lock(&video_source_restrictions_crit_); + VideoSourceRestrictions new_restrictions = ApplyDegradationPreference( + source_restrictor_->source_restrictions(), degradation_preference); + if (video_source_restrictions_ != new_restrictions) { + video_source_restrictions_ = std::move(new_restrictions); + updated_restrictions = video_source_restrictions_; + } + } + if (updated_restrictions.has_value()) { + adaptation_listener_->OnVideoSourceRestrictionsUpdated( + updated_restrictions.value()); + } +} + // TODO(nisse): Delete, once AdaptReason and AdaptationReason are merged. void OveruseFrameDetectorResourceAdaptationModule::UpdateAdaptationStats( AdaptReason reason) { diff --git a/video/overuse_frame_detector_resource_adaptation_module.h b/video/overuse_frame_detector_resource_adaptation_module.h index c5485c19cd..dcd78e83d9 100644 --- a/video/overuse_frame_detector_resource_adaptation_module.h +++ b/video/overuse_frame_detector_resource_adaptation_module.h @@ -27,7 +27,6 @@ #include "call/adaptation/resource_adaptation_module_interface.h" #include "rtc_base/experiments/balanced_degradation_settings.h" #include "video/overuse_frame_detector.h" -#include "video/video_source_sink_controller.h" namespace webrtc { @@ -51,7 +50,6 @@ class OveruseFrameDetectorResourceAdaptationModule public: OveruseFrameDetectorResourceAdaptationModule( VideoStreamEncoder* video_stream_encoder, - VideoSourceSinkController* video_source_controller, std::unique_ptr overuse_detector, VideoStreamEncoderObserver* encoder_stats_observer, ResourceAdaptationModuleListener* adaptation_listener); @@ -186,6 +184,16 @@ class OveruseFrameDetectorResourceAdaptationModule enum class Mode { kAdaptUp, kAdaptDown } mode_; }; + // Makes |video_source_restrictions_| up-to-date and informs the + // |adaptation_listener_| if restrictions are changed, allowing the listener + // to reconfigure the source accordingly. + // TODO(https://crbug.com/webrtc/11222): When + // SetHasInputVideoAndDegradationPreference() stops calling this method prior + // to updating |degradation_preference_| on the encoder queue, remove its + // argument in favor of using |degradation_preference_| directly. + void MaybeUpdateVideoSourceRestrictions( + DegradationPreference degradation_preference); + void UpdateAdaptationStats(AdaptReason reason) RTC_RUN_ON(encoder_queue_); DegradationPreference EffectiveDegradataionPreference() RTC_RUN_ON(encoder_queue_); @@ -193,18 +201,20 @@ class OveruseFrameDetectorResourceAdaptationModule bool CanAdaptUpResolution(int pixels, uint32_t bitrate_bps) const RTC_RUN_ON(encoder_queue_); - // TODO(hbos): Can we move the |source_restrictor_| to the |encoder_queue_| - // and replace |encoder_queue_| with a sequence checker instead? rtc::TaskQueue* encoder_queue_; - ResourceAdaptationModuleListener* const adaptation_listener_ - RTC_GUARDED_BY(encoder_queue_); + // TODO(https://crbug.com/webrtc/11222): Update + // SetHasInputVideoAndDegradationPreference() to do all work on the encoder + // queue (including |source_restrictor_| and |adaptation_listener_| usage). + // When this is the case, remove |VideoSourceRestrictor::crit_| and + // |video_source_restrictions_crit_| and replace |encoder_queue_| with a + // sequence checker. + rtc::CriticalSection video_source_restrictions_crit_; + ResourceAdaptationModuleListener* const adaptation_listener_; + // The restrictions that |adaptation_listener_| is informed of. + VideoSourceRestrictions video_source_restrictions_ + RTC_GUARDED_BY(&video_source_restrictions_crit_); // Used to query CpuOveruseOptions at StartCheckForOveruse(). VideoStreamEncoder* video_stream_encoder_ RTC_GUARDED_BY(encoder_queue_); - // TODO(https://crbug.com/webrtc/11222): When the VideoSourceSinkController is - // no longer aware of DegradationPreference, and the degradation - // preference-related logic resides within this class, we can remove this - // dependency on the VideoSourceSinkController. - VideoSourceSinkController* const video_source_sink_controller_; DegradationPreference degradation_preference_ RTC_GUARDED_BY(encoder_queue_); // Counters used for deciding if the video resolution or framerate is // currently restricted, and if so, why, on a per degradation preference diff --git a/video/video_source_sink_controller.cc b/video/video_source_sink_controller.cc index f3585766a0..a649adc68c 100644 --- a/video/video_source_sink_controller.cc +++ b/video/video_source_sink_controller.cc @@ -21,23 +21,19 @@ namespace webrtc { VideoSourceSinkController::VideoSourceSinkController( rtc::VideoSinkInterface* sink, rtc::VideoSourceInterface* source) - : sink_(sink), - source_(source), - degradation_preference_(DegradationPreference::DISABLED) { + : sink_(sink), source_(source) { RTC_DCHECK(sink_); } void VideoSourceSinkController::SetSource( - rtc::VideoSourceInterface* source, - DegradationPreference degradation_preference) { + rtc::VideoSourceInterface* source) { rtc::VideoSourceInterface* old_source; rtc::VideoSinkWants wants; { rtc::CritScope lock(&crit_); old_source = source_; source_ = source; - degradation_preference_ = degradation_preference; - wants = CurrentSettingsToSinkWantsInternal(); + wants = CurrentSettingsToSinkWants(); } if (old_source != source && old_source) old_source->RemoveSink(sink_); @@ -50,7 +46,7 @@ void VideoSourceSinkController::PushSourceSinkSettings() { rtc::CritScope lock(&crit_); if (!source_) return; - source_->AddOrUpdateSink(sink_, CurrentSettingsToSinkWantsInternal()); + source_->AddOrUpdateSink(sink_, CurrentSettingsToSinkWants()); } VideoSourceRestrictions VideoSourceSinkController::restrictions() const { @@ -109,15 +105,9 @@ void VideoSourceSinkController::SetResolutionAlignment( resolution_alignment_ = resolution_alignment; } +// RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_) rtc::VideoSinkWants VideoSourceSinkController::CurrentSettingsToSinkWants() const { - rtc::CritScope lock(&crit_); - return CurrentSettingsToSinkWantsInternal(); -} - -// RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_) -rtc::VideoSinkWants -VideoSourceSinkController::CurrentSettingsToSinkWantsInternal() const { rtc::VideoSinkWants wants; wants.rotation_applied = rotation_applied_; // |wants.black_frames| is not used, it always has its default value false. @@ -134,25 +124,6 @@ VideoSourceSinkController::CurrentSettingsToSinkWantsInternal() const { ? static_cast(restrictions_.max_frame_rate().value()) : std::numeric_limits::max(); wants.resolution_alignment = resolution_alignment_; - { - // Clear any constraints from the current sink wants that don't apply to - // the used degradation_preference. - switch (degradation_preference_) { - case DegradationPreference::BALANCED: - break; - case DegradationPreference::MAINTAIN_FRAMERATE: - wants.max_framerate_fps = std::numeric_limits::max(); - break; - case DegradationPreference::MAINTAIN_RESOLUTION: - wants.max_pixel_count = std::numeric_limits::max(); - wants.target_pixel_count.reset(); - break; - case DegradationPreference::DISABLED: - wants.max_pixel_count = std::numeric_limits::max(); - wants.target_pixel_count.reset(); - wants.max_framerate_fps = std::numeric_limits::max(); - } - } wants.max_pixel_count = std::min(wants.max_pixel_count, rtc::dchecked_cast(pixels_per_frame_upper_limit_.value_or( diff --git a/video/video_source_sink_controller.h b/video/video_source_sink_controller.h index 79260363ea..379457cdf6 100644 --- a/video/video_source_sink_controller.h +++ b/video/video_source_sink_controller.h @@ -12,7 +12,6 @@ #define VIDEO_VIDEO_SOURCE_SINK_CONTROLLER_H_ #include "absl/types/optional.h" -#include "api/rtp_parameters.h" #include "api/video/video_frame.h" #include "api/video/video_sink_interface.h" #include "api/video/video_source_interface.h" @@ -30,12 +29,7 @@ class VideoSourceSinkController { VideoSourceSinkController(rtc::VideoSinkInterface* sink, rtc::VideoSourceInterface* source); - // TODO(https://crbug.com/webrtc/11222): Remove dependency on - // DegradationPreference! How degradation preference affects - // VideoSourceRestrictions should not be a responsibility of the controller, - // but of the resource adaptation module. - void SetSource(rtc::VideoSourceInterface* source, - DegradationPreference degradation_preference); + void SetSource(rtc::VideoSourceInterface* source); // Must be called in order for changes to settings to have an effect. This // allows you to modify multiple properties in a single push to the sink. void PushSourceSinkSettings(); @@ -55,15 +49,8 @@ class VideoSourceSinkController { void SetRotationApplied(bool rotation_applied); void SetResolutionAlignment(int resolution_alignment); - // TODO(https://crbug.com/webrtc/11222): Outside of testing, this is only used - // by OveruseFrameDetectorResourceAdaptationModule::RefreshTargetFramerate(). - // When the DegradationPreference logic has moved outside of this class, there - // will be no public need for this method other than testing reasons and this - // can be renamed "ForTesting". - rtc::VideoSinkWants CurrentSettingsToSinkWants() const; - private: - rtc::VideoSinkWants CurrentSettingsToSinkWantsInternal() const + rtc::VideoSinkWants CurrentSettingsToSinkWants() const RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_); // TODO(hbos): If everything is handled on the same sequence (i.e. @@ -72,7 +59,6 @@ class VideoSourceSinkController { mutable rtc::CriticalSection crit_; rtc::VideoSinkInterface* const sink_; rtc::VideoSourceInterface* source_ RTC_GUARDED_BY(&crit_); - DegradationPreference degradation_preference_ RTC_GUARDED_BY(&crit_); // Pixel and frame rate restrictions. VideoSourceRestrictions restrictions_ RTC_GUARDED_BY(&crit_); // Ensures that even if we are not restricted, the sink is never configured diff --git a/video/video_source_sink_controller_unittest.cc b/video/video_source_sink_controller_unittest.cc index 61cfafd45c..c4e2ea11d2 100644 --- a/video/video_source_sink_controller_unittest.cc +++ b/video/video_source_sink_controller_unittest.cc @@ -74,10 +74,6 @@ TEST(VideoSourceSinkControllerTest, VideoRestrictionsToSinkWants) { MockVideoSourceWithVideoFrame source; VideoSourceSinkController controller(&sink, &source); - // Balanced degradation preference gives us what we ask for. - EXPECT_CALL(source, AddOrUpdateSink(_, _)).Times(1); - controller.SetSource(&source, DegradationPreference::BALANCED); - VideoSourceRestrictions restrictions = controller.restrictions(); // max_pixels_per_frame() maps to |max_pixel_count|. restrictions.set_max_pixels_per_frame(42u); @@ -95,24 +91,9 @@ TEST(VideoSourceSinkControllerTest, VideoRestrictionsToSinkWants) { }); controller.PushSourceSinkSettings(); - // Disabled degradation preference makes the "wants" unconstrained despite our - // restrictions. - EXPECT_CALL(source, AddOrUpdateSink(_, _)).Times(1); - controller.SetSource(&source, DegradationPreference::DISABLED); - EXPECT_CALL(source, AddOrUpdateSink(_, _)) - .WillOnce([](rtc::VideoSinkInterface* sink, - const rtc::VideoSinkWants& wants) { - EXPECT_EQ(wants.max_pixel_count, kIntUnconstrained); - EXPECT_FALSE(wants.target_pixel_count.has_value()); - EXPECT_EQ(wants.max_framerate_fps, kIntUnconstrained); - }); - controller.PushSourceSinkSettings(); - - // pixels_per_frame_upper_limit() caps |max_pixel_count| regardless of - // degradation preferences. + // pixels_per_frame_upper_limit() caps |max_pixel_count|. controller.SetPixelsPerFrameUpperLimit(24); - // frame_rate_upper_limit() caps |max_framerate_fps| regardless of degradation - // preferences. + // frame_rate_upper_limit() caps |max_framerate_fps|. controller.SetFrameRateUpperLimit(10.0); EXPECT_CALL(source, AddOrUpdateSink(_, _)) diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index bdc0324cca..d1a01d0997 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -324,7 +324,6 @@ VideoStreamEncoder::VideoStreamEncoder( resource_adaptation_module_( std::make_unique( /*video_stream_encoder=*/this, - video_source_sink_controller_.get(), std::move(overuse_detector), encoder_stats_observer, /*adaptation_listener=*/this)), @@ -347,7 +346,7 @@ VideoStreamEncoder::~VideoStreamEncoder() { void VideoStreamEncoder::Stop() { RTC_DCHECK_RUN_ON(&thread_checker_); - video_source_sink_controller_->SetSource(nullptr, DegradationPreference()); + video_source_sink_controller_->SetSource(nullptr); encoder_queue_.PostTask([this] { RTC_DCHECK_RUN_ON(&encoder_queue_); resource_adaptation_module_->StopCheckForOveruse(); @@ -388,7 +387,7 @@ void VideoStreamEncoder::SetSource( rtc::VideoSourceInterface* source, const DegradationPreference& degradation_preference) { RTC_DCHECK_RUN_ON(&thread_checker_); - video_source_sink_controller_->SetSource(source, degradation_preference); + video_source_sink_controller_->SetSource(source); resource_adaptation_module_->SetHasInputVideoAndDegradationPreference( source, degradation_preference); encoder_queue_.PostTask([this, degradation_preference] { @@ -1742,7 +1741,9 @@ void VideoStreamEncoder::TriggerAdaptUp( void VideoStreamEncoder::OnVideoSourceRestrictionsUpdated( VideoSourceRestrictions restrictions) { - RTC_DCHECK_RUN_ON(&encoder_queue_); + // TODO(https://crbug.com/webrtc/11222): DCHECK that we are using the + // |encoder_queue_| when OnVideoSourceRestrictionsUpdated() is no longer + // invoked off this thread due to VideoStreamEncoder::SetSource() stuff. video_source_sink_controller_->SetRestrictions(std::move(restrictions)); video_source_sink_controller_->PushSourceSinkSettings(); }