Remove WithPreJitterBufferReceiveTimeEnabled and always enable it

[biglittlebigben]

Summary by CodeRabbit

• Refactor
  • Removed unused internal configuration option and simplified synchronization timestamp handling logic.

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request removes the PreJitterBufferReceiveTimeEnabled configuration option and field from the synchronizer package, along with all related initialization and usage. Additionally, PTS timestamp handling is simplified to always use pkt.ReceivedAt instead of conditional logic selection.

Changes

Cohort / File(s)	Summary
Synchronizer Configuration Cleanup pkg/synchronizer/synchronizer.go, pkg/synchronizer/track.go	Removed PreJitterBufferReceiveTimeEnabled field from SynchronizerConfig and its builder option function. Deleted corresponding field from TrackSynchronizer, including all initialization and log references. Simplified PTS timestamp handling by removing conditional logic and first-packet drop checks tied to this configuration flag.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

slack-ts:cs-devs:1770206669.807439

Suggested reviewers

• boks1971

Poem

🐰 A flag takes flight, no longer required,
Complexity shed, the old ways retired,
Config grows lean with each line we erase,
Timestamps now simple, a cleaner embrace!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main changes: removing the WithPreJitterBufferReceiveTimeEnabled option and making the feature always enabled by simplifying the conditional logic.

_{✏️ 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 benjamin/remove_WithPreJitterBufferReceiveTimeEnabled

---

📜 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 176dbba and 9c64767.

📒 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/errors.go (1)

• ErrPacketTooOld (23-23)

⏰ 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

_{✏️ 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.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/synchronizer/track.go (1)

254-291: ⚠️ Potential issue | 🟡 Minor

Guard against zero ReceivedAt to avoid false "too old" drops and inconsistent logging.

The getPTSWithoutRebase() and getPTSWithRebase() methods use pkt.ReceivedAt directly without a zero-time guard, creating a risk and inconsistency. The initialize() method already includes the guard (if receivedAt.IsZero() { receivedAt = time.Now() }, lines 186-189), but these methods omit it. If ReceivedAt is ever zero, isPacketTooOld() will call time.Since(time.Time{}), which yields a massive duration, causing false packet drops and skewed age/delay logging.

Additionally, pktReceiveTime is assigned but then the code inconsistently uses both pkt.ReceivedAt (lines 261–262, 280, 286–288, 368–369, 398, 404–406) and pktReceiveTime (lines 294, 412), defeating the purpose of the intermediate variable.

✅ Suggested fix (safe fallback + consistent use)

 	now := time.Now()
 	pktReceiveTime := pkt.ReceivedAt
+	if pktReceiveTime.IsZero() {
+		pktReceiveTime = now
+	}
 	if t.firstTime.IsZero() {
 		t.firstTime = now
 		t.logger.Infow(
 			"starting track synchronizer",
 			"state", t,
-			"pktReceiveTime", pkt.ReceivedAt,
-			"startDelay", t.firstTime.Sub(pkt.ReceivedAt),
+			"pktReceiveTime", pktReceiveTime,
+			"startDelay", t.firstTime.Sub(pktReceiveTime),
 		)
 	} else {
 		t.updateGapHistogram(pkt.SequenceNumber - t.lastSN)
 	}
 	t.lastSN = pkt.SequenceNumber

 	ts := pkt.Timestamp

 	// if first packet of a frame was accepted,
 	// accept all packets of the frame even if they are old
 	if ts == t.lastTS {
 		t.stats.numEmitted++
 		return t.lastPTSAdjusted, nil
 	}

 	// if first packet a frame was too old and dropped,
 	// drop all packets of the frame irrespective of whether they are old or not
-	if ts == t.lastTSOldDropped || t.isPacketTooOld(pkt.ReceivedAt) {
+	if ts == t.lastTSOldDropped || t.isPacketTooOld(pktReceiveTime) {
 		t.lastTSOldDropped = ts
 		t.stats.numDroppedOld++
 		t.logger.Infow(
 			"dropping old packet",
 			"currentTS", ts,
-			"receivedAt", pkt.ReceivedAt,
+			"receivedAt", pktReceiveTime,
 			"now", time.Now(),
-			"age", time.Since(pkt.ReceivedAt),
+			"age", time.Since(pktReceiveTime),
 			"state", t,
 		)
 		return 0, ErrPacketTooOld
 	}

Apply the same pattern to getPTSWithRebase() (starting line 357).

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 176dbba and 9c64767.

📒 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/errors.go (1)

• ErrPacketTooOld (23-23)

⏰ 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

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