Skip to content

fix(preview): publish preview frames per fps slot, not per rounded-up millisecond - #347

Merged
hyperb1iss merged 1 commit into
mainfrom
nova/preview-publish-cadence
Oct 3, 2026
Merged

hyperb1iss merged 1 commit into
mainfrom
nova/preview-publish-cadence

Conversation

@hyperb1iss

@hyperb1iss hyperb1iss commented Oct 3, 2026 •

Copy link
Copy Markdown
Owner

The preview frame gate rounded a target rate up to a whole-millisecond interval, so a 30 fps request became a 34 ms gate. The render loop spaces frames about 33 ms apart, which means every other frame failed the gate and preview subscribers asking for 30 fps received 15 to 19. The Remote live preview's acceptance lane measured exactly that on a 480 px VP8 track whose encoder was keeping pace with what it was given.

🛠️ How it works

should_publish_preview_frame now publishes once per 1/fps slot of the uptime clock: a frame publishes when it lands in a later slot than the last published frame. Slots hand out the target rate on average, absorb render jitter in either direction, and still cap a fast loop at a slow target. The slot index is fixed point, so the comparison stays exact past u32 milliseconds of uptime.

🧪 Validation

Check Result
cargo test --locked -p hypercolor-daemon --lib render_thread::pipeline_runtime 28 passed, including two new cases: a 33 ms render cadence publishes 290 to 300 of 300 frames at a 30 fps target (the old gate published 150), and a 60 fps loop against a 2 fps target publishes exactly 20 in ten seconds
cargo fmt --check, cargo clippy -p hypercolor-daemon --lib -D warnings clean

The uptime test now starts its clock on a slot boundary so its two assertions straddle exactly one slot; the behavior it protects is unchanged.

🤖 Generated with Claude Code

https://claude.ai/code/session_017QqD4e2C7kLCBtZBEfJyTx

Summary by CodeRabbit

  • Bug Fixes
    • Preview updates now follow their intended cadence more reliably, including at 30 frames per second.
    • When a 2-per-second preview limit is set, updates are capped accordingly, even when rendering runs at 60 frames per second.

should_publish_preview_frame turned a target rate into a whole
millisecond interval rounded up: 30 fps became 34 ms. The render loop
delivers frames about 33 ms apart, so the gate refused every other
frame and the preview bus carried 15 to 19 fps to anyone asking for
30. The Remote live preview's acceptance lane measured exactly that
on a 480 px VP8 track whose encoder was keeping pace.

The gate now publishes once per 1/fps slot of the uptime clock: a
frame publishes when it lands in a later slot than the last published
frame. Slots hand out the target rate on average, absorb render jitter
in either direction, and still cap a fast loop at a slow target (a
60 fps loop against 2 fps publishes twice a second). The slot index is
fixed point so the comparison stays exact past u32 milliseconds of
uptime, which the existing test now covers from a slot boundary.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017QqD4e2C7kLCBtZBEfJyTx
@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e0cad00d-204b-4079-a2de-361c199ad70c
📥 Commits

Reviewing files that changed from the base of the PR and between e11545b and a3ac27c.

📒 Files selected for processing (1)
  • crates/hypercolor-daemon/src/render_thread/pipeline_runtime.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Preview publication now uses fixed-point uptime slots instead of a rounded-up millisecond interval. Tests cover long uptime, 30 fps publication, and a 2 fps cap on a 60 fps loop.

Changes

Preview Publication Cadence

Layer / File(s) Summary
Slot-based publication gate and tests
crates/hypercolor-daemon/src/render_thread/pipeline_runtime.rs
The publication gate compares slot indices computed from uptime and target FPS. Tests cover behavior across a long-uptime boundary, 30 fps cadence, and a 2 fps cap on a 60 fps loop.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to a3ac2

This change fixes the preview frame cadence so 30 fps subscribers receive the target rate. No actionable merge risk was identified.

Architecture Summary

Architecture risk: 🟡 Medium · up to a3ac2

The change affects 1 system.

Changed systems: crates

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — crates (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in crates/hypercolor-daemon/src/render_thread/pipeline_runtime.rs: should_publish_preview_frame replaces the minimum elapsed-millisecond interval comparison with slot indices computed as u128(at_ms) * max(target_fps, 1) / 1000. It publishes if no prior time exists or the current slot is greater than the prior slot.
  • observed — Modified behavior in crates/hypercolor-daemon/src/render_thread/pipeline_runtime.rs: The long-uptime test now places its starting timestamp on a 50 ms slot boundary instead of using an arbitrary offset beyond u32::MAX; its existing assertions check behavior across that boundary.
  • observed — Modified behavior in crates/hypercolor-daemon/src/render_thread/pipeline_runtime.rs: Adds cadence tests: across 300 frames spaced 33 ms apart at a 30 fps target, 290–300 must publish; across 600 frames at 60 fps with a 2 fps target, exactly 20 must publish.

Reliability and maintainability

  • inferred — Risk-relevant change factors for crates: blast_radius_2; direct_dependents_2
🚥 Pre-merge checks | ✅ 4 | ❌ 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%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: publishing preview frames by FPS slot instead of a rounded-up millisecond interval.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@hyperb1iss
hyperb1iss merged commit d3fecca into main Oct 3, 2026
42 checks passed
@hyperb1iss
hyperb1iss deleted the nova/preview-publish-cadence branch October 3, 2026 03:40
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