diff --git a/milestones/README.md b/milestones/README.md index d820d9d..dda9ad0 100644 --- a/milestones/README.md +++ b/milestones/README.md @@ -5,10 +5,13 @@ repositories that install `docgen` and maintain their own demo bundle. The library no longer ships an in-repo dogfood; consumers are the integration test of record. -**Active:** **[timing-start-end.md](timing-start-end.md)** — -`timing.json` word/segment `start` / `end` must be JSON numbers. +**Active:** **[ffprobe-returncode.md](ffprobe-returncode.md)** — +ffprobe duration probes must honor exit status; image compose needs `-t`. **Shipped:** +- **[timing-start-end.md](timing-start-end.md)** — + `timing.json` word/segment `start` / `end` must be JSON numbers + (#134). - **[wizard-path-ref.md](wizard-path-ref.md)** — wizard `open-bundle` `path` and `tool/update` `ref` must be JSON strings (#133). diff --git a/milestones/ffprobe-returncode.md b/milestones/ffprobe-returncode.md new file mode 100644 index 0000000..917f4f0 --- /dev/null +++ b/milestones/ffprobe-returncode.md @@ -0,0 +1,39 @@ +# Milestone: ffprobe duration probes must honor returncode + +**Status:** Active +**PR:** [#135](https://github.com/jmjava/documentation-generator/pull/135) +**Depends on:** `milestones/compose-ffmpeg-timeout.md` (PR #121), +`milestones/timing-start-end.md` (PR #134) + +## Problem + +Duration probes in compose / TTS / validate / local timestamps ran ffprobe +without checking ``returncode``, then ``float(stdout)``. A failed probe +that still printed a number (or leftover stdout) looked like a real +duration. + +``_compose_image`` treated a missing duration as ``-t ""`` and stripped +it, so a looping still was muxed **without** a finite ``-t``. + +Manim/simple compose already SKIPs when both probes fail (CLI / pipeline +still fail if composed < mapped). That SKIP stays. + +## Goal + +``returncode != 0`` is a failed probe (``None``, or ``AlignmentError`` +for local timestamps). Image compose raises ``ComposeError`` instead of +dropping ``-t``. + +## Done when + +- [x] Compose / TTS / validate probes ignore stdout when ffprobe fails +- [x] Image compose does not run ffmpeg without a duration +- [x] Local ``probe_duration`` raises on nonzero ffprobe +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` (770 passed, 1 skipped) +- [x] `docgen benchmark` (no clock change; meets baseline) + +## Out of scope + +- Manim/simple compose SKIP when probes fail (CLI still fails if short) +- Validate ``timing_sync`` still skips as passed when duration is unknown diff --git a/milestones/timing-start-end.md b/milestones/timing-start-end.md index c1a023a..06d5fbb 100644 --- a/milestones/timing-start-end.md +++ b/milestones/timing-start-end.md @@ -1,6 +1,6 @@ # Milestone: timing.json word/segment start and end must be numbers -**Status:** Active +**Status:** Shipped **PR:** [#134](https://github.com/jmjava/documentation-generator/pull/134) **Depends on:** `milestones/timing-inner-lists.md` (PR #127), `milestones/bootstrap-timing-helpers.md` (PR #130), diff --git a/src/docgen/align.py b/src/docgen/align.py index fcfee06..507e973 100644 --- a/src/docgen/align.py +++ b/src/docgen/align.py @@ -94,10 +94,19 @@ def probe_duration(audio_path: Path) -> float: "-of", "csv=p=0", str(audio_path)], capture_output=True, text=True, timeout=30, ) - return float(out.stdout.strip()) except FileNotFoundError as exc: raise AlignmentError("ffprobe not found in PATH (required for local timing)") from exc - except (ValueError, subprocess.TimeoutExpired) as exc: + except subprocess.TimeoutExpired as exc: + raise AlignmentError(f"cannot probe duration of {audio_path}: {exc}") from exc + if out.returncode != 0: + detail = (out.stderr or out.stdout or "").strip()[:200] + extra = f": {detail}" if detail else "" + raise AlignmentError( + f"ffprobe failed on {audio_path} (exit {out.returncode}){extra}" + ) + try: + return float(out.stdout.strip()) + except ValueError as exc: raise AlignmentError(f"cannot probe duration of {audio_path}: {exc}") from exc diff --git a/src/docgen/compose.py b/src/docgen/compose.py index c8722be..8e929b0 100644 --- a/src/docgen/compose.py +++ b/src/docgen/compose.py @@ -259,6 +259,11 @@ def _compose_image(self, seg_id: str, relpath: str) -> bool: out.parent.mkdir(parents=True, exist_ok=True) audio_dur = self._probe_duration(audio) + if audio_dur is None or audio_dur <= 0: + raise ComposeError( + f"cannot probe audio duration for {audio.name} — " + "looping image mux needs a finite -t (check ffprobe / the mp3)" + ) cmd = [ "ffmpeg", "-y", "-loop", "1", "-framerate", "30", "-i", str(img), @@ -267,13 +272,12 @@ def _compose_image(self, seg_id: str, relpath: str) -> bool: "pad=1280:720:(ow-iw)/2:(oh-ih)/2,format=yuv420p", "-c:v", "libx264", "-preset", "fast", "-crf", "23", "-c:a", "aac", "-b:a", "128k", - "-t", f"{audio_dur:.3f}" if audio_dur else "", + "-t", f"{audio_dur:.3f}", "-movflags", "+faststart", str(out), ] - cmd = [c for c in cmd if c] self._run_ffmpeg(cmd) - print(f" ok image {img.name} + audio={audio_dur:.1f}s" if audio_dur else " ok image") + print(f" ok image {img.name} + audio={audio_dur:.1f}s") return True def _find_audio(self, seg_id: str) -> Path | None: @@ -317,8 +321,13 @@ def _probe_duration(path: Path) -> float | None: "-of", "csv=p=0", str(path)], capture_output=True, text=True, timeout=30, ) + except (subprocess.TimeoutExpired, FileNotFoundError): + return None + if out.returncode != 0: + return None + try: return float(out.stdout.strip()) - except (ValueError, subprocess.TimeoutExpired, FileNotFoundError): + except ValueError: return None def _run_ffmpeg(self, cmd: list[str]) -> None: diff --git a/src/docgen/tts.py b/src/docgen/tts.py index 1227d38..6c8cf72 100644 --- a/src/docgen/tts.py +++ b/src/docgen/tts.py @@ -23,8 +23,13 @@ def _probe_duration(path: Path) -> float | None: "-of", "csv=p=0", str(path)], capture_output=True, text=True, timeout=30, ) + except (subprocess.TimeoutExpired, FileNotFoundError): + return None + if out.returncode != 0: + return None + try: return float(out.stdout.strip()) - except (ValueError, subprocess.TimeoutExpired, FileNotFoundError): + except ValueError: return None diff --git a/src/docgen/validate.py b/src/docgen/validate.py index 8c5f90e..f3d8380 100644 --- a/src/docgen/validate.py +++ b/src/docgen/validate.py @@ -987,8 +987,13 @@ def _probe_media_duration(path: Path) -> float | None: "-of", "csv=p=0", str(path)], capture_output=True, text=True, timeout=30, ) + except (subprocess.TimeoutExpired, FileNotFoundError): + return None + if out.returncode != 0: + return None + try: return float(out.stdout.strip()) - except (ValueError, subprocess.TimeoutExpired, FileNotFoundError): + except ValueError: return None # ── Helpers ──────────────────────────────────────────────────────── diff --git a/tests/test_align.py b/tests/test_align.py index 6330c1a..f52f979 100644 --- a/tests/test_align.py +++ b/tests/test_align.py @@ -130,3 +130,18 @@ def test_build_local_timing_uses_reconcile_for_near_miss_counts(self) -> None: assert timing["segments"][0]["start"] == pytest.approx(0.0) assert timing["segments"][0]["end"] == pytest.approx(3.5) assert timing["segments"][1]["start"] == pytest.approx(4.0) + + +def test_probe_duration_rejects_nonzero_ffprobe_exit(monkeypatch) -> None: + from pathlib import Path + + from docgen.align import AlignmentError, probe_duration + + class _Proc: + returncode = 1 + stdout = "9.0\n" + stderr = "ffprobe: Invalid data" + + monkeypatch.setattr("docgen.align.subprocess.run", lambda *_a, **_k: _Proc()) + with pytest.raises(AlignmentError, match="ffprobe failed"): + probe_duration(Path("/tmp/x.mp3")) diff --git a/tests/test_compose.py b/tests/test_compose.py index 5243cfc..db01ac0 100644 --- a/tests/test_compose.py +++ b/tests/test_compose.py @@ -311,3 +311,48 @@ def fake_run(cmd, **_kwargs): with pytest.raises(ComposeError, match="ffmpeg timed out after 2s"): composer._run_ffmpeg(["ffmpeg", "-y", str(missing)]) assert not missing.exists() + + +def test_compose_image_raises_when_audio_duration_unknown(tmp_path: Path, monkeypatch) -> None: + cfg = { + "dirs": {"animations": "animations", "audio": "audio", "recordings": "recordings"}, + "segments": {"default": ["01"], "all": ["01"]}, + "segment_names": {"01": "01-demo"}, + "visual_map": {"01": {"type": "image", "source": "images/slide.png"}}, + } + c = _write_cfg(tmp_path, cfg) + (tmp_path / "audio").mkdir(parents=True, exist_ok=True) + (tmp_path / "images").mkdir(parents=True, exist_ok=True) + (tmp_path / "audio" / "01-demo.mp3").write_bytes(b"fake-mp3") + (tmp_path / "images" / "slide.png").write_bytes(b"fake-png") + (tmp_path / "recordings").mkdir(parents=True, exist_ok=True) + + composer = Composer(c) + monkeypatch.setattr(composer, "_probe_duration", lambda _p: None) + ran = {"n": 0} + + def boom(_cmd): + ran["n"] += 1 + + monkeypatch.setattr(composer, "_run_ffmpeg", boom) + with pytest.raises(ComposeError, match="cannot probe audio duration"): + composer._compose_image("01", "images/slide.png") + assert ran["n"] == 0 + + +def test_probe_duration_ignores_stdout_when_ffprobe_fails(tmp_path: Path, monkeypatch) -> None: + cfg = { + "dirs": {"animations": "animations", "audio": "audio", "recordings": "recordings"}, + "segments": {"default": ["01"], "all": ["01"]}, + "visual_map": {"01": {"type": "manim", "source": "Scene01.mp4"}}, + } + c = _write_cfg(tmp_path, cfg) + composer = Composer(c) + + class _Proc: + returncode = 1 + stdout = "12.345\n" + stderr = "error" + + monkeypatch.setattr(subprocess, "run", lambda *_a, **_k: _Proc()) + assert composer._probe_duration(tmp_path / "missing.mp3") is None diff --git a/tests/test_tts.py b/tests/test_tts.py index 32980bc..401ff4b 100644 --- a/tests/test_tts.py +++ b/tests/test_tts.py @@ -162,6 +162,13 @@ def test_probe_duration_returns_none_for_missing_file(tmp_path): @patch("docgen.tts.subprocess.run") def test_probe_duration_returns_float(mock_run): - mock_run.return_value = type("R", (), {"stdout": "12.345\n"})() + mock_run.return_value = type("R", (), {"stdout": "12.345\n", "returncode": 0})() result = _probe_duration(__import__("pathlib").Path("/tmp/test.mp3")) assert result == 12.345 + + +@patch("docgen.tts.subprocess.run") +def test_probe_duration_returns_none_when_ffprobe_fails(mock_run): + mock_run.return_value = type("R", (), {"stdout": "12.345\n", "returncode": 1})() + result = _probe_duration(__import__("pathlib").Path("/tmp/test.mp3")) + assert result is None