Clamp clip duration to the footage remaining after skip_time

[christophervoelpel]

Summary

Clamp video duration to footage remaining after skip_time, then align it to the target frame rate. Recalculate earlier clips when a later clip raises the target frame rate, preserving insertion-order-independent durations. Addresses SM-7 and SM-15.

An unknown probed duration (0) does not reject or clamp a positive caller-supplied duration. Known durations still reject seeks at or beyond the end. Negative time values are rejected, images require an explicit positive duration, and omitted audio duration retains its media-duration fallback (SM-16).

Validation

• FFmpeg suite: 16 passed, including real local render/A/V-sync checks.
• Unknown-duration regression failed before the follow-up fix and passed afterward.
• An independent 24-to-60-fps probe preserved the requested duration after recalculation.
• git diff --check: passed.

The unknown-duration case uses mocked probe metadata; no real asset with an unprobeable duration or deployed container was validated in this follow-up.

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [Mr-Lopes]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [alzmcr]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [rodolfocaldeira]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [mariamnyoussef]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [gDuperran]

[christophervoelpel]

Review: needs rework — the new guard hard-fails footage that renders today

Multi-agent review (reviewer → independent critique agent re-verifying each claim against the code). The critique pass rejected the reviewer's aggressive test-slimming proposal; details at the bottom.

The clamp itself is correct and well-targeted. It's applied once, at input registration, so every downstream consumer (trim, atrim, the xfade offset accumulation at ffmpeg.py:428-452) stays consistent, and re-quantising from raw_duration on an fps bump avoids compounding rounding. No test asserts the ffmpeg argv string — they assert input state or real probed durations. I verified the clamp, negative-value and fps-order tests all fail against main (fps case: 1.0417 vs 1.05). The three ids map cleanly: clamp, negative/excess rejection, fps requantisation.

Caution

Blocker: a container with no format.duration now hard-fails

get_video_properties defaults the duration (actions_lib/ffmpeg.py:120):

'duration': float(properties['format'].get('duration', 0))

So a container that doesn't report a duration probes as 0.0, and the new guard then raises for any skip_time >= 0.0 — including skip_time=0 with an explicit duration:

