This introduces a new class for encapsulating the QualityRampupExperiment
R=hbos@webrtc.org
Bug: webrtc:11553
Change-Id: If2f2347cdcbd0c79821355f90e2d7ad3171143b5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176363
Commit-Queue: Evan Shrubsole <eshr@google.com>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31531}
When a resource is removed from the ResourceAdaptationProcessor,
and the resource is the most limited resource, we reset the current
adaptation to the next most limited resource level. In the case that
this resource is the only adapted resource then we reset all adaptations.
Bug: webrtc:11636
Change-Id: I29acc5a3934f42f00db79b3bb2171156b1313937
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176406
Commit-Queue: Evan Shrubsole <eshr@google.com>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31520}
This CL adds AddAdaptationResource to Call and
AddAdaptationResource/GetAdaptationResources method to relevant
VideoSendStream and VideoStreamEncoder interfaces and implementations.
Unittests are added to ensure that resources can be added to the Call
both before and after the creation of a VideoSendStream and that the
resources always gets added to the streams.
In a follow-up CL, we will continue to plumb the resources all the way
to PeerConnectionInterface, and an integration test will then be added
to ensure that injected resources are capable of triggering adaptation.
Bug: webrtc:11525
Change-Id: I499e9c23c3e359df943414d420b2e0ce2e9b2d56
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/177002
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Evan Shrubsole <eshr@google.com>
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31499}
A more detailed explaination is in the bug, but this changes
the way that adaptation happens when multiple resources are
limited. Only the one that is most limited can trigger an
adaptation up. If multiple resources are most limited both
need to underuse to adapt up.
Some of the changes in this patch to make it all work:
* VideoStreamEncoder unittests that did not reflect this
new behaviour have been changed.
* PeekNextRestrictions returns the adaptation counters as
well as the restrictions.
* Adaptation statstics have changed so that when adapting
up all resources are tagged as triggering the adaptation.
Additionally the statistics for the current adaptation is
now the total number of adaptations per reason, rather then
the number of adaptations due to that reason.
* PreventAdaptUpDueToActiveCounts is removed as most limited
resource is a strong implementation of that.
Bug: webrtc:11553
Change-Id: If1545a201c8e019598edf82657a1befde8b05268
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176128
Commit-Queue: Evan Shrubsole <eshr@google.com>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31497}
Unit test added to verify root cause is fixed.
Scenario test added to verify high-level behavior.
Bug: webrtc:11654
Change-Id: I1ad6e2750f5272e86b4198749edbbf5dfd8315c4
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176564
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31462}
The 'Module' part of the implementation must not be
called via the RtpRtcp interface, but is rather a part of
the contract with ProcessThread. That in turn is an
implementation detail for how timers are currently implemented
in the default implementation.
Along the way I'm deprecating away the factory function which
was inside the interface and tied it to one specific implementation.
Instead, I'm moving that to the implementation itself and down the
line, we don't have to go through it if we just want to create an
instance of the class.
The key change is in rtp_rtcp.h and the new rtp_rtcp_interface.h
header file (things moved from rtp_rtcp.h), the rest falls from that.
Change-Id: I294f13e947b9e3e4e649400ee94a11a81e8071ce
Bug: webrtc:11581
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176419
Reviewed-by: Magnus Flodman <mflodman@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31440}
this trial is by default on for three months since
https://webrtc-review.googlesource.com/c/src/+/168661
Bug: webrtc:11503
Change-Id: I8f2e0996fd1c77113715628198a409f12a525d51
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176242
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31424}
The method is being used externally to create instances
of the deprecated internal implementation.
Instead, I'm moving how we instantiate the internal implementation into
the implementation itself and move towards keeping the interface
separate from a single implementation.
Change-Id: I743aa86dc4c812b545699c546c253c104719260e
Bug: webrtc:11581
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176404
Commit-Queue: Tommi <tommi@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31420}
This is a prerequisite to implementing it externally.
Bug: webrtc:11525
Change-Id: I9cb3b4418396485d3eb9f25cafa51cbff6db7817
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176401
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Reviewed-by: Evan Shrubsole <eshr@google.com>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31418}
ResourceListener::OnResourceUsageStateMeasured() now takes
ResourceUsageState as argument, making Resource::UsageState()
superfluous.
With the existing "fire-and-forget" behavior of always clearing usage
state on reacting to a signal, there is no longer a need to call
ClearUsageState() so this too is removed. (We may want to have a
callback in the future to hint to the Resource that it is a good idea
to clear internal measurement samples, i.e. because the load of the
system is about to change, but we can revisit that when we need it.)
Moving the usage state to the callback has the benefit of getting rid
of the assumption that UsageState() has to return the same value every
time it is called in the same task.
This CL is also the final nail in the coffin for Resource needing to
know about the adaptation task queue: ResourceAdaptationProcessor's
ResourceListener now takes care of posting to the adaptation task
queue. To support this, the processor's SequenceChecker is replaced
by a TaskQueueBase pointer.
Bug: webrtc:11525, webrtc:11618
Change-Id: I2277e71cc3759c85b62465020935603f03792c94
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176376
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Evan Shrubsole <eshr@google.com>
Cr-Commit-Position: refs/heads/master@{#31416}
IsAdaptationUpAllowed is moved from Resource to AdaptationConstraint.
OnAdaptationApplied is moved from Resource to AdaptationListener.
In a future CL, Resource will be moved to api/, but
AdaptationConstraint and AdaptationListener will stay in call/.
The processor, encode stream and manager are updated to keep track of
both resources, constraints and listeners. Fakes and tests are updated.
After this CL, the manager's inner classes that prevent adaptation
implement AdaptationConstraint instead of Resource.
Bug: webrtc:11525
Change-Id: Ie9cd5b1ba7d8e161951e131ab8f6bd9d5cf765bf
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176368
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Evan Shrubsole <eshr@google.com>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31409}
This CL is in preparation for moving Resource to the api/ folder. It
does not move it, but makes it such that the moving CL can be a pure
move.
In order to do this, we must stop depending on rtc_base/rtc::TaskQueue
in favor of api/webrtc::TaskQueueBase.
There are also other rtc_base/ dependencies that we do not want to
expose to the api/ folder, like critical sections and thread
annotations which are not publically exposed. To get around this, we
make Resource an abstract interface and move all of the base class
functionality into a new non-api/ class: VideoStreamEncoderResource.
The Resource now has Register/UnregisterAdaptationTaskQueue() methods.
By explicitly unregistering, we can ensure validity of the pointer even
if the Resource outlives the PeerConnection. While public interface
methods are only to be called on the adaptation task queue, posting to
the task queue happens off-queue, so a |lock_| is introduced to guard
it.
Bug: webrtc:11525
Change-Id: I50b3a30960cdec9032016c779b47001c01dad32f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176320
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@{#31402}
This CL moves webrtc::NackModule to a deprecated folder and annotates
the type with RTC_DEPRECATED.
Since the header should not be used outside of WebRTC, this CL doesn't
created a forward header.
Bug: webrtc:11611
Change-Id: I4d5899d473d78b8c7f4a6a018e2805648244b5f1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176360
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Reviewed-by: Tommi <tommi@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31394}
This CL adds a 1 second cooldown period for QualityScalerResource to
signal kUnderuse due to being disabled.
If underuse is signaled every frame, any RTC_LOGging performed by the
ResourceAdaptationProcessor would become very spammy.
Plus we don't need to adapt every single frame.
Bug: webrtc:11616
Change-Id: Id76e5ca39a5e5dac9b71fdab79fb4f3dd5aeab1f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176228
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31374}
This only replaces the methods with the signatures VerifyX(wants,
value). This ensures that failing expectations give the corrent line,
where previously it gave the line in the helper methods.
Bug: None
Change-Id: I89e883ccd15b3ea5ce364d2c16b0c3ac219f139c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175912
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Evan Shrubsole <eshr@google.com>
Cr-Commit-Position: refs/heads/master@{#31345}
This will help show what the restrictions are before
and after they transform into sink wants.
These will not be so spammy as adaptations do not happen
very often.
Bug: None
Change-Id: Ib72b313f68c6934d7833d8a3f14ce00df602832b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175800
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Evan Shrubsole <eshr@google.com>
Cr-Commit-Position: refs/heads/master@{#31341}
This ended up with needing to fork the current implementation
in order to not break downstream projects that were inheriting
from it. While those get updated, we'll move on with the forked
class.
Bug: webrtc:11581,b/8278269
Change-Id: I05b596cbda71aa5b72894c31a7119d17d4761883
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175500
Reviewed-by: Per Kjellander <perkj@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31334}
Some VideoStreamEncoderTests such as
AdaptsFramerateForLowQuality_MaintainResolutionMode had been observed
to be flaky. In a local run, this test failed 1/12000 times, but on
bots it may have failed more often.
All tests could sometimes fail due to expecting something to be the
case without waiting for that thing to happen, which was made evident
when adding SleepMs() at strategic points in the code and running the
tests locally.
With this CL, the tests should no longer be flaky after having added
waiting for adaptation to be applied or EXPECT_TRUE_WAIT for sink wants
to have the appropriate values.
Bug: webrtc:11586
Change-Id: I60e76c742d9ccc8305feb14a834a4f61a60a62a3
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175654
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31327}
No code reads these properties.
They are recalculated by RtpPayloadParams
and set directly into RTPVideoHeader::generic.
Bug: webrtc:10342
Change-Id: Ib7bda4e6e5b0d0b3a585a848e3312fb66f8ae36d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175127
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31269}
Define VideoHeaderMetadata, containing a subset of the metadata in RTP
video header, and expose it the TransformableVideoFrameInterface, to
enable web application to compute additional data according to their own
logic, and eventually remove GetAdditionalData() from the interface.
Bug: chromium:1069295
Change-Id: Id85b494a72cfd8bdd4c0614844b9f0ffae98c956
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/174822
Commit-Queue: Marina Ciocea <marinaciocea@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Magnus Flodman <mflodman@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31265}
This test ensures that when changing degradation preference,
a resource that was previously downgraded in a different degradation
preference can not adapt up.
Bug: webrtc:11522, webrtc:11523
Change-Id: Id362530408db4c49b0d0b2516be9a11ccc7c8f37
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175012
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Evan Shrubsole <eshr@google.com>
Cr-Commit-Position: refs/heads/master@{#31250}
(Misc cleanup associated with
https://webrtc-review.googlesource.com/c/src/+/174441.)
This test was previously assuming what when QP usage is handled it
first posts to the adaptation queue and then back with the result from
the encoder queue.
While the assumption is correct it is not an implementation detail that
the test was trying to assert, nor do we need such a test.
TriggerQualityScalerHighQpAndReturnIfQpSamplesShouldBeCleared() is
updated to wait for an event associated with QP having been handled,
which is all that the test really cares about.
Bug: webrtc:11542, webrtc:11520
Change-Id: I3286c3ab631f09c43abe0fd59f31c3997aedd9f8
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175004
Commit-Queue: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Evan Shrubsole <eshr@google.com>
Cr-Commit-Position: refs/heads/master@{#31243}
ScopedTaskSafety simplifies usage of PendingTaskSafetyFlag,
so this CL also includes ToQueuedTask support for ScopedTaskSafety
and test updates.
This is following up on feedback in the following CL:
https://webrtc-review.googlesource.com/c/src/+/174262
Change-Id: Idd38dfc1914b24a05fdc4ad256b409dcf1795fc0
Bug: none
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/174740
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31241}
This CL unblocks future Call-Level Mitigation strategies by moving the
ResourceAdaptationProcessor to a separate task queue. This signifies a
major milestone in the new resource adaptation architecture because
with this CL the threading model is in place and moving the Processor
to the Call and increasing its responsibilities is made possible.
In this CL, we still have one Processor per VideoStreamEncoder and the
VideoStreamEncoder is responsible for the creation and the destruction
of its Processor and that Processor's task queue. But the PostTasks are
in place and the decision-making is executed on a separate queue.
This CL:
- Moves ResourceAdaptationProcessor to an adaptation task queue.
It continues to be entirely single-threaded, but now operates on a
separate task queue.
- Makes Resources thread-safe: Interaction with the Processor, i.e.
OnResourceUsageStateMeasured() and IsAdaptationUpAllowed(), happens
on the adaptation task queue. State updates are pushed from the
encoder task queue with PostTasks.
- QualityScalerResource operates on both task queues; the QP usage
callbacks are invoked asynchronously.
- The VideoStreamEncoderResourceManager operates on the encoder task
queue with the following exceptions:
1) Its resources are accessible on any thread (using a mutex). This
is OK because resources are reference counted and thread safe.
This aids adding and removing resources to the Processor on the
adaptation task queue.
2) |active_counts_| is moved to the adaptation task queue. This makes
it possible for PreventAdaptUpDueToActiveCounts to run
IsAdaptationUpAllowed() on the adaptation task queue.
A side-effect of this is that some stats reporting now happen on
the adaptation task queue, but that is OK because
VideoStreamEncoderObserver is thread-safe.
The Manager is updated to take the new threading model into account:
- OnFrameDroppedDueToSize() posts to the adaptation task queue to
invoke the Processor.
- OnVideoSourceRestrictionsUpdated(), now invoked on the adaptation
task queue, updates |active_counts_| synchronously but posts to the
encoder task queue to update video source restrictions (which it
only uses to calculate target frame rate).
- MaybePerformQualityRampupExperiment() posts to the adaptation task
queue to maybe reset video source restrictions on the Processor.
|quality_rampup_done_| is made std::atomic.
Bug: webrtc:11542, webrtc:11520
Change-Id: I1cfd76e0cd42f006a6d2527f5aa2aeb5266ba6d6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/174441
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@{#31231}
This CL makes the VideoStreamEncoderResourceManager's inner Resources
(PreventAdaptUpDueToActiveCounts,
PreventIncreaseResolutionDueToBitrateResource and
PreventAdaptUpInBalancedResource) not directly depend on any of the
manager's states that will continue to live on the encoder task queue
when the adaptation task queue is introduced in the next CL.
PreventAdaptUpDueToActiveCounts depends on effective degradation
preference, which it can get from the Processor, and the active counts,
which will move to the adaptation queue and is safe to use.
PreventIncreaseResolutionDueToBitrateResource depends on encoder
settings and target bitrate. This Resource now listens to these states
being updated, which may be implemented with a PostTask when the
adaptation queue is added.
PreventAdaptUpInBalancedResource depends on the effective degradation
preference, which it can get from the Processor; balanced settings,
which is a const readonly struct (thread-safe); and encoder target
bitrate, which it listens for being updated (to be PostTask'ed).
All resources depends on GetReasonFromResource() which will be callable
from the adaptation queue.
Bug: webrtc:11542, webrtc:11520
Change-Id: Ifa7bd87d9d8729988073f78f6a37c6f3b8aa4db1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/174807
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Evan Shrubsole <eshr@google.com>
Cr-Commit-Position: refs/heads/master@{#31220}
This CL adds a queue for pending QualityScalerQpUsageHandlerCallbacks
and private methods for "Queueing", "Handling" and "Aborting" them,
using a sequence number as an ID to ensure we don't accidentally invoke
the same callback twice.
Because we don't have the adaptation task queue yet, callbacks are still
synchronously handled, which means the "pending callbacks" queue would
never have more than 1 element. However, when the adaptation task queue
is added and this is made asynchronous, it will be possible for multiple
callbacks to be pending simultaneously. This design is future-proof.
This CL is split out to aid reviewability. The CL that adds the
adaptation task queue will affect a lot of code. By landing this
separately, the adaptation queue CL will be easier to review.
This CL adds quality_scaler_resource_unittest.cc.
Bug: webrtc:11542, webrtc:11520
Change-Id: I00e7f6bfda9f8e8e82ec25916aa48e9349c8d70c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/174802
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@{#31219}
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}
