Skip to content

fix: prevent duplicate frames at trimmed speed boundaries - #888

Merged
EtienneLescot merged 1 commit into
mainfrom
codex/issue-876-render-duration
Sep 28, 2026
Merged

EtienneLescot merged 1 commit into
mainfrom
codex/issue-876-render-duration

Conversation

@EtienneLescot

@EtienneLescot EtienneLescot commented Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Skip zero-width speed regions before splitting render spans in both the TypeScript progress total and Rust compositor. A rounded region at a trimmed clip edge no longer causes the preceding 1x span to be counted and rendered twice.

Related issue

Fixes #876

Type of change

  • Bug fix

Release impact

  • Patch

Desktop impact

  • Windows
  • macOS
  • Linux
  • Installer / packaging

Screenshots / video

No UI changes. The Windows smoke export is in the local temp fixture directory as issue876-windows-smoke.mp4.

Testing

  • npx vitest --run src/lib/exporter/outputFrameCount.test.ts — 11 passed.
  • npx tsc --noEmit and npx tsc -p tsconfig.test.json --noEmit — passed.
  • Targeted Biome check — passed.
  • Targeted Rust compositor test — passed (1 test).
  • Built the Windows compositor addon and ran the Electron CLI export on the supplied main project fixture. ffprobe reports 5,523 video frames and 92.05 s format duration; the supplied RC export has 8,213 frames and 136.883 s.

The archive's separate minimal project computes to 8.333 s and does not reproduce this boundary-rounding case. The Computer Use session exposed no native app/window target (apps was empty and listWindows was unavailable), so a click-driven GUI pass was not possible here.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed frame totals for clips with empty speed regions at trim boundaries, preventing normal-speed portions from being counted twice.
    • Invalid speed values now fall back to normal speed when calculating clip timing.

@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

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: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 4ef9bc18-5e6a-4b80-8fd1-74f4110ec797

📥 Commits

Reviewing files that changed from the base of the PR and between 2416e44 and bc3e62f.

📒 Files selected for processing (3)
  • crates/compositor/src/regions.rs
  • src/lib/exporter/outputFrameCount.test.ts
  • src/lib/exporter/outputFrameCount.ts

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


📝 Walkthrough

Walkthrough

The Rust compositor and TypeScript exporter now skip speed regions with zero or negative clipped width before counting the preceding 1× gap. Regression tests cover a trim-boundary empty region and assert totals of 2,690 frames for one clip and 5,523 frames for two clips at 60 fps.

Changes

Speed-region frame counting

Layer / File(s) Summary
Skip empty regions and verify frame totals
crates/compositor/src/regions.rs, src/lib/exporter/outputFrameCount.ts, src/lib/exporter/outputFrameCount.test.ts
Both frame-counting paths skip regions with zero or negative clipped width before adding a 1× gap. Tests assert the one-clip and two-clip totals; the TypeScript test records the prior 8,213-frame result.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to bc3e6

Empty speed regions at trimmed boundaries no longer double-count normal-speed spans, and the reported case has regression coverage. No unresolved material merge risk is established.

Architecture Summary

Architecture risk: 🟡 Medium · up to bc3e6

The change affects 2 systems.

Changed systems: crates, src

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

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

Before / after behavior

  • observed — Modified behavior in crates/compositor/src/regions.rs: speed_segments_for_window now skips regions whose clipped end is not after their start before emitting any gap. Previously, the gap could be emitted while the cursor remained unchanged, duplicating that span. Valid finite positive speeds are retained; other speeds use 1×.
  • observed — Modified behavior in crates/compositor/src/regions.rs: The exporter frame-total test adds a 60-fps case with an empty region at the first clip’s trim boundary, then asserts that the first clip totals 2,690 frames and both clips total 5,523.
  • observed — Modified behavior in src/lib/exporter/outputFrameCount.test.ts: Added a regression test asserting that an empty speed region at a clip’s trimmed edge does not cause the clip’s normal-speed prefix to be counted twice; the two-clip case must total 5,523 frames at 60 fps. The test comment records the prior 8,213-frame result.
  • observed — Modified behavior in src/lib/exporter/outputFrameCount.ts: clipOutputFrameCount now skips regions when the clipped end is not after the clipped start before adding any 1× gap. This prevents zero-width intervals from causing an unchanged cursor’s gap to be counted twice; valid regions still use the existing speed fallback, frame calculation, and cursor update.

Reliability and maintainability

  • inferred — Risk-relevant change factors for crates: blast_radius_1; blast_radius_2; direct_dependents_1
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main fix: preventing duplicate frames at trimmed speed boundaries.
Description check ✅ Passed The description follows the repository template. It explains the fix, links issue #876, identifies the change as a bug fix with patch impact, lists platform impact, and provides detailed testing resul…
Linked Issues check ✅ Passed Issue #876 requires export duration to follow the edited timeline and avoid duplicate output at trimmed speed boundaries. outputFrameCount.ts and crates/compositor/src/regions.rs now skip clipped …
Out of Scope Changes check ✅ Passed The changes stay within issue #876. They modify the TypeScript frame-count path, the Rust compositor span path, and add regression coverage for the reported boundary condition. No unrelated production…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Warning

Some tools did not complete. Review the errors below.

🔧 Clippy (1.98.1)

Clippy execution failed


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.

@EtienneLescot
EtienneLescot merged commit 4f35409 into main Sep 28, 2026
20 checks passed
@EtienneLescot
EtienneLescot deleted the codex/issue-876-render-duration branch September 28, 2026 17:24
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.

[Bug]: macOS export renders ~137 s from a 95.7 s edited timeline — trim/speed not reflected in the exported duration (2.0.0-rc.1)

1 participant