RED depacketizing now lives in mediatransportutil so the SFU and
replay export share one implementation. Drop the local copies; the
encoder round-trip tests now exercise the shared package.
* sfu: broadcast RTP to down tracks without a per-packet closure
Every forwarded packet allocated a closure capturing the packet and layer,
and an atomic write counter that escaped with it, even for tracks with a
single subscriber. Add BroadcastRTP, which carries the packet and layer in
the worker state and sums the writes per worker. The serial path no longer
allocates; the parallel path allocates the shared state and the worker
funcval only.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* sfu: reuse the forwarded packet copy in the RED receivers
Both RED receivers copied the ExtPacket (and the opus one the rtp.Packet)
per forwarded packet, and the copies moved to the heap because the
broadcast takes their address. Keep them on the receiver like the existing
redPayloadBuf: ForwardRTP runs on one goroutine and down tracks do not keep
the packet past WriteRTP.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* sfu: skip BroadcastRTP allocation assertion under the race detector
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* sfu: group BroadcastRTP and its interfaces after the spreader methods
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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.
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
This is not all of it as it is not possible (or at least I do not know
of a way) to get all suggestions for a repo/project. Did this via loop
searching mainly and taking the modernize suggestions.
* Add support for RTP stream restart.
When an unhandled packet is encountered, try a restart sequence.
Restart happens when 5 packets with contiguous sequence numbers and same
or increasing time stamps are received. Note that this does not work for
B-frame type of scenarios, but that is true for receive path handling
even before this. As WebRTC does not use B-frames, it is fine. But,
needs to be looked at again if B-frames are necessary.
It is controlled by a config that is disabled by default.
* clean up
* debug log
* Log write count atomic.
* Return write count from WriteRTP.
Apologies for the frequent changes on this. With relays, the down track
could write to several targets. So, use count to have an accurate
indication of how may subscribers were written to.
Packets not being forwarded were getting included in forwarding stats
calculation and skewing the measurement towards a smaller number.
The latency measurement does not include the batch IO of packets on
send. With a 2ms batching, that will add an average latency of 1ms.
With publish RED and subscribe Opus, the RTCP sender reports were not
sent to down track as publisher sender reports were not forwarded to the
down track.
* Normalize mime type and add utilities.
An attempt to normalize mime type and avoid string compares remembering
to do case insensitive search.
Not the best solution. Open to ideas. But, define our own mime types
(just in case Pion changes things and Pion also does not have red mime
type defined which should be easy to add though) and tried to use it everywhere.
But, as we get a bunch of callbacks and info from Pion, needed conversion in
more places than I anticipated. And also makes it necessary to carry
that cognitive load of what comes from Pion and needing to process it
properly.
* more locations
* test
* Paul feedback
* MimeType type
* more consolidation
* Remove unused
* test
* test
* mime type as int
* use string method
* Pass error details and timeouts. (#3402)
* go mod tidy (#3408)
* Rename CHANGELOG to CHANGELOG.md (#3391)
Enables markdown features in this otherwise already markdown'ish formatted document
* Update config.go to properly process bool env vars (#3382)
Fixes issue https://github.com/livekit/livekit/issues/3381
* fix(deps): update go deps (#3341)
Generated by renovateBot
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Use a Twirp server hook to send API call details to telemetry. (#3401)
* Use a Twirp server hook to send API call details to telemetry.
* mage generate and clean up
* Add project_id
* deps
* - Redact requests
- Do not store responses
- Extract top level fields room_name, room_id, participant_identity,
participant_id, track_id as appropriate
- Store status as int
* deps
* Update pkg/sfu/mime/mimetype.go
* Fix prefer codec test
* handle down track mime changes
---------
Co-authored-by: Denys Smirnov <dennwc@pm.me>
Co-authored-by: Philzen <Philzen@users.noreply.github.com>
Co-authored-by: Pablo Fuente Pérez <pablofuenteperez@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Paul Wells <paulwe@gmail.com>
Co-authored-by: cnderrauber <zengjie9004@gmail.com>
* Set FEC enabled properly in connection stats module.
With RED, the FEC indication is in primary codec.
Also, clean up some bits that were not necessary (TrackInfoAvailable is
not needed)
TODO: There are still a couple of things to figure out
- If codec is RED, Opus is added as second codec synthetically using
https://github.com/livekit/livekit/blob/33098337fc17705bbdb3283c7a7034aa6b2f3745/pkg/rtc/mediaengine.go#L31
which hard codecs FEC enabled. Ideally, we should get the primary
codec parameters from SDP offer.
- The WebRTCReceiver does not have information about primary codec. For
now, just setting FEC to true when RED is enabled. It is okay as it
just affects when we declare quality drops, but ideally the primary
codec should be retrieved from SDP offer.
* clean up and comment
* full prop check
* Handle cases of long mute/rollover of time stamp.
There are cases where the track is muted for long enough for timestamp
roll over to happen. There are no packets in that window (typically
there should be black frames (for video) or silence (for audio)). But,
maybe the pause based implementation of mute is causing this.
Anyhow, use time since last packet to gauge how much roll over should
have happened and use that to update time stamp. There will be really
edge cases where this could also fail (for e. g. packet time is affected
by propagation delay, so it could theoretically happen that mute/unmute
+ packet reception could happen exactly around that rollover point and
miscalculate, but should be rare).
As this happen per packet on receive side, changing time to `UnixNano()`
to make it more efficient to check this.
* spelling
* tests
* test util
* tests
When converting from RED -> Opus, if there is a loss, SFU recovers
that loss if it can using a subsequent redundant packet. That path
was not setting the extended sequence number properly.
Also, ensuring use of monotonic clock for first packet time adjustment
also.
* Perform unsubscribe in parallel to avoid blocking
When unsubscribing from tracks, we flush a blank frame in order to prepare
the transceivers for re-use. This process is blocking for ~200ms. If
the unsubscribes are performed serially, it would prevent other subscribe
operation from continuing.
This PR parallelizes that operation, and ensures subsequent subscribe
operations could reuse the existing transceivers.
* also perform in parallel when uptrack close
* fix a few log fields
When the publisher stops publishing, the individual receivers would close
attached DownTracks first before notifying MediaTrackReceiver callbacks.
This means #1454 does not fix the issue entirely since there is still a window
when we can subscribe to a closing track.
David Colburn