if skip_time >= properties['duration']:
  raise ValueError(f'skip_time ({skip_time}) must be less than video duration'

This is reachable, not theoretical: ui/src/app/storyboard/add-scene-dialog/add-scene-dialog.html:43 is accept="video/*", and browser MediaRecorder output (WebM/MKV) classically ships without a container-level duration. Those files render today on the duration > 0 path and would become a hard failure after this PR.

Fix (2 lines): guard on a known duration —

if properties['duration'] > 0 and skip_time >= properties['duration']:

and in _clean_duration, skip clamping when source_duration <= 0 (unknown rather than zero).

Dead code introduced alongside the fix

_clean_duration (ffmpeg.py:96-109) takes both available_duration and source_duration, but available = max(0.0, source - skip) with skip >= 0 means available <= source always — so min(source_duration, available_duration) is always available_duration. That makes three things dead:

• the source_duration parameter
• the 'source_duration' input key at line 303
• the max(0.0, ...) at line 272, unreachable after the raise above it

Keep max(1, round(...)) — that one is genuinely needed for sub-frame remainders.

Minor, same class: min_value (ffmpeg.py:89-92) is only ever 0.0, so the f'{name} must be >= {min_value}' branch is speculative. One line to drop if you're in the file anyway.

One test to delete

test_ffmpeg.py:499-573 — test_shipped_workflow_examples_accepted_by_ffmpeg (~75 lines) transcribes the arrangement loop from actions/combine_video.py:107-135 and then asserts len(inputs) == len(arrangement). It passes against old code, and it will keep passing as combine_video.py's defaults drift — so it guards nothing while duplicating logic across two files.

Behaviour change worth documenting

add_image now rejects duration <= 0. But combine_video.py:109 defaults duration = arr.get('duration', -1) for images too, so a hand-authored arrangement with an image and no explicit duration becomes a new hard failure. Same class as the blocker above. This is defensible and arguably correct — it just isn't in the PR body, and it's the item most likely to break an existing arrangement.

Explicitly not recommended

The first-pass review proposed shrinking the test file from 419 → ~170 lines. The critique agent rejected most of that:

• Keep test_real_ffmpeg_render_av_sync (~115 lines). It's the only test that would have caught the original desync end to end, and .github/workflows/python-tests.yml:32 installs ffmpeg, so it actually runs in CI.
• Keep test_target_fps_order_independent_determinism. It's the only guard for the fps-requantisation half of this PR.
• Don't reuse test_audio_render_matrix's source_videos fixture. It's a pytest fixture; test_ffmpeg.py is a unittest.TestCase. Not free.
• Don't rewrite the file into subTest tables. Realistic deletable set is ~60–80 lines (the test above, plus merging the longhand negative-value asserts), not 250.

Formatting nit: the 92-char docstring at ffmpeg.py:102 is the only new line over pyink's line-length = 80.

Method caveat: the 0-duration failure mode is reasoned from the code path, not observed — I found no 0-duration asset in-repo to reproduce it with.

[christophervoelpel]

Consolidated Review

Verdict: Merge after one small fix. This note reconciles the 19:50 comment on this PR against head 13972f8, and every item was re-verified by tracing callers on both sides of the guard and running the tests or probes under Evidence. The regression is confirmed at code level with a mocked probe; no real container without a probeable duration could be produced locally, so its frequency in practice is unproven. The two-line guard is still worth landing: it restores base behaviour whenever ffprobe reports no duration.

Do before merge

1. actions_lib/ffmpeg.py:267 (and _clean_duration, ffmpeg.py:96-109): change the guard from if skip_time >= properties['duration'] to if properties['duration'] > 0 and skip_time >= properties['duration'], and skip clamping when source_duration <= 0. Why: get_video_properties defaults duration to 0.0 when format.duration is absent (ffmpeg.py:143); an unknown duration, reported as 0.0, is treated as zero by the guard, so add_video(skip_time=0.0, duration=5.0, ...) now raises even with a valid caller-supplied duration. Confirmed by mocking duration=0.0: base accepts and records duration=5.0, head raises ValueError. Acceptance: the mocked call succeeds on head, and test_ffmpeg.py still passes (15 tests).

Sequencing note: once this lands, re-check whether source_duration (parameter and the 'source_duration' key at ffmpeg.py:300) is still dead. It is unused today because the raise at line 267 guarantees available_duration <= source_duration, but this fix reopens a path where duration is unknown and source_duration becomes the fallback. Do not delete it here.

Optional, does not block

• Drop the speculative non-zero min_value branch in _require_finite_number (ffmpeg.py:89-92); all six call sites pass 0.0.
• Consider deleting test_shipped_workflow_examples_accepted_by_ffmpeg (test_ffmpeg.py:499-573): it mocks a fixed duration=30.0 and only asserts input count, so it passes unmodified on base and never exercises the clamp, negative-value guards, or fps requantization. It still guards the arrangement-parsing loop against schema drift, so keeping it is fine.
• Wrap the 92-char docstring at ffmpeg.py:102, the only new line past pyink's 80-column limit.

Rejected or superseded, do not re-litigate

• The 19:50 comment's claim that add_image's new duration <= 0 rejection "isn't in the PR body": not accurate. The PR body has a dedicated section on the add_image duration contract naming both shipped arrangement files, which already carry explicit positive image durations. Fail-fast instead of a degenerate end < start overlay window is intentional and strictly safer than base.
• The crossfade-overlap-vs-post-seek-remainder gap: real, but pre-existing in combine() (ffmpeg.py:470-515), untouched here, already guarded client-side by findTransitionContractViolation. A separate ticket, not this PR.
• Shrinking the test file from 419 to roughly 170 lines: already rejected within the 19:50 comment's own critique. test_real_ffmpeg_render_av_sync and test_target_fps_order_independent_determinism each guard something this PR changes and should stay.

Evidence

• TZ=UTC python -m pytest -q actions_lib/test/test_ffmpeg.py at head: 15 passed.
• Mocked duration=0.0, called add_video(skip_time=0.0, duration=5.0, ...) on base versus head: base accepts, head raises, reproduced independently twice.
• test_shipped_workflow_examples_accepted_by_ffmpeg copied onto base ffmpeg.py: 1 passed, confirming no PR-specific coverage.
• Line-length check against diff hunks: only line 102 among over-80-column lines falls inside this PR's changes.
• Attempted a real webm with an undiscoverable format.duration (pipe capture, FIFO stream, killed mid-capture, truncated file): ffprobe recovered a duration each time, confirming the defect at the code level but not on a real asset.
• Not run: live deployment, browser upload through add-scene-dialog, the Cloud Run image's pinned ffprobe version, paid model calls.

Way forward

• Suggested order for the batch: Use an explicit MIME allow-list and support AVIF #196, Clear video source on resolve failure and keep cached blobs #205, Validate video duration limits and audio parameter types #204, Exclude archived candidates from the rendered video #199 and Clamp clip duration to the footage remaining after skip_time #200 first (independent, ready or one small fix each), then Preserve deployed env values on redeploy and accept quoted config #197 and Remove the duplicate GCS signing cache from orch.py #198 once their fixes land, then the stacked children Validate model-returned product and image ids #201 (on Use an explicit MIME allow-list and support AVIF #196), Treat an absent storyboard key as no change #203 (on Remove the duplicate GCS signing cache from orch.py #198) and Guard setup inputs on the real project load error and hydration state #202 (on Exclude archived candidates from the rendered video #199), each rebased onto main after its parent squash-merges and retargeted to main so CI runs.
• Push the two-line fix as a new commit, re-run test_ffmpeg.py, then squash-merge as usual.
• Confirm the Cloud Run image's pinned ffprobe behaves like local regarding duration probing, the one boundary this review could not exercise directly.

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [gDuperran,
  mariamnyoussef]

[christophervoelpel]

Follow-up on Consolidated Review

I addressed the unknown-duration blocker from the Consolidated Review on head a0f3132304ca.

• A probed duration of 0 is now treated as unknown: a positive caller-supplied clip duration is preserved instead of being rejected or clamped to zero.
• Known durations still enforce seek and remaining-footage limits, with the existing frame-rate recalculation behavior retained.

Verification: the FFmpeg suite passed with 16 tests, including local FFmpeg render and audio/video-sync checks. The unknown-duration regression failed before the fix and passed afterward; an independent 24-to-60-fps probe also preserved the requested duration. The unknown metadata case was mocked, and no deployed container was validated.

Current-head GitHub CI is complete: Python 3.11/3.12/3.13, UI build/lint/tests, deploy checks and security scans passed. Conditional zizmor jobs were skipped.

[victor-paunescu]

When source_duration <= 0 (unprobeable container duration) and the caller passes duration <= 0 (such as the -1 default in actions/combine_video.py meaning "use the rest of the clip"), _clean_duration enters the else branch (effective_duration = min(0.0, 0.0) = 0.0) and max(1, round(0.0 / frame_duration)) evaluates to 1 frame (1.0 / target_fps, ~0.033s). Instead of failing fast, combine() then passes duration=0.0333... to trim and atrim, silently truncating the entire clip to a single frame.

Raising a ValueError when both duration <= 0 and source_duration <= 0 ensures unprobeable clips require an explicit positive duration rather than silently rendering 1 frame.

Suggested change

	if duration > 0:
	effective_duration = (
	min(duration, available_duration)
	if source_duration > 0
	else duration
	)
	else:
	effective_duration = min(source_duration, available_duration)
	if duration > 0:
	effective_duration = (
	min(duration, available_duration)
	if source_duration > 0
	else duration
	)
	elif source_duration > 0:
	effective_duration = min(source_duration, available_duration)
	else:
	raise ValueError(
	'Explicit positive duration is required when video source duration is'
	' unknown'
	)

[christophervoelpel]

Agreed, applied in 453ca1e (also added a Raises: section to the docstring).

This was worse than a silent truncation relative to main, which I checked with real ffmpeg on a 3 s clip. On main, this path produced clean_duration = 0, and ffmpeg treats trim=duration=0 as no limit, so the output was the full 3.000 s. With the max(1, ...) in this PR, it became trim=duration=0.0333 → 0.033 s output. So the PR had turned "whole clip" into "one frame". Failing fast is the right call, since a 0 duration would also corrupt the xfade offset arithmetic.

New test test_unknown_source_duration_requires_explicit_duration: it fails against the previous commit and passes now, and it also asserts nothing was appended to inputs.

[victor-paunescu]

With mock_get_props returning duration: 10.0 and skip_time=3.0, available_duration is 7.0. Because Control 2 passes duration=7.0, all four cases in test_add_video_duration_clamp_and_controls assert duration == available_duration (7.0, 7.0, 10.0, 7.0). Passing a duration strictly less than available_duration (e.g. duration=4.0) in Control 2 verifies that an explicit positive duration within the remaining footage is preserved rather than clamped to available_duration.

Suggested change

	# Control 2: skip_time=3, duration=7 is unchanged (7.0 s)
	ffmpeg4 = FFMPEG()
	ffmpeg4.add_video(
	path='clip.mp4',
	skip_time=3.0,
	duration=7.0,
	transition=None,
	transition_overlap=0,
	)
	self.assertEqual(ffmpeg4.inputs[0]['duration'], 7.0)
	# Control 2: skip_time=3, duration=4 (< available 7 s) is unchanged (4.0 s)
	ffmpeg4 = FFMPEG()
	ffmpeg4.add_video(
	path='clip.mp4',
	skip_time=3.0,
	duration=4.0,
	transition=None,
	transition_overlap=0,
	)
	self.assertEqual(ffmpeg4.inputs[0]['duration'], 4.0)

[christophervoelpel]

Good catch, applied in 453ca1e. Control 2 now uses duration=4.0 and asserts 4.0.

Mutation check: replacing min(duration, available_duration) with available_duration (i.e. always clamping to the remaining footage) now fails this test. With the old duration=7.0 control it passed.

[victor-paunescu]

Falling back to probe_data['format']['duration'] if duration is absent from a stream dict can mask A/V stream desync because format.duration is a single shared container value (max(v_dur, a_dur)), which would set v_dur == a_dur and make abs(a_dur - v_dur) == 0.0 pass automatically. Indexing s['duration'] directly ensures the test asserts each stream's independent duration.

Suggested change

	v_dur, a_dur = None, None
	for s in probe_data.get('streams', []):
	if s.get('codec_type') == 'video':
	v_dur = float(s.get('duration', probe_data['format']['duration']))
	elif s.get('codec_type') == 'audio':
	a_dur = float(s.get('duration', probe_data['format']['duration']))
	v_dur, a_dur = None, None
	for s in probe_data.get('streams', []):
	if s.get('codec_type') == 'video':
	v_dur = float(s['duration'])
	elif s.get('codec_type') == 'audio':
	a_dur = float(s['duration'])

[christophervoelpel]

Agreed, applied in 453ca1e. The same format.duration fallback was also in the crossfade half of this test (the out_xfade probe), so I removed it there too; it had the same masking problem.

I checked that ffprobe reports per-stream duration for these libx264/aac MP4 outputs (video,3.000000 / audio,3.000000), so indexing s['duration'] directly is safe and not flaky. Full suite: 715 passed.