Synchronizer tidying up

[milos-lk]

Changes aimed to reduce duplication a bit. Extracted 2 functions which are relatively independent, wanted to do more but that requires either having functions with a lot of params of some sort of context which doesn't make it really easier to read (high coupling with caller). Smaller tidying up changes: replacing mono with time and interface with any

Summary by CodeRabbit

• Refactor
  • Updated internal synchronizer module to improve code reliability and maintainability through modernized time-handling and helper method extraction.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR replaces the mono.Now() time source with the standard library time.Now() across the synchronizer package, removing the github.com/livekit/protocol/utils/mono dependency. Additionally, it refactors track synchronization logic to retrieve external media deadlines before acquiring track locks, preventing potential deadlocks during PTS calculations.

Changes

Cohort / File(s)	Summary
Mono time replacement pkg/synchronizer/synchronizer.go	Replaced mono.Now() with time.Now() in getExternalMediaDeadline function, removing mono package dependency.
Track refactoring and deadlock prevention pkg/synchronizer/track.go	Introduced helper methods getSynchronizer() and getSynchronizerDeadline() to retrieve deadlines before acquiring locks. Extracted PTS offset adjustment logic into maybeAdjustPTSOffset(). Replaced all mono.Now() calls with time.Now(). Updated isPacketTooOld() to use time.Since(). Changed logging field type from []interface{} to []any.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Fix for deadlock on inverse locking order #841: Modifies TrackSynchronizer GetPTS paths to read external media deadline before taking the track lock to prevent deadlocks, with similar PTS-normalization and deadline handling updates.

Suggested labels

slack-ts:cs-devs:1770198234.151599

Suggested reviewers

• anunaym14

Poem

🐰 A rabbit hops through synchronized time,
From mono's old ways to stdlib's prime,
No deadlocks here, just smooth PTS flow,
With helpers and refactors stealing the show! ⏰✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Synchronizer tidying up' is vague and generic, using non-descriptive language that doesn't convey the specific changes made to the codebase.	Consider a more specific title that highlights the main changes, such as 'Replace mono with time package and extract helper methods in synchronizer' or 'Refactor synchronizer to remove duplication and modernize time usage'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch tidying-sync

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2ec7166 and bc059a3.

📒 Files selected for processing (2)

• pkg/synchronizer/synchronizer.go
• pkg/synchronizer/track.go

🧰 Additional context used

🧬 Code graph analysis (1)

pkg/synchronizer/track.go (1)

pkg/synchronizer/synchronizer.go (1)

• Synchronizer (139-153)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: test

🔇 Additional comments (9)

pkg/synchronizer/synchronizer.go (1)

320-320: LGTM!

The replacement of mono.Now() with time.Now() is appropriate here since the value is used for elapsed time calculation via now.Sub(startTime). Go's time.Now() includes a monotonic clock component that ensures accurate duration calculations even across wall clock adjustments.

pkg/synchronizer/track.go (8)

130-130: LGTM!

Consistent use of time.Now() for both initialization and comparison in shouldAdjustPTS.

---

251-252: Good deadlock prevention pattern.

Fetching the external media deadline before acquiring the track lock prevents the A-B/B-A deadlock scenario with Synchronizer.End(), which holds the Synchronizer lock while draining tracks.

---

862-874: Well-structured helpers for safe synchronizer access.

The two-step approach (get pointer under lock, then use it) correctly handles the race with Close(): even if t.sync is set to nil after getSynchronizer() returns, the returned pointer remains valid since the Synchronizer object persists.

---

813-834: Good extraction of duplicated PTS adjustment logic.

The helper consolidates the offset adjustment, throttling, and logging that was previously duplicated in both getPTSWithoutRebase and getPTSWithRebase. The logic is preserved correctly.

---

709-710: LGTM!

Idiomatic Go 1.18+ style using any alias.

---

789-791: LGTM!

Consistent time.Now() usage across the throttling mechanism (initialization at line 130, update at line 832, and this check).

---

836-838: LGTM!

Using time.Since() is more idiomatic than mono.Now().Sub() and correctly calculates packet age for comparison against the threshold.

---

326-326: LGTM!

Correct replacement of inline PTS adjustment logic with the new maybeAdjustPTSOffset helper.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}