* Tests for down stream packet push.
A recent issue (padding bit in RTP header) surfaced a gap which slipped
through due to lack of tests. Changes in pion/rtp were not adopted
properly.
So, adding some tests (thank you Claude for the heavy lifting) to test
the down stream packet path using the whole pion chain.
Split out some interfaces so it is easier to have it all in one place
and create fakes.
Will help adding more tests, for example include the upstream path also
in the integration test. May have to create more interfaces and make
things testable, but this is a start.
* missed file
* rtx specific test
Addresses https://github.com/livekit/livekit/issues/4689
We were probably missing a couple of bits with this
1. Not affected for regular traffic like from browsers as it does not
add padding, but special clients were affected.
2. Probe packets were probably using wrong last byte as pion/rtp would
have overwrriten with 0 because the header.PaddingSize for the newer
versions were not set. That could have affected bandwidth estimation
catch up.
Following up on this PR (https://github.com/livekit/livekit/pull/4678 -
great catch btw, thank you), propagating the spreader threshold to RED
receivers also.
Removing the test as it was a simple case and that would not have caught
the original issue.
* fix: don't undercount data channel bitrate over idle gaps
BitrateCalculator divided drained bytes by wall-clock time, including
idle gaps between sparse writes. A fast channel fed sparsely (e.g. 1 KB
every second, acked in a few ms) therefore reported its low offered load
(~8 kbps) instead of its drain capacity (~800 kbps). In the unreliable
writer that shrinks targetLatencyLimit and drops bursts on a channel
that is actually keeping up.
Measure the rate over backlogged ("busy") time only, so idle time no
longer dilutes the estimate. Backlog is detected from the buffer
occupancy just before a write (bufferedAmount - bytesWritten): since no
bytes are added between writes the buffer only drains, so a non-zero
pre-write level means it never emptied and the interval was genuinely
busy. Gating on the post-write buffered amount would be wrong -- it
always includes the just-enqueued (not yet SACKed) bytes and is ~never
zero, collapsing the estimate back to wall-clock. When no backlog is
ever observed the calculator reports no estimate (ok=false) instead of a
confidently-low number.
Introduce BitrateMode:
- BusyOnly (writer default): busy denominator, all drained bytes counted.
- ExcludeIdleDrain: also drops bytes drained across non-backlogged
intervals for a clean drain-capacity estimate free of idle
contamination.
- WallClock: elapsed-time denominator, all bytes; for the test-client
reader which has no send buffer to observe.
Call sites keep the conservative behaviour (writers BusyOnly, reader
WallClock). Tests exercise both writer modes and the mixed idle-then-
backlog window where they diverge.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Trim comments in data channel bitrate calculator
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Make BitrateMode.String a method on the enum
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Some data track related tests are failing due to setting a target
latency of 100ms. Would be good to understand if taget latency or min
buffer is causing the drops.
* fix: bound data-track buffering under downlink congestion
The SFU data-track down-channel writer was constructed with targetLatency=0,
which disables the buffered-amount/latency-based drop in writeUnreliable. Under
subscriber downlink congestion, frames were queued unbounded and in order in the
per-subscriber SCTP send buffer instead of being dropped, so end-to-end latency
grew without limit and only drained once congestion cleared. This contradicts
data tracks being a low-latency, lossy transport.
Wire a dedicated `datachannel_data_track_target_latency` config through to both
data-track writer call sites (mirroring how `datachannel_lossy_target_latency`
controls the lossy publishData channel), reusing lossyDataChannelMinBufferedAmount
as the drop floor. Defaults to 100ms so data tracks are latency-bounded out of
the box; set to 0 to restore the previous unbounded behavior.
Fixes#4666
* fix: track drained bytes on successful unreliable data channel writes
writeUnreliable only called BitrateCalculator.AddBytes when a write failed,
so on the common success path the calculator never received samples. As a
result Bitrate() had no data and the latency-based drop threshold collapsed to
the static minBufferedAmount floor, making the per-subscriber latency control
(datachannel_lossy_target_latency and the new datachannel_data_track_target_latency)
effectively inert. Call AddBytes after every write, matching writeReliable.
Use by cloud simulated tracks.
Introduces a lock, but it is used only in the background worker (where
it will be almost always uncontested) and when API GetStats is accessed.
Does not touch the per-packet Update path.
* Move ForwardStats aggregation off the packet forwarding path
ForwardStats is a process-wide singleton and its Update runs for every
forwarded packet. It previously took a shared mutex, updated a windowed
aggregate, and observed into a global Prometheus histogram on every call.
Update now only buffers the transit sample into a sharded lock-free ring
(one atomic add to reserve a slot, one atomic store to publish). A
background worker drains the ring every summary interval, observes each
sample into the histogram (per-packet fidelity retained) and folds the
interval summary into a window ring for the latency/jitter gauges. Under
sustained overload the oldest excess samples are dropped and counted, and
the count is logged.
Ring slots are atomic.Int64 so producer store / consumer load are
synchronized (race-clean). Capacity is numShards*shardCap = 131072
samples; at the default 50ms summary interval that sustains ~2.6M
samples/s before dropping.
Benchmark: BenchmarkForwardStatsUpdate (0 allocs/op both), Update per call:
cores before after speedup
1 ~8.6 ns ~1.5 ns ~6x
8 ~175 ns ~22 ns ~8x
The before path (mutex + windowed Welford aggregate + per-packet histogram
observe) degrades ~20x from 1 to 8 cores under contention; the after path
does not block and stays an order of magnitude lower under load.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Fix ring publish race and report-path double-flush
Addresses two review findings on the forward-stats sample buffer.
1. Publish race (reserve-before-store): push reserved a ring slot by
advancing writeIdx and stored the value afterwards, so drain could read a
slot the producer had reserved but not yet written, getting a stale/zero
value; the producer's later store then landed behind the read cursor and
was lost. Each slot now carries a publish epoch. push stores the value and
then publishes seq = index+1. drain reads a slot only when seq == r+1; a
reserved-but-unpublished slot at the cursor stops the drain and is picked
up on the next call, so no sample is read stale or lost. A slot overwritten
during the read is detected on re-check and counted as dropped.
The windowed latency/jitter stats did not permanently drift even before
this change (report recomputes from a fixed-size ring that overwrites, not
discounts, old buckets, so any error aged out within the report window),
but these values feed the capacity manager, so the buffer is made exact.
2. Report-path double-flush: run() flushed on both the summary tick and the
report tick, and every flush advances the window ring, so the ring cycled
faster than intended and the effective window was shorter than configured
(~5% at 50ms/1s/1m). The report tick no longer flushes; the summary ticker
keeps the ring current to within one summary interval.
Benchmark, Update per call (0 allocs/op), 1 and 8 cores:
after fixes: ~5.5 ns / ~29 ns
before fixes (this branch): ~1.5 ns / ~22 ns
baseline (mutex + per-packet histogram): ~8.6 ns / ~175 ns
The publish epoch adds one atomic store to push; the path stays non-blocking,
zero-allocation, and well below the baseline under contention.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Async attributes on participant.
How it is different from existing participant attributes?
1. Async attribute can be added one at a time.
2. These are not included in `ParticipantInfo`.
3. Get an attribute bt participant identity and async attribute ID as
and when needed.
* clean up
* get full definitions, not just ids
* listener OnDataTrackSchema
* name length config
* data blob
* deps
* static check
* Add missing request ID
* Update protocol commit
* Wire up StoreDataBlobResponse
* Pass request ID through in GetDataBlobResponse
* deps
* atomic
* sctp at 1.9.5
* remove proto clone
---------
Co-authored-by: Jacob Gelman <3182119+ladvoc@users.noreply.github.com>
* feat: acquire requested video layer directly at HIGH quality by default
Two changes that together remove the visible low->high quality ramp for a new
subscriber (both publisher-first and subscriber-first join orders):
1. Default a subscriber's initial video quality to HIGH on bind instead of LOW
for adaptive stream, so the subscribed max layer is the top layer. Adaptive
stream clients can still scale down afterwards based on viewport.
2. On initial layer acquisition the forwarder/selector latch directly onto the
allocator's target (the requested top layer) instead of opportunistically
latching onto the first lower key frame that arrives. A short
initial-acquisition grace aims the target at the requested layer; if it does
not show up in time, the target falls back to the highest layer seen so
acquisition never stalls.
Always on - no configuration flag.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat: gate start-at-desired-quality behind EnableStartAtDesiredQuality flag
Put the "acquire requested video layer directly at HIGH quality" behavior
behind a per-subscriber EnableStartAtDesiredQuality flag (default off, so
the original low->high ramp-up is restored unless enabled).
Plumbed from config.RTC.EnableStartAtDesiredQuality through ParticipantParams
-> SubscribedTrack/DownTrack -> Forwarder -> simulcast selector, gating all
three behavior changes: the HIGH default on bind, the forwarder's
initial-acquisition grace, and the selector's direct-latch-onto-target.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* remove config.
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Fix skipped packets accounting.
No need to copy unskipped packet RTP header to skipped packet.
That was causing padding bytes to be counted.
Also use Header.PaddingSize as base PaddingSize is deprecated.
* PaddingSize in header in utils
Reverting
- https://github.com/livekit/livekit/pull/4521
- https://github.com/livekit/livekit/pull/4525
There are TWCC feedback packets that are larger than MTU. Seems to
happen under a couple of conditions
1. Bad client data, i. e. severely out-of-order packets, bad sequence
numbers, etc.
2. On an ICE restart - this is rare, but it seemed to be flaky network
with some packets arriving and some not and causing a lot of gaps.
Either case, not much to do. If fargmentation/re-assembly back to
publisher works, the feedback will make it through. If not, feedbacks
will be missed and clients have to work with some missing data which is
not unexpected and the protocol is designed to handle.
However, filed pion/interceptor issue just in case - https://github.com/pion/interceptor/issues/416
Not a major issue, but just avoiding duplicate creation of NACK module.
RTCP feedback of `nack` and `nack pli` end up getting treated as `nack`
and was double creating.
* Log large packets receive/send.
Seeing cases of servers reporting need for segmentation/re-assembly of
packets. So, logging packet receive/send for RTP/RTCP to check if
anything is seeing more than 1400 byte packets.
* log downtrack RTCP too
* test: verify upstream and downstream connection stats
Adds TestConnectionStats integration test where two clients connect,
each publishes audio + video, and the test asserts that both
publisher-side (LocalMediaTrack.GetTrackStats) and subscriber-side
(DownTrack.GetTrackStats) report non-zero packets and bytes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* sfu: add DownTrack.OnStatsUpdate hook and use it in stats test
Adds a public OnStatsUpdate setter on DownTrack mirroring the existing
pattern on WebRTCReceiver. The new callback fires alongside the
configured DownTrackListener (production path is unaffected) and is
intended for tests/observers to validate the AnalyticsStat data flowing
through the listener.
Augments TestConnectionStats to:
- hook WebRTCReceiver.OnStatsUpdate for each published track and assert
the captured AnalyticsStat has non-zero packets/bytes (upstream).
- hook the new DownTrack.OnStatsUpdate for each subscribed track and
make the same assertion (downstream).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Media loss proxy is not use, so it is okay, but was an unintentional
delete in https://github.com/livekit/livekit/pull/3252/changes
Was checking code due to a report of how Chrome does RTCP reports in
147+ breaking a few services. Don't think that affects LK, but found
this while reading code.
* Legacy TrackInfo.Simulcast flag.
When AddTrack did not send SimulcastCodecs, the legacy `Simulcast` flag
was not set. Fix it by setting the flag when a second layer is
published.
* staticcheck
* use the existing PrimaryReceiver function
- prevent some escape to heap
- avoid copying by using a ring buffer for receiver reports (probably
should remove this as this is for debugging only and data so far has
shown clients sending bad data and nothing more.)
* Use Muted in TrackInfo to propagated published track muted.
When the track is muted as a receiver is created, the receiver
potentially was not getting the muted property. That would result in
quality scorer expecting packets.
Use TrackInfo consistently for mute and apply the mute on start up of a
receiver.
* update mute of subscriptions
* Update go deps
Generated by renovateBot
* update api usage
---------
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: David Zhao <dz@livekit.io>
For a participant migrating out, the track could be resumed on a
different node, but ending on the migrating out node. So, `flush` should
be used to indicate if track is going to be resumed.
Removing some logs which have not been useful in terms of insights other
than saying that there are a bunch of packets missing. Going to start
looking at gaps in terms of time if the inter-packet gap is too high.
Also, using logging these events as first 20 and then every 200.
A bunch of edges to note here
RED packet does not have sequence number for redundant blocks. It only
has timestamp offset compared to the primary payload. The receivers are
supposed to use just timestamp to sequence the payload and decode.
But, when converting from RED -> Opus, the packets extracted from RED
packet should be assigned a sequence number before they can be
forwarded. The simple rule is, if packet N contains X redundant
payloads, they are assigned sequence number of N - X to N - 1.
However there are cases like the following sequence (with 1 packet
redundancy)
- Seq num 10, timestamp 2000, forwarded
- Seq num 11 is lost
- Seq num 12 has a redundant payload. Seq num 12 has timestamp of 4000.
Ideally would expect the redundant payload to have a timestamp offset
of 1000, so the redundant payload can be mapped to sequence number 11
and timestamp 3000 (4000 - 1000). But, in the problematic case, it has
an offset of 3000 resulting in sequence number 11 and timestamp of
1000 causing an inversion with packet at sequence number 10.
Unclear if this a publisher issue, i. e. packing RED wrong or if this is
some expected behaviour with DTX. i. e. the DTX packets are not included
in redundant payload. For example, the sequence
- Seq num 10 -> DTX
- Seq num 11 -> DTX -> lost
- Seq num 12 -> Regular packet and include sequence num 9 as that is the
last regular packet.
Anyhow, detect this condition and drop the time inverted packet.
Note however this handles only inversion against the highest sent packet
sequence number and timestamp. So, some old packet inverted with some
other old packet getting forwarded will get through. That has been the
case always though and detecting that would be expensive and
complicated.
At least for egress, will also look at adding a check for inversion so
that it can catch it before sending it down the gstreamer pipeline. As
the egress uses a jitter buffer with ordered sequence number emits, it
will be simpler to detect timestamp going back when sequence number is
moving forward (of course the mute/dtx challenege is there).
* Log time inversion between incoming packets
Log of timestamp inversion within a red packet did not show anything.
Log across packets. Not dropping till there is more evidence of the
cause.
* save
* comment
* Guard against timestamp inversion in RED -> Opus conversion.
Seeing timestamp inversion (sequence number is +1, but timestamp is
-960, i.e. 20ms) in the RED -> Opus conversion path. Not able to spot
any bugs in code. So, logging details upon detection and also dropping
the packet. If not dropped, downstream components like Egress treat it
as a big timestamp jump (because sequence number is moving forward) and
try to adjust pts which ends up causing drops.
* do not log time reversal at the start
* typo
