PeerConnection::Close() is, per-spec, a blocking operation.
Unfortunately, PeerConnection is implemented to own resources used by
the network thread, and Close() - on the signaling thread - destroys
these resources. As such, tasks run in parallel like getStats() get into
race conditions with Close() unless synchronized. The mechanism in-place
is RTCStatsCollector::WaitForPendingRequest(), it waits until the
network thread is done with the in-parallel stats request.
Prior to this CL, this was implemented by performing
rtc::Thread::ProcessMessages() in a loop until the network thread had
posted a task on the signaling thread to say that it was done which
would then get processed by ProcessMessages(). In WebRTC this works, and
the test is RTCStatsIntegrationTest.GetsStatsWhileClosingPeerConnection.
But because Chromium's thread wrapper does no support
ProcessMessages(), calling getStats() followed by close() in Chrome
resulted in waiting forever (https://crbug.com/850907).
In this CL, the process messages loop is removed. Instead, the shared
resources are guarded by an rtc::Event. WaitForPendingRequest() still
blocks the signaling thread, but only while shared resources are in use
by the network thread. After this CL, calling WaitForPendingRequest() no
longer has any unexpected side-effects since it no longer processes
other messages that might have been posted on the thread.
The resource ownership and threading model of WebRTC deserves to be
revisited, but this fixes a common Chromium crash without redesigning
PeerConnection, in a way that does not cause more blocking than what
the other PeerConnection methods are already doing.
Note: An alternative to using rtc::Event is to use resource locks and
to not perform the stats collection on the network thread if the
request was cancelled before the start of processing, but this has very
little benefit in terms of performance: once the network thread starts
collecting the stats, it would use the lock until collection is
completed, blocking the signaling thread trying to acquire that lock
anyway. This defeats the purpose and is a riskier change, since
cancelling partial collection in this inherently racy edge-case would
have observable differences from the returned stats, which may cause
more regressions.
Bug: chromium:850907
Change-Id: Idceeee0bddc0c9d5518b58a2b263abb2bbf47cff
Reviewed-on: https://webrtc-review.googlesource.com/c/121567
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26707}
(reverted in https://webrtc-review.googlesource.com/c/src/+/123182/1)
Original cl description:
Always offer transport sequence number header extension for audio
If the extension is negotiated, it will only be used if
the field trial WebRTC-Audio-SendSideBwe is enabled.
This allows simpler experimentation if it should be used or not.
Patchset 3 contain the only change:
Add the field trial WebRTC-Audio-SendSideBwe to call/rampup_tests.cc
TBR: srte@webrtc.org,ossu@webrtc.org
Bug: webrtc:10309 webrtc:10286
Change-Id: I2c1224e8a9fab52c1030369c1364686322e88a0f
Reviewed-on: https://webrtc-review.googlesource.com/c/123183
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26706}
Plus all the annotations that are necessary to make things compile
again.
Bug: webrtc:9987
Change-Id: I53caa3c6644c5d0fcb50f7c1e89e853eddd769a5
Reviewed-on: https://webrtc-review.googlesource.com/c/122882
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Commit-Queue: Karl Wiberg <kwiberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26705}
This function is used to post messages onto rtc::Threads. The thread
invokes the functor without blocking the calling thread. Messages posted
in this way are executed in the order that they were posted. This is
meant to work as the equivalent of "thread->PostTask()" in Chromium.
Note: AsyncInvoker currently does something similar but it is more
cumbersome to use (somebody has to create it and own it and make sure
not to destroy it while tasks are pending or else they're cancelled). It
also comes with a fundamental flaw: You cannot destroy the AsyncInvoker
from within the functor (this results in a neverending Wait). This makes
the AsyncInvoker not suitable for implementing "destructor traits"
amongst other things.
This CL will allow us to easily add "PostTask()" to rtc::Thread or add
support for DestructorTraits, which is especially useful when you have a
reference counted object that is referenced from multiple threads but
owns resources that has to be destroyed on a particular thread.
Blocking invokes are forbidden in Chromium but WebRTC performs them
frequently. Being able to perform the equivalent of PostTask() is a
good thing.
Bug: webrtc:10293
Change-Id: Ie2a612059a783f18ddf98cff6edb7fce447fb5be
Reviewed-on: https://webrtc-review.googlesource.com/c/121408
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26704}
This reverts commit fd965c008c7bc395bb276f260262ac11ccd25406.
Reason for revert: Cause test failure.
Original change's description:
> Always offer transport sequence number header extension for audio
>
> If the extension is negotiated, it will only be used if
> the field trial WebRTC-Audio-SendSideBwe is enabled.
> This allows simpler experimentation if it should be used or not.
>
> Bug: webrtc:10309 webrtc:10286
> Change-Id: I797e6f14c06d46189e40f6d09805c2e09afc015b
> Reviewed-on: https://webrtc-review.googlesource.com/c/122542
> Commit-Queue: Per Kjellander <perkj@webrtc.org>
> Reviewed-by: Erik Språng <sprang@webrtc.org>
> Reviewed-by: Sebastian Jansson <srte@webrtc.org>
> Reviewed-by: Oskar Sundbom <ossu@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#26689}
TBR=ossu@webrtc.org,sprang@webrtc.org,srte@webrtc.org,perkj@webrtc.org
Change-Id: I1b7d3fa5c282a5bf049ca54695ad16c8278a2698
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:10309 webrtc:10286
Reviewed-on: https://webrtc-review.googlesource.com/c/123182
Reviewed-by: Ying Wang <yinwa@webrtc.org>
Commit-Queue: Ying Wang <yinwa@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26703}
This change adds incoming & outgoing packet rates to the
event_log_visualizer.
The outgoing packet rate is drawn on the graph with outgoing RTP rate,
because we want to see it together with bandwidth estimate and probe
clusters.
The incoming packet rate is drawn separately.
Bug: webrtc:9719
Change-Id: I32648d016359af110837440ed1a5f9c31c841ea7
Reviewed-on: https://webrtc-review.googlesource.com/c/122941
Reviewed-by: Björn Terelius <terelius@webrtc.org>
Reviewed-by: Bjorn Mellem <mellem@webrtc.org>
Commit-Queue: Peter Slatala <psla@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26696}
This is a reland of ce470aab518f067a67aa03aaab1fc61a45afa0ec
Original change's description:
> Enabling Simulcast use via AddTransceiver.
>
> This change removes the restriction on multiple send encodings when
> calling AddTransceiver. If RIDs are not provided in the
> simulcast scenario, they are auto-generated by the platform.
>
> This effectively enables the use of spec-compliant simulcast.
> Tests are also added to verify simulcast functionality.
>
> Bug: webrtc:10075
> Change-Id: I088feba70a26e85abcc7bfbe3ea1fe5103cd47d2
> Reviewed-on: https://webrtc-review.googlesource.com/c/121389
> Reviewed-by: Florent Castelli <orphis@webrtc.org>
> Reviewed-by: Steve Anton <steveanton@webrtc.org>
> Commit-Queue: Amit Hilbuch <amithi@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#26590}
Bug: webrtc:10075
Change-Id: Ib4392e36d5d60b966ecff56d4df69bf60d13a73d
Reviewed-on: https://webrtc-review.googlesource.com/c/122962
Reviewed-by: Amit Hilbuch <amithi@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Commit-Queue: Amit Hilbuch <amithi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26694}
If the extension is negotiated, it will only be used if
the field trial WebRTC-Audio-SendSideBwe is enabled.
This allows simpler experimentation if it should be used or not.
Bug: webrtc:10309 webrtc:10286
Change-Id: I797e6f14c06d46189e40f6d09805c2e09afc015b
Reviewed-on: https://webrtc-review.googlesource.com/c/122542
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Oskar Sundbom <ossu@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26689}
This is not a functional change. I've verified that the event_log_visualizer outputs the same bytes before and after the CL.
Bug: webrtc:10102, webrtc:10312
Change-Id: I49c4c847926078cefc9b72fe57fbdaebf76423e9
Reviewed-on: https://webrtc-review.googlesource.com/c/122844
Reviewed-by: Mirta Dvornicic <mirtad@webrtc.org>
Commit-Queue: Björn Terelius <terelius@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26685}
Removing simulcast stream support as it was broken.
Bug: webrtc:9510
Change-Id: I42ba285bbea81e6ffd5b1d1a1aec4e5eb0990b1e
Reviewed-on: https://webrtc-review.googlesource.com/c/123040
Commit-Queue: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Björn Terelius <terelius@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26684}
There was a name collision with downstream test frameworks.
Bug: webrtc:9510
Change-Id: I7e37a8a54701ef4a47c687aec51f37523759f1b2
Reviewed-on: https://webrtc-review.googlesource.com/c/123044
Reviewed-by: Christoffer Rodbro <crodbro@webrtc.org>
Commit-Queue: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26683}
This will be used to investigate the effect of congestion window
pushback on bandwidth esimation. There is currently no data available
in event logs to analyze this in test runs.
Bug: None
Change-Id: I2397842e90fd4acab6306b03d1ee9daf62469ee3
Reviewed-on: https://webrtc-review.googlesource.com/c/121765
Reviewed-by: Konrad Hofbauer <hofbauer@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Commit-Queue: Evan Shrubsole <eshr@google.com>
Cr-Commit-Position: refs/heads/master@{#26681}
This change does not include receive_timestamps for ACKs, because there is 1 problem.
That problem will be resolved in a separate change.
I am getting receive_timestamp errors that have to do with delta compression with optional fields.
Two failure modes that I noticed:
1) the base event does not have the timestamp: it crashes with length validation
# Check failed: base <= MaxUnsignedValueOfBitWidth(params_.value_width_bits()) (1820716 vs. 131071)
2) all events are null, it crashes with assert that X events were expected, but no events were deserialized.
Bug: webrtc:9719
Change-Id: I5d1bbb95dfd15ca7321667aad5e4d89c085e9c06
Reviewed-on: https://webrtc-review.googlesource.com/c/122360
Commit-Queue: Peter Slatala <psla@webrtc.org>
Reviewed-by: Björn Terelius <terelius@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26668}
On NetEq level latency corresponds to delay and two terms can be used interchangeably here.
In order to implement latency constraint we need to provide a range of possible values which should be constant. See getCapabilities() here:
https://www.w3.org/TR/mediacapture-streams/#dfn-applyconstraints-algorithm
Lowest possible value accepted value is constant and equals 0. But because |packet_len_ms_| and |maximum_delay_ms_| may be updated during live of DelayManager upper bound is not constant. Moreover, due to change in |packet_len_ms_| the |minimum_delay_ms_| which was valid when its was set may be considered invalid later on.
To circumvent that and provide constant range for capabilities we separate base minimum delay and minimum delay. ApplyConstraints algorithm will set base minimum delay. Base minimum delay will act as recommendation for lower bound of minimum delay and will be used to limit target delay. If user sets base minimum delay through ApplyConstraints which is bigger than currently
possible maximum (e.g. bigger than NetEq maximum buffer size in milliseconds) then base minimum delay will be clamped to currently possible maximum to match user's intentions as best as possible.
Note, that we keep original behavior when minimum_delay_ms_ (effective_minimum_delay_ms after this CL) in LimitTargetLevel method may be above upper bound due to changing packet audio length.
Bug: webrtc:10287
Change-Id: I06b8f5cd3fd1bc36800af0447f91f7c4dc21a766
Reviewed-on: https://webrtc-review.googlesource.com/c/121700
Commit-Queue: Ruslan Burakov <kuddai@google.com>
Reviewed-by: Minyue Li <minyue@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26666}
Since it's a common pattern it makes sense to explicitly provide the
interface rather than reimplementing it every time it's used.
Bug: webrtc:9883
Change-Id: I4dca84bd7c8616fcbcbaba511718671a3668e743
Reviewed-on: https://webrtc-review.googlesource.com/c/122300
Commit-Queue: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Oskar Sundbom <ossu@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26664}
This works around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89305, which
causes GCC to fail to build the code due to |thread_checker_| being const
there and not having a declared constructor.
../../api/ice_transport_factory.cc: In constructor ‘webrtc::{anonymous}::IceTransportWithTransportChannel::IceTransportWithTransportChannel(std::unique_ptr<cricket::IceTransportInternal>)’:
../../api/ice_transport_factory.cc:31:3: error: uninitialized const member in ‘const class rtc::ThreadChecker’ [-fpermissive]
IceTransportWithTransportChannel(
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../api/ice_transport_factory.cc:45:28: note: ‘const rtc::ThreadChecker webrtc::{anonymous}::IceTransportWithTransportChannel::thread_checker_’ should be initialized
const rtc::ThreadChecker thread_checker_;
^~~~~~~~~~~~~~~
Bug: chromium:819294
Change-Id: I750e8cdd796b3b0e076de01194cf7de988ac4ce2
Reviewed-on: https://webrtc-review.googlesource.com/c/122820
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#26662}
In order to correctly close audio dump files, TestPeers have to be
destroyed after the call is finished.
Bug: webrtc:10138
Change-Id: I948e4e1844dfbffd1eef7926a4dd4d7631dbe632
Reviewed-on: https://webrtc-review.googlesource.com/c/122301
Reviewed-by: Artem Titov <titovartem@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26661}
This quality boost means that we sometimes drop a _lot_ of frames in the
base layer. It also interacts poorly with the bitrate adjuster since
even if frames are dropped they are often over-sized.
The setting still leaves the current behavior as default, but can be
changed using the WebRTC-VideoRateControl field trial.
Bug: webrtc:10155
Change-Id: I1a92ec69bab61b5148fe9d8bc391ac5ee1019367
Reviewed-on: https://webrtc-review.googlesource.com/c/122840
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26659}
