sync.Pool drops a quarter of returned items when built with -race, so the
pooled send path averages about one allocation per packet in CI and the
zero-allocation assertion flips between passing and failing. Keep the
extension checks and skip only the allocation count under race.
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
SendPacket allocated four times per down track write: a 3 byte slice for
abs-send-time, a 2 byte slice for transport-cc, and two appends to a nil
Extensions slice because the pooled header was reset to a zero value.
Give pacer.Packet fixed scratch arrays for the two extensions and marshal
into them. The Packet is pooled and owned by one send until SendPacket
returns, so nothing is shared across down tracks. Keep the pooled header's
Extensions capacity across reuse, both when returning it to the pool and
when a down track initializes it.
Adds a test that asserts zero allocations per SendPacket.
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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
* 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
* Use sync.Pool for objects in packet path.
Seeing cases of forwarding latency spikes that aling with GC.
This might be a bit overkill, but using sync.Pool for small +
short-lived objects in packet path.
Before this, all these were increasing in alloc_space heap profile
samples over time. With these, there is no increase (actually the lines
corresponding to geting from pool does not even show up in heap
accounting when doing `list` in `pprof`)
* merge
* Paul feedback
Seeing a lot of queuing delay based back offs. Trying a couple of things
1. Accept a bit more queuing.
2. An option to try a different pacer. Would like to try with pass
through. That will produce some out-of-order packets. Remains to be
seen if it will have a negative impact.
* WIP
* no worker
* fixes
* use congested packet groups
* oldest group
* markers
* WIP
* WIP
* WIP
* WIP
* WIP
* clean up
* fmt
* consolidate
* store last packet only for bwe extension cases
Cosmetic. While thinking through how to structure probing better,
noticing small things here and there. Cleaning up and making some small
PRs along the way.
* Fix header size calculation in stats.
With pacer inserting some extensions, the header size used in stats
(and more impoetantly when probing for bandwidth estimation and
metering the bytes to control the probes) was incorrect. The size
was effectively was that of incoming extensions. It would have been
close enough though.
Anyhow, a bit of history
- initially was planning on packaging all the necessary fields into
pacer packet and pacer would callback after sending, but that was not
great for a couple of reasons
- had to send in a bunch of useless data (as far as pacer is
concerned) into pacer.
- callback every packet (this is not bad, just a function call which
happens in the foward path too, but had to lug around the above
data).
- in the forward path, there is a very edge case issue when calling stats update
after pacer.Enqueue() - details in https://github.com/livekit/livekit/pull/2085,
but that is a rare case.
Because of those reasons, the update was placed in the forward path
before enqueue, but did not notice the header size issue till now.
As a compromise, `pacer.Enqueue` returns the headerSize and payloadSize.
It uses a dummy header to calculate size. Real extension will be added
just before sending packet on the wire. pion/rtp replaces extension if
one is already present. So, the dummy would be replaced by the real one
before sending on the wire.
https://github.com/pion/rtp/blob/a21194ecfb5362261a0dc4af1f68e4a8944df345/packet.go#L398
This does introduce back the second rare edge case, but that is very
rare and even if it happens, not catastrophic.
* cleanup
* add extensions and dummy as well in downtrack to make pacer cleaner
* file output
* wake under lock
* keep track of RTX bytes separately
* packet group
* Packet group of 50ms
* Minor refactoring
* rate calculator
* send bit rate
* WIP
* comment
* reduce packet infos size
* extended twcc seq num
* fix packet info
* WIP
* queuing delay
* refactor
* config
* callbacks
* fixes
* clean up
* remove debug file, fix rate calculation
* fmt
* fix probes
* format
* notes
* check loss
* tweak detection settings
* 24-bit wrap
* clean up a bit
* limit symbol list to number of packets
* fmt
* clean up
* lost
* fixes
* fmt
* rename
* fixes
* fmt
* use min/max
* hold on early warning of congestion
* make note about need for all optimal allocation on hold release
* estimate trend in congested state
* tweaks
* quantized
* fmt
* TrendDetector generics
* CTR trend
* tweaks
* config
* config
* comments
* clean up
* consistent naming
* pariticpant level setting
* log usage mode
* feedback
* Split RTPStats into receiver and sender.
For receiver, short types are input and need to calculate extended type.
For sender (subscriber), it can operate only in extended type.
This makes the subscriber side a little simpler and should make it more
efficient as it can do simple comparisons in extended type space.
There was also an issue with subscriber using shorter type and
calculating extended type. When subscriber starts after the publisher
has already rolled over in sequence number OR timestamp, when
subsequent publisher side sender reports are used to adjust subscriber
time stamps, they were out of whack. Using extended type on subscriber
does not face that.
* fix test
* extended types from sequencer
* log
* Pacer interface to send packets
* notify outside lock
* use select
* use pass through pacer
* add error to OnSent
* Remove log which could get noisy
* Starting TWCC work (#1727)
* add packet time
* WIP commit
* WIP commit
* WIP commit
* minor comments
* Some measurements (#1736)
* WIP commit
* some notes
* WIP commit
* variable name change and do not post to closed channel
* unlock
* clean up
* comment
* Hooking up some more bits for TWCC (#1752)
* wake under lock
* Pacer in down stream path.
Splitting out only the pacer from a feature branch to
introduce the concept of pacer.
Currently, there should be no difference in functionality
as a pass through pacer is used.
Another implementation exists which is just put it in a queue and send
it from one goroutine.
A potential implementation to try would be data paced by bandwidth
estimate. That could include priority queues and such.
But, the main goal here is to introduce notion of pacer in the down
stream path and prepare for more congestion control possibilities down
the line.
* Don't need peak detector
* remove throttling of write IO errors
Raja Subramanian