[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}
diff --git a/call/adaptation/resource.cc b/call/adaptation/resource.cc
index bb3e63d..0ffc78b 100644
--- a/call/adaptation/resource.cc
+++ b/call/adaptation/resource.cc
@@ -17,30 +17,25 @@
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::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::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::UnregisterListener(ResourceListener* listener) {
- RTC_DCHECK(listener);
- auto it = absl::c_find(listeners_, listener);
- if (it != listeners_.end())
- listeners_.erase(it);
-}
-
-ResourceUsageState Resource::usage_state() const {
+absl::optional<ResourceUsageState> Resource::usage_state() const {
return usage_state_;
}
+void Resource::ClearUsageState() {
+ usage_state_ = absl::nullopt;
+}
+
bool Resource::IsAdaptationUpAllowed(
const VideoStreamInputState& input_state,
const VideoSourceRestrictions& restrictions_before,
@@ -51,15 +46,10 @@
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 7c802eb4..bde8cd7 100644
--- a/call/adaptation/resource.h
+++ b/call/adaptation/resource.h
@@ -25,10 +25,7 @@
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 @@
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);
- ResourceUsageState usage_state() const;
+ absl::optional<ResourceUsageState> usage_state() const;
+ void ClearUsageState();
+
// 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 @@
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 d75f62b..e69b205 100644
--- a/call/adaptation/resource_adaptation_processor.cc
+++ b/call/adaptation/resource_adaptation_processor.cc
@@ -42,13 +42,13 @@
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 @@
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 @@
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 @@
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 39d8f3d..1cb5375 100644
--- a/call/adaptation/resource_unittest.cc
+++ b/call/adaptation/resource_unittest.cc
@@ -26,10 +26,10 @@
(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 @@
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 243f1e0..c7114a8 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 852428c..dd311424 100644
--- a/call/adaptation/test/fake_resource.h
+++ b/call/adaptation/test/fake_resource.h
@@ -20,8 +20,7 @@
// 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 44ac6e1..38bd28b 100644
--- a/video/video_stream_encoder_unittest.cc
+++ b/video/video_stream_encoder_unittest.cc
@@ -159,12 +159,9 @@
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(),
