Skip to content

Commit

Permalink
[Adaptation] Make Resources reference counted and add more DCHECKs.
Browse files Browse the repository at this point in the history
In a future CL, adaptation processing and stream encoder resource
management will happen on different task queues. When this is the case,
asynchronous tasks will be posted in both directions and some resources
will have internal states used on multiple threads.

This CL makes the Resource class reference counted in order to support
posting tasks to a different threads without risk of use-after-free
when a posted task is executed with a delay. This is preferred over
WeakPtr strategies because WeakPtrs are single-threaded and preferred
over raw pointer usage because the reference counted approach enables
more compile-time and run-time assurance. This is also "future proof";
when resources can be injected through public APIs, ownership needs to
be shared between libwebrtc and the application (e.g. Chrome).

To reduce the risk of making mistakes in the future CL, sequence
checkers and task queue DCHECKs are added as well as other DCHECKs to
make sure things have been cleaned up before destruction, e.g:
- Processor gets a sequence checker. It is entirely single-threaded.
- Processor must not have any attached listeners or resources on
  destruction.
- Resources must not have any listeners on destruction.
- The Manager, EncodeUsageResource and QualityScalerResource DCHECKs
  they are running on the encoder queue.
- TODOs are added illustrating where we want to add PostTasks in the
  future CL.

Lastly, upon VideoStreamEncoder::Stop() we delete the
ResourceAdaptationProcessor. Because the Processor is already used in
posted tasks, some if statements are added to ensure the Processor is
not used after destruction.

Bug: webrtc:11542, webrtc:11520
Change-Id: Ibaa8a61d86d87a71f477d1075a117c28d9d2d285
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/174760
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Evan Shrubsole <eshr@google.com>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31217}
  • Loading branch information
henbos authored and Commit Bot committed May 11, 2020
1 parent cc57b93 commit c55516d
Show file tree
Hide file tree
Showing 21 changed files with 663 additions and 306 deletions.
4 changes: 4 additions & 0 deletions call/adaptation/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,17 @@ rtc_library("resource_adaptation") {
]
deps = [
"../../api:rtp_parameters",
"../../api:scoped_refptr",
"../../api/video:video_adaptation",
"../../api/video:video_frame",
"../../api/video:video_stream_encoder",
"../../api/video_codecs:video_codecs_api",
"../../modules/video_coding:video_coding_utility",
"../../rtc_base:checks",
"../../rtc_base:rtc_base_approved",
"../../rtc_base:rtc_task_queue",
"../../rtc_base/experiments:balanced_degradation_settings",
"../../rtc_base/synchronization:sequence_checker",
"//third_party/abseil-cpp/absl/algorithm:container",
"//third_party/abseil-cpp/absl/types:optional",
]
Expand All @@ -56,6 +59,7 @@ if (rtc_include_tests) {
deps = [
":resource_adaptation",
":resource_adaptation_test_utilities",
"../../api:scoped_refptr",
"../../api/video:video_adaptation",
"../../api/video_codecs:video_codecs_api",
"../../rtc_base:checks",
Expand Down
11 changes: 7 additions & 4 deletions call/adaptation/resource.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ ResourceListener::~ResourceListener() {}

Resource::Resource() : usage_state_(absl::nullopt), listener_(nullptr) {}

Resource::~Resource() {}
Resource::~Resource() {
RTC_DCHECK(!listener_)
<< "There is a listener depending on a Resource being destroyed.";
}

void Resource::SetResourceListener(ResourceListener* listener) {
// If you want to change listener you need to unregister the old listener by
Expand All @@ -40,21 +43,21 @@ bool Resource::IsAdaptationUpAllowed(
const VideoStreamInputState& input_state,
const VideoSourceRestrictions& restrictions_before,
const VideoSourceRestrictions& restrictions_after,
const Resource& reason_resource) const {
rtc::scoped_refptr<Resource> reason_resource) const {
return true;
}

void Resource::OnAdaptationApplied(
const VideoStreamInputState& input_state,
const VideoSourceRestrictions& restrictions_before,
const VideoSourceRestrictions& restrictions_after,
const Resource& reason_resource) {}
rtc::scoped_refptr<Resource> reason_resource) {}

void Resource::OnResourceUsageStateMeasured(ResourceUsageState usage_state) {
usage_state_ = usage_state;
if (!listener_)
return;
listener_->OnResourceUsageStateMeasured(*this);
listener_->OnResourceUsageStateMeasured(this);
}

} // namespace webrtc
15 changes: 9 additions & 6 deletions call/adaptation/resource.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@
#include <vector>

#include "absl/types/optional.h"
#include "api/scoped_refptr.h"
#include "call/adaptation/video_source_restrictions.h"
#include "call/adaptation/video_stream_input_state.h"
#include "rtc_base/ref_count.h"

namespace webrtc {

Expand All @@ -34,15 +36,16 @@ class ResourceListener {
virtual ~ResourceListener();

// Informs the listener of a new measurement of resource usage. This means
// that |resource.usage_state()| is now up-to-date.
virtual void OnResourceUsageStateMeasured(const Resource& resource) = 0;
// that |resource->usage_state()| is now up-to-date.
virtual void OnResourceUsageStateMeasured(
rtc::scoped_refptr<Resource> resource) = 0;
};

class Resource {
class Resource : public rtc::RefCountInterface {
public:
// By default, usage_state() is null until a measurement is made.
Resource();
virtual ~Resource();
~Resource() override;

void SetResourceListener(ResourceListener* listener);

Expand All @@ -56,12 +59,12 @@ class Resource {
const VideoStreamInputState& input_state,
const VideoSourceRestrictions& restrictions_before,
const VideoSourceRestrictions& restrictions_after,
const Resource& reason_resource) const;
rtc::scoped_refptr<Resource> reason_resource) const;
virtual void OnAdaptationApplied(
const VideoStreamInputState& input_state,
const VideoSourceRestrictions& restrictions_before,
const VideoSourceRestrictions& restrictions_after,
const Resource& reason_resource);
rtc::scoped_refptr<Resource> reason_resource);

virtual std::string name() const = 0;

Expand Down
Loading

0 comments on commit c55516d

Please sign in to comment.