fix(timeline): quantise the scene timeline once instead of per scene - #412
Merged
Merged
Conversation
Two 1.05s scenes rendered 64 frames where 2.1s at 30fps is 63: each scene rounded 31.5 up on its own and the error accumulated. Twenty 0.35s beats ended 10 frames past their soundtrack. The timeline is now defined in seconds first and quantised once: quantised_spans walks the scene durations and the transition overlaps as real numbers, then rounds each cumulative boundary. A scene's frame count is the difference of two rounded boundaries, so the half frame is paid once by whichever side the boundary rounds toward and never twice. A transition's overlap falls out of the same arithmetic -- it is spans[i].end - spans[i+1].start, not a separately rounded figure that could disagree with the scenes around it. Four sites were computing frames from a duration independently, and they did not agree: - encode/video/tasks.rs, v1 and v2, now both derive from the spans. v2's cursor carries seconds rather than accumulated frames, so `at` resolves on the same grid as the automatic placement. - encode/video_audio.rs rounded each scene again to place audio, so an embedded video's sound drifted from its own picture by a frame per scene. It now reads the spans. - cli/commands/migrate.rs computed the v2 `at`/`duration` values from its own per-scene rounding. Its own frame-identity check caught this: migration of the six-scene fixture went 405 -> 404. It now derives both from quantised_spans, and the check passes. - cli/commands/info.rs printed per-scene frame counts that summed past the total it announced (32 + 32 against 63). It now prints the spans. The migrate snap test's expected 15.0s was itself an artifact of the old chain: scene 4 was given 51 frames where its window at the snapped start is 52. Recomputed by hand from the migrated file's own `at` values and the bpm-11 beat grid, the answer is 451 frames.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #372 (the half that #391 left open).
The defect
Two 1.05 s scenes rendered 64 frames where 2.1 s at 30 fps is 63: each scene
rounded 31.5 up on its own and the error accumulated. Twenty 0.35 s beats ended
10 frames past their soundtrack.
The model
The timeline is defined in seconds first and quantised once.
quantised_spanswalks the scene durations and the transition overlaps as real numbers, then rounds
each cumulative boundary. A scene's frame count is the difference of two rounded
boundaries, so the half frame is paid once by whichever side the boundary rounds
toward — never twice.
A transition's overlap falls out of the same arithmetic: it is
spans[i].end - spans[i+1].start, not a separately rounded figure that coulddisagree with the scenes around it.
Four sites, and they did not agree
encode/video/tasks.rs(v1)round(d·fps)per scene, minus separately rounded transitionsencode/video/tasks.rs(v2)atresolves on the same grid as automatic placementencode/video_audio.rscli/commands/migrate.rsat/durationfrom its own per-scene roundingquantised_spanscli/commands/info.rsvideo_audio.rsmattering here is not cosmetic: it places an embedded video'saudio slice, so it was drifting from that video's own picture by a frame per scene.
migrate caught itself
migrate's own frame-identity check is what surfaced the fourth site — migrationof the six-scene fixture went 405 → 404 as soon as the render side was corrected.
It now derives from the same function, and the check passes:
One expected value changed, and why
snap_beat_on_top_of_the_migrated_file_moves_cuts_and_changes_durationasserted15.0 s. That number was an artifact of the old chain: scene 4 was given 51 frames
where its window at the snapped start is 52 (
round(379.27) - round(327.27)).Recomputed by hand from the migrated file's own
atvalues (0, 1.9, 4.2, 6.5, 9.3667, 11.1) against the bpm-11 beat grid (0 s, 5.4545 s, 10.9091 s), with thethree clamped-forward cuts, the answer is 451 frames. The test now pins that,
and states the structure it comes from rather than a bare literal.
The issue's repros, end to end
infoffprobe)at: 0/at: 1.0, both 2.0 sirisReverts that bite
Restoring the per-scene
round(d·fps)insidequantised_spansfails three of thesix new tests:
Gate
cargo fmt --all --checkclean ·cargo clippy --workspace --all-targets --features rustmotion/studio -D warningsclean ·cargo test --workspace1777 passed.