aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorHenrik Boström <hbos@webrtc.org>2020-04-20 12:04:12 +0200
committerCommit Bot <commit-bot@chromium.org>2020-04-20 10:54:53 +0000
commitdc4f75f7eed26fff21d475ddacaba073eab3c8a0 (patch)
tree7bbc70d639d2df0e39cd390f3e5049c28753eb70
parent4c3a7dbe1482e389b54c799f332c8ec6e5ac6af9 (diff)
downloadwebrtc-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.cc38
-rw-r--r--call/adaptation/resource.h24
-rw-r--r--call/adaptation/resource_adaptation_processor.cc25
-rw-r--r--call/adaptation/resource_unittest.cc16
-rw-r--r--call/adaptation/test/fake_resource.cc9
-rw-r--r--call/adaptation/test/fake_resource.h3
-rw-r--r--video/video_stream_encoder_unittest.cc7
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(),