sync engine#890
Conversation
boks1971
left a comment
boks1971
left a comment
There was a problem hiding this comment.
not able to read it all, looks good from what I could read
|
started looking into this today - I need to set aside couple of hours tomorrow to review it in depth. |
|
The old sync drops packets older than oldPacketThreshold. The new sync has no equivalent. Very old packets coming in a burst could fill in gst queues and slow down burst recovery. Is there any reason not to have it inside the new sync? |
|
Similar to the note above - new sync doesn't integrate pipeline time feedback (existing |
|
Synchronization related bugs were the hardest class of bugs I have seen for egress in the previous period. I think we need to have good logging around this also in the new logic. Adjustments performed, stats etc. The existing syncrhonizer code has decent amount of logs which came to be through the pain of debugging sync issues. |
|
Maybe it's worth calling out that some of the options being in the current sync logic aren't supported - like disabling PTS adjustments - I guess we can use existing synchronizer for cases where this is needed |
|
Pretty neat idea of using both NtpEsitmator and OWD estimator to map everything through session timeline - I like that 👍 |
My hope was that if we just ripped all the logic out of chrome, we wouldn't need those options. Doing this in parallel with the sdk compositing so we have a good way to test/measure other than just deploy to canary and hope |
👍 definitively nice for side by side comparison. I am afraid though we might need it, but let's see 🤞 . A lot of straightening for chrome is happening on DSP level, inside libwebrtc with NetEq which is what e.g fixes audio timeline with changing tempo and not touching PTS (more complex and probably robust logic than what I built with pitch audio tempo control.). |
|
Integration tests all passing livekit/egress#1198 |
|
Sorry @frostbyte73 . Backlogged on this. Will try to read through this today. |
boks1971
left a comment
boks1971
left a comment
There was a problem hiding this comment.
Good stuff @frostbyte73 ! Left some comments.
I am not able to check on every bit as there are a lot of scars from older battles. I think @milos-lk caught some of those and you have included it all. Guess, this will require good chunk of testing + good amount of soak in staging.
milos-lk
left a comment
milos-lk
left a comment
There was a problem hiding this comment.
We might need to update drift value for the case when we don't touch audio PTS as we discussed yesterday - but other than that 👍
3 participants
New Synchronizer based on Chrome's
RtpToNtpEstimator,StreamSynchronization, andRtpStreamsSynchronizerNtpEstimator: Per-track linear regression (OLS over last 20 RTCP Sender Reports) mapping RTP timestamps to NTP wall-clock time.ParticipantSync— Compares NTP-mapped times across one participant's audio and video tracks to compute A/V lip sync adjustments, with time-based slew rate limiting (5ms/s) and a 5ms deadband.SessionTimeline— Maps each participant's independent NTP clock domain onto a shared recording timeline using one-way delay estimation to normalize out clock offsets between participants.SyncEngine— Orchestrator that wires the three layers together, handles wall-clock fallback before SRs arrive, manages the transition to NTP-grounded PTS, and enforces monotonicity and drain. Implements the Sync/TrackSync interfaces so AppWriter is agnostic to which engine is active.