Files
Anton 0c40770cb2 fix(render): preserve source frame rate by default; add --fps override (#55)
* fix(render): preserve source frame rate by default; add --fps override

extract_segment hardcoded `-r 24`, so every render was forced to 24 fps
regardless of the source. That silently downsamples 30/60 fps footage
(e.g. OBS screen/webcam captures at 60 fps) and contradicts the skill's
stated "match the source unless asked otherwise" principle. There was no
CLI option to change it.

Probe the source's r_frame_rate with ffprobe and pass it through to
ffmpeg verbatim (so fractional rates like 30000/1001 survive without
rounding), falling back to 24 only when the rate can't be determined.
Add a `--fps N` flag to force a specific rate when desired.

Behavior change: renders now keep the source frame rate by default
instead of always producing 24 fps.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(render): resolve one frame rate per render, not per segment

Addresses review feedback (identified by cubic): probing the frame rate
inside extract_segment meant a multi-source EDL mixing rates (e.g. a
30fps and a 60fps source) would encode segments at different rates. The
lossless concat (`-c copy`) requires every segment to share a frame
rate, so that would break the concat for multi-source edits.

Resolve a single output rate once in extract_all_segments and pass it to
every segment: explicit --fps wins, otherwise preserve the first source's
rate (falls back to 24 if unprobeable). Single-source renders still keep
the source rate; multi-source renders stay homogeneous and concat-safe.

extract_segment now takes a resolved `rate` string (still probes its own
source when called standalone with no rate).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(render): harden source frame rate handling

Prefer avg_frame_rate for variable-rate inputs, fall back to r_frame_rate, and accept decimal or rational --fps overrides. Add focused tests for validation, probing, and uniform multi-source render rates.

* fix(render): validate fps before fraction parsing

Restrict FPS input to bounded ffmpeg-compatible numeric and rational forms, canonicalize accepted values, and cover explicit overrides, fallback behavior, and probe failures.

* fix(render): keep canonical fps parsing idempotent

Reject reduced FPS fractions that exceed FFmpeg AVRational component bounds, ensuring every accepted canonical rate can be parsed again safely.

---------

Co-authored-by: Anton Sidorov aka anticodeguy <a@anticodeguy.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-08-29 21:01:48 -07:00

144 lines
5.2 KiB
Python

import argparse
import contextlib
import importlib.util
import io
import json
import subprocess
import tempfile
import unittest
from pathlib import Path
from unittest.mock import patch
MODULE_PATH = Path(__file__).parents[1] / "helpers" / "render.py"
SPEC = importlib.util.spec_from_file_location("video_use_render", MODULE_PATH)
assert SPEC and SPEC.loader
render = importlib.util.module_from_spec(SPEC)
SPEC.loader.exec_module(render)
class ParseFpsTests(unittest.TestCase):
def test_accepts_integer_decimal_and_rational_rates(self):
expected = {
"60": "60/1",
"29.97": "2997/100",
"30000/1001": "30000/1001",
}
for value, canonical in expected.items():
with self.subTest(value=value):
self.assertEqual(render.parse_fps(value), canonical)
def test_canonical_rates_are_idempotent(self):
for value in ("60", "29.97", "30000/1001"):
with self.subTest(value=value):
canonical = render.parse_fps(value)
self.assertEqual(render.parse_fps(canonical), canonical)
def test_rejects_invalid_or_non_positive_rates(self):
for value in (
"",
"nope",
"0",
"-24",
"1/0",
"1e3",
"1_000",
"0.12345678901234567890",
"1" * 33,
):
with self.subTest(value=value):
with self.assertRaises(argparse.ArgumentTypeError):
render.parse_fps(value)
class ProbeSourceFpsTests(unittest.TestCase):
@staticmethod
def _probe_result(avg: str, nominal: str) -> subprocess.CompletedProcess:
stdout = json.dumps({
"streams": [{"avg_frame_rate": avg, "r_frame_rate": nominal}]
})
return subprocess.CompletedProcess([], 0, stdout=stdout, stderr="")
def test_prefers_average_rate(self):
result = self._probe_result("30000/1001", "30/1")
with patch.object(render.subprocess, "run", return_value=result):
self.assertEqual(render.probe_source_fps(Path("source.mp4")), "30000/1001")
def test_falls_back_to_nominal_rate(self):
result = self._probe_result("0/0", "60/1")
with patch.object(render.subprocess, "run", return_value=result):
self.assertEqual(render.probe_source_fps(Path("source.mp4")), "60/1")
def test_returns_none_for_unusable_probe_output(self):
result = subprocess.CompletedProcess([], 0, stdout='{"streams": []}', stderr="")
with patch.object(render.subprocess, "run", return_value=result):
self.assertIsNone(render.probe_source_fps(Path("source.mp4")))
def test_returns_none_when_ffprobe_fails(self):
error = subprocess.CalledProcessError(1, ["ffprobe"])
with patch.object(render.subprocess, "run", side_effect=error):
self.assertIsNone(render.probe_source_fps(Path("source.mp4")))
class RenderRateTests(unittest.TestCase):
@staticmethod
def _edl() -> dict:
return {
"sources": {"first": "first.mp4", "second": "second.mp4"},
"ranges": [
{"source": "first", "start": 0, "end": 1},
{"source": "second", "start": 0, "end": 1},
],
}
def test_multi_source_render_resolves_one_rate_from_first_source(self):
edl = self._edl()
with tempfile.TemporaryDirectory() as temp_dir:
edit_dir = Path(temp_dir)
with (
patch.object(render, "probe_source_fps", return_value="60/1") as probe,
patch.object(render, "extract_segment") as extract,
):
with contextlib.redirect_stdout(io.StringIO()):
render.extract_all_segments(edl, edit_dir, preview=False)
probe.assert_called_once_with((edit_dir / "first.mp4").resolve())
self.assertEqual([call.kwargs["rate"] for call in extract.call_args_list], ["60/1", "60/1"])
def test_explicit_rate_skips_probe_and_applies_to_every_segment(self):
with tempfile.TemporaryDirectory() as temp_dir:
with (
patch.object(render, "probe_source_fps") as probe,
patch.object(render, "extract_segment") as extract,
contextlib.redirect_stdout(io.StringIO()),
):
render.extract_all_segments(
self._edl(), Path(temp_dir), preview=False, fps="30"
)
probe.assert_not_called()
self.assertEqual(
[call.kwargs["rate"] for call in extract.call_args_list],
["30/1", "30/1"],
)
def test_failed_probe_falls_back_to_24_for_every_segment(self):
with tempfile.TemporaryDirectory() as temp_dir:
with (
patch.object(render, "probe_source_fps", return_value=None),
patch.object(render, "extract_segment") as extract,
contextlib.redirect_stdout(io.StringIO()),
):
render.extract_all_segments(
self._edl(), Path(temp_dir), preview=False
)
self.assertEqual(
[call.kwargs["rate"] for call in extract.call_args_list],
["24", "24"],
)
if __name__ == "__main__":
unittest.main()