feat: raise playback-speed cap to 100x#80
Conversation
Chromium hard-caps HTMLMediaElement.playbackRate at 16, so preview and the real-time audio-export capture cannot exceed 16x. This takes both paths off the media element only above 16x, leaving the <=16x behaviour untouched: - Preview: >16x segments frame-step (a virtual clock forward-seeks the muted element each frame); <=16x plays natively exactly as before. - Audio export: a timeline with any >16x segment renders offline with a streaming WSOLA pitch-preserving time-stretch; <=16x keeps the proven real-time capture path. - Video export already retimed frames offline with no ceiling - unchanged. - Timeline-segment math is extracted to timelineSegments.ts so the video decoder and the offline audio path derive identical segments (A/V sync). - UI: the custom-speed input now accepts up to 100x (presets unchanged), with a hint that >16x previews as muted frame-stepping while export is unaffected. Closes getopenscreen#79 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR raises the playback-speed cap to 100x, adds native-cap preview stepping above 16x, and routes fast-speed export audio through an offline WSOLA-based path. Shared timeline-segmentation helpers are introduced and reused across preview and export code. ChangesPlayback speed cap increase
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant RAF as AnimationFrame
participant Handlers as videoEventHandlers
participant Video as HTMLVideoElement
RAF->>Handlers: updateTime tick
Handlers->>Handlers: compute activeSpeed
alt speed > MAX_NATIVE_PLAYBACK_RATE
Handlers->>Handlers: advance virtual clock
Handlers->>Video: seek to virtual time (muted)
Video-->>Handlers: seeked event
Handlers->>Handlers: clear step-seek in-flight
else speed <= MAX_NATIVE_PLAYBACK_RATE
Handlers->>Video: set native playbackRate
Handlers->>Video: resume native play
end
sequenceDiagram
participant Exporter as VideoExporter
participant Processor as AudioProcessor
participant Timeline as timelineSegments
participant Wsola as WsolaTimeStretcher
participant Muxer as VideoMuxer
Exporter->>Processor: process(..., targetFrameRate)
Processor->>Timeline: buildSpeedSegments(trim, speedRegions)
Processor->>Timeline: maxTimelineSpeed(segments)
alt maxSpeed > MAX_NATIVE_PLAYBACK_RATE
Processor->>Wsola: stretch per segment via push/flush
Wsola-->>Processor: stretched PCM
Processor->>Muxer: mux encoded offline audio
else maxSpeed <= MAX_NATIVE_PLAYBACK_RATE
Processor->>Muxer: mux real-time pitch-preserved audio
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/exporter/timelineSegments.ts`:
- Around line 24-50: The computeKeepSegments function can move cursor backward
when trimRegions overlap or nest because it assigns cursor directly from each
trim’s end, which can cause already-trimmed ranges to be emitted again. Update
the loop in computeKeepSegments to keep cursor monotonic by advancing it with
the furthest trim end seen so far, while preserving the existing
sorted-by-startMs flow and the final tail segment handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc8cfcf5-66d8-4cee-af1a-30cdd01afdee
📒 Files selected for processing (26)
src/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/customPlaybackSpeed.test.tssrc/components/video-editor/types.tssrc/components/video-editor/videoPlayback/videoEventHandlers.tssrc/i18n/locales/ar/settings.jsonsrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/it/settings.jsonsrc/i18n/locales/ja-JP/settings.jsonsrc/i18n/locales/ko-KR/settings.jsonsrc/i18n/locales/pt-BR/settings.jsonsrc/i18n/locales/ru/settings.jsonsrc/i18n/locales/tr/settings.jsonsrc/i18n/locales/vi/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/i18n/locales/zh-TW/settings.jsonsrc/lib/exporter/audioEncoder.tssrc/lib/exporter/audioTimeStretch.test.tssrc/lib/exporter/audioTimeStretch.tssrc/lib/exporter/streamingDecoder.tssrc/lib/exporter/timelineSegments.test.tssrc/lib/exporter/timelineSegments.tssrc/lib/exporter/videoExporter.test.tssrc/lib/exporter/videoExporter.ts
A trim nested inside another (the list is sorted only by start) could move the keep-cursor backward and re-emit source an earlier trim already removed. Clamp with Math.max so the shared video + offline-audio segment builder stays correct for overlapping regions or legacy saved data. No change for non-overlapping trims. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
A few issues from a careful read. The main one is a webcam-preview freeze on Windows that this PR's throttle for the main preview doesn't cover. The remaining items are smaller.
Blocking
- Webcam preview still re-seeks every frame in >16× regions (
VideoPlayback.tsx~1866) — see inline comment. Reproduces the failure mode the main preview was just fixed for. TheseekSteppingRef/stepSeekInFlightmachinery added for the main video needs an equivalent here, or at minimum the resync needs to be skipped while the main video is in seek-stepping mode.
Worth fixing before merge
appendOutputis O(N²) in output samples for long regions (audioEncoder.ts:717). The offline path is selected whenever any segment exceeds 16×, so a project with one >16× spike and a long 1× run will spend quadratic time copying the accumulator. A circular buffer / amortized slab fixes it.- First decoded
AudioData.timestampis non-zero in practice (audioEncoder.ts:827). TheframeStartSampleseed gives a source-time origin offset from 0, which can drop a few hundred input samples at the start of the timeline. Worth knowing about; the test sources don't exercise it. - Misleading test comment (
videoExporter.test.ts:55) — see inline comment.
Minor
- Silence pad in
finalizeSegment(audioEncoder.ts:761) deserves a one-line comment so future readers don't try to "fix" the underrun branch — the silence pad is what keeps A/V lock when WSOLA under-fills on a short high-speed segment.
Overall the seek-stepping throttle for the main preview is a thoughtful fix, but the same fix has to land on the webcam path before this is safe on Windows. The O(N²) appendOutput is the other real concern.
- Skip the webcam per-frame currentTime resync while the main video is in >16x seek-stepping, so its decoder isn't driven perpetually mid-seek — the same Windows freeze the main-preview throttle fixed, on the webcam path. - Replace the O(N^2) grow-and-copy output accumulator in the offline audio path with a front-drained chunk queue (PlanarChunkQueue, O(1) amortized) + unit tests. - Keep offline audio on raw source timestamps (the same origin the video decoder uses) so codec preroll is dropped and A/V stays locked; add a clarifying comment rather than re-anchoring, which would desync audio from the video path. - Clarify the silence-pad and fast-path-block test comments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the careful read — all addressed in b380556. Webcam preview freeze (blocking). Fixed. The webcam resync is now skipped while the main video is in seek-stepping (
First Silence pad comment and misleading 100× test comment — both reworded/annotated as suggested. All green locally (typecheck, Biome, 347 tests incl. the new queue tests). Preview + offline export re-verified on macOS and Windows. |
…ilence) The video export maps source content at time τ to output τ/speed, holding its first decoded frame over the pre-roll head (streamingDecoder decodeAll). The offline audio path instead left-packed its first decoded sample to output 0, so for a source whose first audio timestamp is non-zero (codec priming, or an audio track that starts after the video) audio led video by ~T/speed across the first segment. Feed that head gap as source-domain silence into segment 0's stretcher (compressed by the segment speed) so real audio keeps its true source-time position and stays aligned with video. No-op when the first timestamp is ~0, so self-recorded content is unaffected; negative codec preroll is still discarded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Following up on the first- The key is how the two paths position the pre-roll head. The video path ( The correct fix keeps raw timestamps (no re-anchor — that would desync the whole timeline, as noted): when segment 0's first sample lands at I verified this against the actual frame-emission code (that video does hold-from-0 rather than subtract an origin) and stress-tested the WSOLA silence→audio boundary; residual imprecision is sub-grain and non-accumulating. Green locally (347 tests). Thanks for pushing on this one — it turned a "known minor asymmetry" into an actual correctness fix. |
2 participants
Closes #79.
What
Raises the editor's playback-speed cap from 16× to 100×.
The cap existed because Chromium hard-caps
HTMLMediaElement.playbackRateat 16× (setting more throwsNotSupportedError), and both preview and audio export drove speed straight through a media element. This PR takes the twoplaybackRate-bound paths off the real-time engine only for segments above 16×; everything at or below 16× keeps its exact current behaviour.How
Shared segment math (
timelineSegments.ts) —computeKeepSegments+splitBySpeedare extracted fromstreamingDecoder's private methods into one module now used by both the video decoder and the new offline audio path, so the two can't compute different timelines (behaviour-preserving refactor; covered by new unit tests).Preview (
videoEventHandlers.ts,VideoPlayback.tsx) — for a segment faster than 16×, the muted element stays "playing" at the native ceiling while the rAF loop advances a virtual clock at the true speed and forward-seeks the element to it each frame ("seek-stepping" — a fast frame-step, like native editors at extreme rates). The ≤16× native path is unchanged. Supplemental audio is paused and the webcam rate is clamped in those regions.Video export — no change needed.
streamingDecoderalready retimes frames offline (sourceTime = start + (frameIndex/fps)·speed) with no speed ceiling; 100× worked already and was only fenced off by the UI.Audio export (
audioEncoder.ts,audioTimeStretch.ts) — the real-time<audio>+MediaRecordercapture can't exceed 16×, so when any segment does, the whole speed timeline is rendered offline with a streaming WSOLA (pitch-preserving) time-stretch instead. Timelines whose fastest segment is ≤16× still use the existing real-time path unchanged. Each segment's output is clamped toround(inputSamples/speed)so audio stays locked to the independently-retimed video, and the stretcher streams (bounded memory) so a multi-hour region isn't held in RAM.UI — the custom-speed input's cap is raised (16→100); the preset buttons are untouched. A small hint appears when a region is set above 16× explaining that preview frame-steps and is muted there while the export is unaffected (added to all 13 locales). The existing "too fast" message is interpolated so it reflects the new cap (
{{max}}).Tradeoffs / notes
atempo, Premiere audio remap, CapCut all cap there).Tests
New unit tests: WSOLA pitch-preservation + length scaling + passthrough + amplitude (
audioTimeStretch.test.ts), shared segment math (timelineSegments.test.ts), updated cap boundary tests (customPlaybackSpeed.test.ts), 100× fast-path guard (videoExporter.test.ts). Full suite green; typecheck and Biome clean.Testing / platform
Verified end-to-end on macOS (Apple Silicon) and Windows — preview frame-stepping above 16× and the offline pitch-preserved audio export both check out on each. (An early revision froze the >16× preview on Windows because it seeked the element every animation frame and the slower decoder stayed perpetually mid-seek; the seek is now throttled to
seekedcompletion, so it steps frames on both.) I haven't tested Linux.Summary by CodeRabbit