Skip to content

feat(speed-ramp): cap output gap duration - #53

Open
Joilence wants to merge 1 commit into
shreyaskarnik:mainfrom
Joilence:pr/output-gap-cap
Open

Joilence wants to merge 1 commit into
shreyaskarnik:mainfrom
Joilence:pr/output-gap-cap

Conversation

@Joilence

Copy link
Copy Markdown
Contributor

What

  • Add speedRamp.maxGapMs to cap each gap's output duration, including leading and trailing gaps.
  • With speedRamp: { gapSpeed: 1, maxGapMs: 2000 }, a 60-second gap becomes 2 seconds; shorter gaps stay at normal speed.
  • Override fixed gap-speed settings while preserving explicit scene speeds and remapping chapters, subtitles and camera timing.
  • Reject non-positive and non-finite caps. Existing configurations remain unchanged.

Test

  • Build and full suite passed before comment/assertion cleanup: 879 tests passed, 2 skipped.
  • All 9 speed-ramp tests passed after cleanup.
  • Compared 96 legacy configurations against upstream; outputs matched.
  • Real FFmpeg export: 29-second input produced 14.033 seconds against a 14-second plan, within one 30 fps frame.

@shreyaskarnik

Copy link
Copy Markdown
Owner

Thanks @Joilence! Capping wait time by output duration is a nicer way to set this up than a fixed gapSpeed. "No gap longer than 2 seconds" is how people actually think about dead time in a demo. The implementation holds up too. I checked the parts outside the diff:

  • No crash without a config. Removing the early return is safe, because applySpeedRampToTimeline falls back to config ?? { gapSpeed: 1.0 }.
  • Very large ratios work. A 10-minute gap capped to 2s runs at 300×, and the existing buildAtempoChain already chains past ffmpeg's 100× per-instance limit.
  • Every export path gets it. No path rebuilds the speed-ramp config field by field, so maxGapMs reaches pipeline, argo export, preview and variants unchanged.
  • Existing configs are unaffected. Without maxGapMs, the old branches run exactly as before, which is consistent with your 96-configuration comparison.

One design question before merging

Right now maxGapMs replaces the other gap speeds instead of bounding them, so adding a cap can make a gap slower. Running computeSegments from this branch on two scenes with a 4s gap between them:

gapSpeed 3                              → gap plays 1333ms
gapSpeed 3 + maxGapMs 2000              → gap plays 2000ms   (50% longer)

scene playbackSpeed 25 (loading scene)  → gap after it plays 160ms
... + maxGapMs 2000                     → gap after it plays 2000ms   (12.5× longer)

The README states the override, so it's documented, and your second test pins it on purpose. I'd still suggest reconsidering, for two reasons:

  1. The name implies a maximum. Most people will read maxGapMs: 2000 as "never longer than 2s" and not expect it to also switch off a faster gapSpeed.
  2. It undoes an existing behavior. playbackSpeed: 25 on a loading scene is documented to fast-forward the dead time after it as well. Adding a cap anywhere in the config quietly turns that back into a 2s wait.

The alternative is one change per gap: keep the existing rule, then speed up only when the output would still exceed the cap.

useSpeed = Math.max(useSpeed, gapMs / maxGapMs);

That way the cap only ever shortens a gap, the playbackSpeed inheritance still works, and the README no longer needs the "overrides gapSpeed, minGapMs and inherited scene speed" caveat. Your first test still passes as written. The second one changes from 5000 to 4500, since its trailing 1s gap after the 2× scene would keep inheriting 2× (500ms) instead of dropping back to 1× (1000ms).

If there's a case where you want the cap to replace a faster speed, I'm happy to hear it. I may be missing the scenario that led you to pin that behavior.

Minor, not blocking

An invalid maxGapMs only throws inside computeSegments, which runs at export, after TTS and recording have finished. gapSpeed isn't checked any earlier either, so this matches existing behavior, but a check in argo validate would catch a typo before a long recording. Could be a follow-up.

Thanks again!

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.

2 participants