diff options
author | Henrik Boström <hbos@webrtc.org> | 2020-04-20 12:04:12 +0200 |
---|---|---|
committer | Commit Bot <commit-bot@chromium.org> | 2020-04-20 10:54:53 +0000 |
commit | dc4f75f7eed26fff21d475ddacaba073eab3c8a0 (patch) | |
tree | 7bbc70d639d2df0e39cd390f3e5049c28753eb70 | |
parent | 4c3a7dbe1482e389b54c799f332c8ec6e5ac6af9 (diff) | |
download | webrtc-dc4f75f7eed26fff21d475ddacaba073eab3c8a0.tar.gz |
[Adaptation] Make ResourceUsageState nullable, remove kStable.
This CL is part of the Call-Level Adaptation Processing design doc:
https://docs.google.com/document/d/1ZyC26yOCknrrcYa839ZWLxD6o6Gig5A3lVTh4E41074/edit?usp=sharing
The ResourceUsageState was written as: {kOveruse, kStable, kUnderuse}.
The assumption was that if a resource neither wanted to adapt up or
down it would report kStable. But with the addition of
Resource::IsAdaptationUpAllowed() (prior CL) the notion of being
"stable" was already captured outside of ResourceUsageState.
Furthermore, kStable failed to capture what IsAdaptationUpAllowed() did
not: whether we can go up depends on the resulting resolution or frame
rate (restrictions_after). Perhaps we can go up a little, but not a lot.
This CL also adds Resource::ClearUsageState(). After applying an
adaptation, all usage states become invalidated (new measurements are
needed to know if we are still over- or underusing). This was always
the case, but prior to this CL this was not accurately reflected in the
Resource::usage_state() in-between measurements.
Bug: webrtc:11172
Change-Id: I140ff3114025b7732e530564690783e168d2509b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/173088
Reviewed-by: Evan Shrubsole <eshr@google.com>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31110}
-rw-r--r-- | call/adaptation/resource.cc | 38 | ||||
-rw-r--r-- | call/adaptation/resource.h | 24 | ||||
-rw-r--r-- | call/adaptation/resource_adaptation_processor.cc | 25 | ||||
-rw-r--r-- | call/adaptation/resource_unittest.cc | 16 | ||||
-rw-r--r-- | call/adaptation/test/fake_resource.cc | 9 | ||||
-rw-r--r-- | call/adaptation/test/fake_resource.h | 3 | ||||
-rw-r--r-- | video/video_stream_encoder_unittest.cc | 7 |
7 files changed, 53 insertions, 69 deletions
diff --git a/call/adaptation/resource.cc b/call/adaptation/resource.cc index bb3e63deb5..0ffc78b4de 100644 --- a/call/adaptation/resource.cc +++ b/call/adaptation/resource.cc @@ -17,28 +17,23 @@ namespace webrtc { ResourceListener::~ResourceListener() {} -Resource::Resource() : usage_state_(ResourceUsageState::kStable) {} +Resource::Resource() : usage_state_(absl::nullopt), listener_(nullptr) {} -Resource::~Resource() { - RTC_DCHECK(listeners_.empty()); -} +Resource::~Resource() {} -void Resource::RegisterListener(ResourceListener* listener) { - RTC_DCHECK(listener); - RTC_DCHECK(absl::c_find(listeners_, listener) == listeners_.end()) - << "ResourceListener was added twice."; - listeners_.push_back(listener); +void Resource::SetResourceListener(ResourceListener* listener) { + // If you want to change listener you need to unregister the old listener by + // setting it to null first. + RTC_DCHECK(!listener_ || !listener) << "A listener is already set"; + listener_ = listener; } -void Resource::UnregisterListener(ResourceListener* listener) { - RTC_DCHECK(listener); - auto it = absl::c_find(listeners_, listener); - if (it != listeners_.end()) - listeners_.erase(it); +absl::optional<ResourceUsageState> Resource::usage_state() const { + return usage_state_; } -ResourceUsageState Resource::usage_state() const { - return usage_state_; +void Resource::ClearUsageState() { + usage_state_ = absl::nullopt; } bool Resource::IsAdaptationUpAllowed( @@ -51,15 +46,10 @@ bool Resource::IsAdaptationUpAllowed( ResourceListenerResponse Resource::OnResourceUsageStateMeasured( ResourceUsageState usage_state) { - ResourceListenerResponse response = ResourceListenerResponse::kNothing; usage_state_ = usage_state; - for (auto* listener : listeners_) { - ResourceListenerResponse listener_response = - listener->OnResourceUsageStateMeasured(*this); - if (listener_response != ResourceListenerResponse::kNothing) - response = listener_response; - } - return response; + if (!listener_) + return ResourceListenerResponse::kNothing; + return listener_->OnResourceUsageStateMeasured(*this); } } // namespace webrtc diff --git a/call/adaptation/resource.h b/call/adaptation/resource.h index 7c802eb411..bde8cd7597 100644 --- a/call/adaptation/resource.h +++ b/call/adaptation/resource.h @@ -25,10 +25,7 @@ class Resource; enum class ResourceUsageState { // Action is needed to minimze the load on this resource. kOveruse, - // No action needed for this resource, increasing the load on this resource - // is not allowed. - kStable, - // Increasing the load on this resource is allowed. + // Increasing the load on this resource is desired, if possible. kUnderuse, }; @@ -69,22 +66,17 @@ class ResourceListener { const Resource& resource) = 0; }; -// A Resource is something which can be measured as "overused", "stable" or -// "underused". When the resource usage changes, listeners of the resource are -// informed. -// -// Implementations of this interface are responsible for performing resource -// usage measurements and invoking OnResourceUsageStateMeasured(). class Resource { public: - // By default, usage_state() is kStable until a measurement is made. + // By default, usage_state() is null until a measurement is made. Resource(); virtual ~Resource(); - void RegisterListener(ResourceListener* listener); - void UnregisterListener(ResourceListener* listener); + void SetResourceListener(ResourceListener* listener); + + absl::optional<ResourceUsageState> usage_state() const; + void ClearUsageState(); - ResourceUsageState usage_state() const; // This method allows the Resource to reject a proposed adaptation in the "up" // direction if it predicts this would cause overuse of this resource. The // default implementation unconditionally returns true (= allowed). @@ -104,8 +96,8 @@ class Resource { ResourceUsageState usage_state); private: - ResourceUsageState usage_state_; - std::vector<ResourceListener*> listeners_; + absl::optional<ResourceUsageState> usage_state_; + ResourceListener* listener_; }; } // namespace webrtc diff --git a/call/adaptation/resource_adaptation_processor.cc b/call/adaptation/resource_adaptation_processor.cc index d75f62b2f4..e69b205b99 100644 --- a/call/adaptation/resource_adaptation_processor.cc +++ b/call/adaptation/resource_adaptation_processor.cc @@ -42,13 +42,13 @@ ResourceAdaptationProcessor::effective_degradation_preference() const { void ResourceAdaptationProcessor::StartResourceAdaptation() { for (auto* resource : resources_) { - resource->RegisterListener(this); + resource->SetResourceListener(this); } } void ResourceAdaptationProcessor::StopResourceAdaptation() { for (auto* resource : resources_) { - resource->UnregisterListener(this); + resource->SetResourceListener(nullptr); } } @@ -106,13 +106,10 @@ void ResourceAdaptationProcessor::MaybeUpdateVideoSourceRestrictions( ResourceListenerResponse ResourceAdaptationProcessor::OnResourceUsageStateMeasured( const Resource& resource) { - switch (resource.usage_state()) { + RTC_DCHECK(resource.usage_state().has_value()); + switch (resource.usage_state().value()) { case ResourceUsageState::kOveruse: return OnResourceOveruse(resource); - case ResourceUsageState::kStable: - // TODO(https://crbug.com/webrtc/11172): Delete kStable in favor of null. - RTC_NOTREACHED(); - return ResourceListenerResponse::kNothing; case ResourceUsageState::kUnderuse: OnResourceUnderuse(resource); return ResourceListenerResponse::kNothing; @@ -129,6 +126,13 @@ bool ResourceAdaptationProcessor::HasSufficientInputForAdaptation( void ResourceAdaptationProcessor::OnResourceUnderuse( const Resource& reason_resource) { + // Clear all usage states. In order to re-run adaptation logic, resources need + // to provide new resource usage measurements. + // TODO(hbos): Support not unconditionally clearing usage states by having the + // ResourceAdaptationProcessor check in on its resources at certain intervals. + for (Resource* resource : resources_) { + resource->ClearUsageState(); + } VideoStreamInputState input_state = input_state_provider_->InputState(); if (effective_degradation_preference_ == DegradationPreference::DISABLED || !HasSufficientInputForAdaptation(input_state)) { @@ -163,6 +167,13 @@ void ResourceAdaptationProcessor::OnResourceUnderuse( ResourceListenerResponse ResourceAdaptationProcessor::OnResourceOveruse( const Resource& reason_resource) { + // Clear all usage states. In order to re-run adaptation logic, resources need + // to provide new resource usage measurements. + // TODO(hbos): Support not unconditionally clearing usage states by having the + // ResourceAdaptationProcessor check in on its resources at certain intervals. + for (Resource* resource : resources_) { + resource->ClearUsageState(); + } VideoStreamInputState input_state = input_state_provider_->InputState(); if (!input_state.has_input()) { return ResourceListenerResponse::kQualityScalerShouldIncreaseFrequency; diff --git a/call/adaptation/resource_unittest.cc b/call/adaptation/resource_unittest.cc index 39d8f3dc9b..1cb53756dc 100644 --- a/call/adaptation/resource_unittest.cc +++ b/call/adaptation/resource_unittest.cc @@ -26,10 +26,10 @@ class MockResourceListener : public ResourceListener { (const Resource& resource)); }; -TEST(ResourceTest, AddingListenerReceivesCallbacks) { +TEST(ResourceTest, RegisteringListenerReceivesCallbacks) { StrictMock<MockResourceListener> resource_listener; - FakeResource fake_resource(ResourceUsageState::kStable); - fake_resource.RegisterListener(&resource_listener); + FakeResource fake_resource("FakeResource"); + fake_resource.SetResourceListener(&resource_listener); EXPECT_CALL(resource_listener, OnResourceUsageStateMeasured(_)) .Times(1) .WillOnce([](const Resource& resource) { @@ -37,14 +37,14 @@ TEST(ResourceTest, AddingListenerReceivesCallbacks) { return ResourceListenerResponse::kNothing; }); fake_resource.set_usage_state(ResourceUsageState::kOveruse); - fake_resource.UnregisterListener(&resource_listener); + fake_resource.SetResourceListener(nullptr); } -TEST(ResourceTest, RemovingListenerStopsCallbacks) { +TEST(ResourceTest, UnregisteringListenerStopsCallbacks) { StrictMock<MockResourceListener> resource_listener; - FakeResource fake_resource(ResourceUsageState::kStable); - fake_resource.RegisterListener(&resource_listener); - fake_resource.UnregisterListener(&resource_listener); + FakeResource fake_resource("FakeResource"); + fake_resource.SetResourceListener(&resource_listener); + fake_resource.SetResourceListener(nullptr); EXPECT_CALL(resource_listener, OnResourceUsageStateMeasured(_)).Times(0); fake_resource.set_usage_state(ResourceUsageState::kOveruse); } diff --git a/call/adaptation/test/fake_resource.cc b/call/adaptation/test/fake_resource.cc index 243f1e04ec..c7114a8cdc 100644 --- a/call/adaptation/test/fake_resource.cc +++ b/call/adaptation/test/fake_resource.cc @@ -14,18 +14,13 @@ namespace webrtc { -FakeResource::FakeResource(ResourceUsageState usage_state) - : FakeResource(usage_state, "FakeResource") {} +FakeResource::FakeResource(std::string name) + : Resource(), name_(std::move(name)) {} FakeResource::~FakeResource() {} void FakeResource::set_usage_state(ResourceUsageState usage_state) { last_response_ = OnResourceUsageStateMeasured(usage_state); } -FakeResource::FakeResource(ResourceUsageState usage_state, - const std::string& name) - : Resource(), name_(name) { - set_usage_state(usage_state); -} } // namespace webrtc diff --git a/call/adaptation/test/fake_resource.h b/call/adaptation/test/fake_resource.h index 852428c7e3..dd31142469 100644 --- a/call/adaptation/test/fake_resource.h +++ b/call/adaptation/test/fake_resource.h @@ -20,8 +20,7 @@ namespace webrtc { // Fake resource used for testing. class FakeResource : public Resource { public: - explicit FakeResource(ResourceUsageState usage_state); - FakeResource(ResourceUsageState usage_state, const std::string& name); + explicit FakeResource(std::string name); ~FakeResource() override; void set_usage_state(ResourceUsageState usage_state); diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 44ac6e1390..38bd28b24b 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -159,12 +159,9 @@ class VideoStreamEncoderUnderTest : public VideoStreamEncoder { overuse_detector_proxy_ = new CpuOveruseDetectorProxy(stats_proxy)), task_queue_factory), - fake_cpu_resource_( - std::make_unique<FakeResource>(ResourceUsageState::kStable, - "FakeResource[CPU]")), + fake_cpu_resource_(std::make_unique<FakeResource>("FakeResource[CPU]")), fake_quality_resource_( - std::make_unique<FakeResource>(ResourceUsageState::kStable, - "FakeResource[QP]")) { + std::make_unique<FakeResource>("FakeResource[QP]")) { InjectAdaptationResource(fake_quality_resource_.get(), VideoAdaptationReason::kQuality); InjectAdaptationResource(fake_cpu_resource_.get(), |