If the real-time clock makes a sudden and large shift forward while
there is at least one packet in the queue, the pacer would previously
try to "replay" the behavior it would have done while it was asleep.
This can lead to grand bursts of padding packets, which is
undesireable.
This CL mitigates this problem by forwarding the internal state clock
to (now - 50ms) if there is nothing to do but generate padding.
Bug: webrtc:11340, b/258509536
Change-Id: I5b8a130d938dd2566f72c1946c139f50e099e5a9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/285380
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38752}
Created probes are currently timed out after 5s. But to be safe, also limit the number of pending probes to 5.
Bug: webrtc:14392, b/259541308
Change-Id: Ibf630704ffe14cb165ab849b881cf75857376f82
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/284080
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38697}
We have seen a few instances in a down-stream project where the circuit
breaker is still triggering and causing issues.
This CL makes the threshold configurable and adds more debug logging to
try and get to the bottom of this rarely occuring bug.
Bug: webrtc:11340, b/258509536
Change-Id: I92674d446b926ad66538ff9c8be2a32a3d95b057
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/283762
Reviewed-by: Emil Lundmark <lndmrk@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38664}
BurstyPacer is currently controlled via field trials. In order for
Chrome to be able to have burst without relying on a field trial, this
parameter is added.
When all burst experiments have concluded we may be able to have a
hardcoded constant instead, but for now the parameter is added to
RTCConfiguration.
NOTRY=True
Bug: chromium:1354491
Change-Id: I386c1651dbbcbf309c15ea3d3380cf8f632b5429
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/283420
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38621}
This metric was always supposed to be the spec's answer to
googBucketDelay, and is defined as "The total number of seconds that
packets have spent buffered locally before being transmitted onto the
network." But our implementation measured the time between capture and
send, including encode time. This is incorrect and yields a much larger
value than expected.
This CL updated the metric to do what the spec says. Implementation-wise
we measure the time between pushing and popping each packet from the
queue (in modules/pacing/prioritized_packet_queue.cc).
The spec says to increment the delay counter at the same time as we
increment the packet counter in order for the app to be able to do
"delta totalPacketSendDelay / delta packetSent". For this reason,
`total_packet_delay` is added to RtpPacketCounter. (Previously, the
two counters were incremented on different threads and observers.)
Running Google Meet on a good network, I could observe a 2-3 ms average
send delay per packet with this implementation compared to 20-30 ms
with the old implementation. See b/137014977#comment170 for comparison
with googBucketDelay which is a little bit different by design -
totalPacketSendDelay is clearly better than googBucketDelay.
Since none of this depend on the media kind, we can wire up this metric
for audio as well in a follow-up:
https://webrtc-review.googlesource.com/c/src/+/280523
Bug: webrtc:14593
Change-Id: If8fcd82fee74030d0923ee5df2c2aea2264600d4
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/280443
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38480}
The pacer can thus run on the Worker thread or an owned TQ depending on field trial string "WebRTC-SendPacketsOnWorkerThread"
Bug: webrtc:14502
Change-Id: Ic74b92b21371cc62c7b2f62f039bc800dcceef8c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/277622
Reviewed-by: Evan Shrubsole <eshr@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38301}
The value is today set to 200 which is too low for an audio packet to trigger sending probes.
For the initial probing, it would be good if audio packets, that may arrive before the first video frame can trigger sending a probe.
Also fix field trial parsing of required number of probes.
Bug: webrc:14392
Change-Id: I1f3cebcda38b71446e3602eef9cfa76de61a1ccf
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/275620
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Reviewed-by: Diep Bui <diepbp@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38089}
This CL adds the ability to forward aborted retransmission notifications
to specified RTP modules, as well as a way to find the RTX ssrc
associated with a media SSRC.
These will both be used by upcoming logic that can selectively flush
given streams from the pacer queue.
Bug: webrtc:11340
Change-Id: Ief3be47e4fd7dc5a1499bc21890e8979400ecb44
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/274706
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38050}
BlockingCall doesn't take rtc::Location parameter and thus most of the dependencies on location can be removed
Bug: webrtc:11318
Change-Id: I91a17e342dd9a9e3e2c8f7fbe267474c98a8d0e5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/274620
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38045}
This adds a (default off) flag which makes retransmissions be processed
immediately, just like audio packets normally are.
This might increase send rates and thus losses in some cases, but will
also reduce retranmission delays especially when timer slack or bursting
is used. Usefuleness TBD via experiment.
Bug: chromium:1354491
Change-Id: Icaa83125bfb30826ce72e6e786963d411e05ea57
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/272483
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37926}
And add a unit test that verifies that next probe time is set at is
the expected if the recommended probe size is used.
Bug: webrtc:14392
Change-Id: I239bb3a1c8eefc85509aacc82037c64e3ce49ed7
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/272648
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37901}
This experiment will tell if we still see the performance gains that we
saw with the "bursty slacked pacer" even if we don't apply slack (since
the "slack without burst" showed little impact at Stable).
The hope is that without slack all quality regressions will go away but
that bursting will still provide the desired performance benefits.
Bug: chromium:1354491
Change-Id: I95f05d040713addaaa1856c8e374a01c27311612
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/272366
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37845}
A recent cleanup cl (r36900) had an unintended side-effect.
If the queue-time limit is expected to be hit, we adjust the pacing
bitrate up to make sure all packets are sent within the nominal time
frame.
However after that change we stopped adjusting the pacing rate back to
normal levels when queue clears - at least not until the next BWE
update (which is fairly often - but not immediate).
This CL fixes that, and also makes sure whe properly update the
adjusted media rate on enqueu, dequeue and set rate calls.
Bug: webrtc:10809
Change-Id: If00dc35169f1a1347fea6eb44fdb2868282ed3b7
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/265387
Reviewed-by: Per Kjellander <perkj@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37178}
This CL introduces PacketQueue::SizeInPacketsPerRtpPacketMediaType
keeping track of the number of packets in the queue per
RtpPacketMediaType.
The TaskQueuePacedSender is updated not to apply slack if the queue
contains any kRetransmission or kAudio packets. The hope is that not
slacking retransmissions will make the NACK/retransmission regression
of the SlackedPacer experiment go away. Wanting to not slack audio
packets is unrelated to the regression but a sensible thing to due
since audio is highest priority.
This CL does not change anything when the SlackedPacer experiment is
not running, since if its not running then none of the packets are
slacked.
Bug: webrtc:14161
Change-Id: I1e588599b6b64ebfd7d890706b6afd0b84fd746d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/265160
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37139}
When the slacked pacer experiment is enabled the next pacing opportunity
may be a full tick (~16 ms) from now. Add a flag to allow experimenting
with a burst interval (= 16 ms?) such that we can send bursts in
MaybeProcessPackets.
A common use case would be that EnqueuePackets triggers
MaybeProcessPackets when we are off-tick but we'd still like to create
an immediate burst instead of waiting for the next tick or two for that
to happen.
Bug: webrtc:14152
Change-Id: Ib0ed8312cb7d53b80f3520fff3a6e3bbb5a93fd1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264985
Reviewed-by: Per Kjellander <perkj@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37116}
Allows the PacerController to send packets in bursts. If there are enqued packets, or a packet is enqueued while the pacer have a small media debt, an enqued packet is allowed to be sent immediately as long as the debt is smaller than the set burst interval.
Bug: b/233850913
Change-Id: Ibb0fa63c97409ca23b9fa7148b5ff6ce8c4517e2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264462
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37098}
Ensure each test create its own PacerController.
Move (most) operations on the pacer controller to the actual test. (the
rest should be moved too eventually....)
Use only one test fixture.
Bug: none
Change-Id: I0b8eee9d2c2f91f7102858a1a544e45e8b0b7b5d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264120
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37017}
Structs with user-declared constructors are not aggregates and cannot
be initialized with designated initializers. Remove declarations that
don't actually affect anything.
Bug: chromium:1284275
Change-Id: Ib45ea334d7be28bfa7bbce132985612f0e6ecd10
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/262820
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Commit-Queue: Philip Eliasson <philipel@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36916}
Instead of using field trials in BitrateProber for probe duration, use values provided in ProbeClusterConfig from GoogCC.
Field trials are instead read in ProbeController.
To avoid having to do a thread jump for every ProbeClusterConfig, RtpPacketPacer interface is changed to RtpPacketPacer::CreateProbeClusters(std::vector<ProbeClusterConfig>
Deprecates field trial "WebRTC-Bwe-ProbingConfiguration"
Change-Id: I3991e4b54770601855a3af2d6a16678f11d41c31
Bug: webrtc:14027
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261265
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36911}
The new TaskQueuePacedSender has been default-on in code since M97, and
there are no further usages of it that I can find. Let's clean this up!
The PacingController and associated tests will be cleaned up in a
follow-up cl.
Bug: webrtc:10809
Change-Id: I0cb888602939add953415977ee79ff0b3878fea5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/258025
Reviewed-by: Per Kjellander <perkj@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36890}
This queue is a more strict round robing queue, unlike the class
named RoundRobinPacketQueue. That is, we don't have the same logic to
prioritize lower-bitrate streams.
The queue time mechanism is essentially directly copied from the
previous implementation however.
Bug: webrtc:11340
Change-Id: Ie38ba8ce27c985f5f1e907cec068d6a365089bcc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260562
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36737}
This will make it easier to extend testing, implement new features (e.g.
packet culling) and experiment with new variants.
Bug: webrtc:11340
Change-Id: I747f5f6cff61e11a420e43b06ffe0c4aba438c7b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260116
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36670}
This avoids potentially creating structures in the queue, and removes
usage of a special priority class enabling us to move priority handling
in a follow-up.
Bug: webrtc:11340
Change-Id: I2286ef6eac62e1d6dd89f6eb6035b23f27543d8a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259960
Reviewed-by: Emil Lundmark <lndmrk@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36641}
This reverts commit fc5871cb98481937351bee2f46b2a2e3987d177a.
Reason for revert: I rage quit.
Original change's description:
> Reland "Send first probe packet directly instead of enqueuing it."
>
> This is a reland of commit 8088aad5ac0154d8fdc252de9e8ab4e0172d7da2
>
> Patchset 1 is original CL, patchset 2 contains a fix to make behavior
> exactly identical to original. It fixes a difference when the first
> small probe packet comprises the entire probe.
>
> Original change's description:
> > Send first probe packet directly instead of enqueuing it.
> >
> > This avoids potentially creating needless containers in the packet
> > queue and removes usage of the packet prio, allowing it to be moved in
> > an upcoming CL.
> >
> > Bug: webrtc:11340
> > Change-Id: Iddd9e7e4e73c97ab25a85e42bcc0094d61fd60d3
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259524
> > Reviewed-by: Emil Lundmark <lndmrk@webrtc.org>
> > Commit-Queue: Erik Språng <sprang@webrtc.org>
> > Cr-Commit-Position: refs/heads/main@{#36602}
>
> Bug: webrtc:11340
> Change-Id: I7d2f476a7043edd03f44a4a3de31f13c3c946a8d
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259776
> Reviewed-by: Emil Lundmark <lndmrk@webrtc.org>
> Commit-Queue: Erik Språng <sprang@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#36626}
Bug: webrtc:11340
Change-Id: Ife22cff8f5fbf9b70dc24926b84caad82d2738b8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259778
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36628}
This is a reland of commit 8088aad5ac0154d8fdc252de9e8ab4e0172d7da2
Patchset 1 is original CL, patchset 2 contains a fix to make behavior
exactly identical to original. It fixes a difference when the first
small probe packet comprises the entire probe.
Original change's description:
> Send first probe packet directly instead of enqueuing it.
>
> This avoids potentially creating needless containers in the packet
> queue and removes usage of the packet prio, allowing it to be moved in
> an upcoming CL.
>
> Bug: webrtc:11340
> Change-Id: Iddd9e7e4e73c97ab25a85e42bcc0094d61fd60d3
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259524
> Reviewed-by: Emil Lundmark <lndmrk@webrtc.org>
> Commit-Queue: Erik Språng <sprang@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#36602}
Bug: webrtc:11340
Change-Id: I7d2f476a7043edd03f44a4a3de31f13c3c946a8d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259776
Reviewed-by: Emil Lundmark <lndmrk@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36626}
This is a reland of commit 8088aad5ac0154d8fdc252de9e8ab4e0172d7da2
Original change's description:
> Send first probe packet directly instead of enqueuing it.
>
> This avoids potentially creating needless containers in the packet
> queue and removes usage of the packet prio, allowing it to be moved in
> an upcoming CL.
>
> Bug: webrtc:11340
> Change-Id: Iddd9e7e4e73c97ab25a85e42bcc0094d61fd60d3
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259524
> Reviewed-by: Emil Lundmark <lndmrk@webrtc.org>
> Commit-Queue: Erik Språng <sprang@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#36602}
Bug: webrtc:11340
Change-Id: I3e0db452961f7f5a8d00ea3283e92ec43da2f66a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259774
Reviewed-by: Emil Lundmark <lndmrk@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36619}
