Skip to content

Send RTT in probe messages and tune jitter buffer#1265

Merged
kixelated merged 4 commits into
mainfrom
kixelated/rtt-probe
Apr 9, 2026
Merged

Send RTT in probe messages and tune jitter buffer#1265
kixelated merged 4 commits into
mainfrom
kixelated/rtt-probe

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Summary

  • Read RTT from QUIC session stats and include it in moq-lite-04 probe messages instead of hardcoding None
  • Floor min RTT at 10ms before applying 1.25x jitter multiplier
  • Downgrade late frame warnings to console.debug
  • Switch web demo to latency="real-time" mode

Test plan

  • Run just dev and verify RTT values appear in probe messages
  • Confirm jitter buffer shows ~10ms on localhost
  • Check browser console no longer shows late frame warnings at default log level

🤖 Generated with Claude Code

kixelated and others added 3 commits April 9, 2026 12:09
- Read RTT from QUIC session stats and include it in moq-lite-04
  probe messages instead of hardcoding None.
- Floor min RTT at 10ms before applying 1.25x jitter multiplier.
- Downgrade late frame warnings to console.debug.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@kixelated kixelated enabled auto-merge (squash) April 9, 2026 19:10
@coderabbitai

coderabbitai Bot commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

This pull request updates the jitter buffer configuration system across multiple layers. The demo's <moq-watch> element replaces the jitter attribute with a new latency attribute that accepts either millisecond values or the string "real-time". The Sync class exposes its RTT signal field as a public property instead of keeping it private. The Rust publisher's probe message now includes actual RTT values derived from session statistics, converting them to milliseconds, rather than omitting the value entirely.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

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.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main changes: sending RTT in probe messages and tuning the jitter buffer, which aligns with the changeset's core modifications.
Description check ✅ Passed The description is directly related to the changeset, providing specific details about RTT inclusion in probe messages, jitter buffer tuning, logging downgrades, and web demo updates that match the changes made.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch kixelated/rtt-probe
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch kixelated/rtt-probe

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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

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

⚠️ Outside diff range comments (1)
js/watch/src/sync.ts (1)

79-87: ⚠️ Potential issue | 🟠 Major

Apply RTT floor before multiplier to match the stated behavior.

At Line 86, the math currently floors after scaling (max(10, minRtt * 1.25)).
If the target behavior is “floor RTT at 10ms, then multiply,” this should be max(10, minRtt) * 1.25.

🐛 Proposed fix
-				const jitter = Math.max(MIN_JITTER, this.#minRtt * 1.25) as Time.Milli;
+				const baselineRtt = Math.max(MIN_JITTER, this.#minRtt);
+				const jitter = (baselineRtt * 1.25) as Time.Milli;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@js/watch/src/sync.ts` around lines 79 - 87, The jitter calculation applies
the MIN_JITTER floor after scaling; change it to floor the RTT first then apply
the multiplier: when computing jitter in the block that reads this.rtt (using
effect.get and updating this.#minRtt), replace the current use of
Math.max(MIN_JITTER, this.#minRtt * 1.25) with Math.max(MIN_JITTER,
this.#minRtt) * 1.25 (preserving the Time.Milli cast) before calling
this.jitter.set.
🧹 Nitpick comments (1)
js/watch/src/sync.ts (1)

47-58: Make rtt readonly to prevent accidental signal reassignment.

This property is only assigned once during initialization at line 58. Adding readonly prevents accidental rebinding after construction while maintaining public access.

♻️ Proposed change
-	rtt?: Signal<number | undefined>;
+	readonly rtt?: Signal<number | undefined>;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@js/watch/src/sync.ts` around lines 47 - 58, Make the instance property rtt
readonly to prevent accidental reassignment after construction: update the
declaration of rtt (the Signal<number | undefined> property on the class) to be
readonly and leave its initialization in the constructor as-is (it is assigned
from props?.rtt in constructor). Ensure the readonly modifier is only on the
property declaration (rtt) so external code can still read the signal but cannot
rebind the property.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@js/watch/src/sync.ts`:
- Around line 79-87: The jitter calculation applies the MIN_JITTER floor after
scaling; change it to floor the RTT first then apply the multiplier: when
computing jitter in the block that reads this.rtt (using effect.get and updating
this.#minRtt), replace the current use of Math.max(MIN_JITTER, this.#minRtt *
1.25) with Math.max(MIN_JITTER, this.#minRtt) * 1.25 (preserving the Time.Milli
cast) before calling this.jitter.set.

---

Nitpick comments:
In `@js/watch/src/sync.ts`:
- Around line 47-58: Make the instance property rtt readonly to prevent
accidental reassignment after construction: update the declaration of rtt (the
Signal<number | undefined> property on the class) to be readonly and leave its
initialization in the constructor as-is (it is assigned from props?.rtt in
constructor). Ensure the readonly modifier is only on the property declaration
(rtt) so external code can still read the signal but cannot rebind the property.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b1105b83-2187-49ac-ba64-68733e40db5b

📥 Commits

Reviewing files that changed from the base of the PR and between 709b578 and fc306fa.

📒 Files selected for processing (3)
  • demo/web/src/index.html
  • js/watch/src/sync.ts
  • rs/moq-lite/src/lite/publisher.rs

@kixelated kixelated disabled auto-merge April 9, 2026 19:35
@kixelated kixelated merged commit 24043b2 into main Apr 9, 2026
0 of 2 checks passed
@kixelated kixelated deleted the kixelated/rtt-probe branch April 9, 2026 19:35
This was referenced Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant