fix(utils): stabilize segmented video reversal

CHANGELOG.md

@@ -10,6 +10,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 (unreleased)=
 ## [Unreleased](https://github.com/jeertmans/manim-slides/compare/v5.7.0...HEAD)
 
+(unreleased-fixed)=
+### Fixed
+
+- Fixed segmented video reversal stalls and made segment ordering deterministic.
+  [@rusetiq](https://github.com/rusetiq) [#668](https://github.com/jeertmans/manim-slides/pull/668)
+
 (v5.7.0)=
 ## [v5.7.0](https://github.com/jeertmans/manim-slides/compare/v5.6.0...v5.7.0)
 

docs/source/contributing/internals.md

@@ -6,6 +6,40 @@ Manim-Slides' work is split in two steps: first, when rendering animation, and,
 
 To render animations, Manim Slides simply uses Manim or ManimGL, and creates some additional output files that it needs for the presentation.
 
+### Segmented video reversal
+
+Long animations are split into segments and reversed by spawned worker processes.
+Fresh interpreters avoid inheriting locks from renderer or logging threads.
+Direct calls to `reverse_video_file` from a script need an
+`if __name__ == "__main__":` guard, as with other multiprocessing code.
+
+The controlled reproducer for the inherited-lock failure is a three-second slide
+on platforms with `fork`:
+
+```sh
+uv run manim render tests/data/reversal_deadlock.py ReversalDeadlock -ql --disable_caching
+```
+
+It selects `fork` as the application's default and synchronizes a background
+thread so that it holds the renderer's Rich console lock when workers fork.
+PyAV logging in an inherited worker then waits on a lock whose owner thread no
+longer exists. Spawned reversal workers finish even with that application default.
+This forces a concrete unsafe-fork condition; it does not establish that every
+stall reported in issues #562 and #569 has this same cause.
+
+The utility regression runs the same setup on a tiny generated video with one
+and two workers, checks every reversed frame, and kills the entire subprocess
+group after 30 seconds if reversal stalls:
+
+```sh
+uv run pytest tests/test_utils.py -k inherited_renderer_lock
+```
+
+The controlled fork test is skipped on platforms without `fork`. The frame-order
+test exercises real spawned workers on all supported platforms. When comparing
+against the old default-context pool, run the regression above to bound the
+stall and clean up its workers; the standalone scene deliberately has no timeout.
+
 ## Slides presentation
 
 Manim Slides searches for the local artifacts it generated previously, and concatenates them into one presentation. For the graphical interface, it uses `PySide6`.

manim_slides/convert.py

@@ -53,7 +53,7 @@ def open_with_default(file: Path) -> None:
     if system == "Darwin":
         subprocess.call(("open", str(file)))
     elif system == "Windows":
-        os.startfile(str(file))  # nosec # type: ignore
+        os.startfile(str(file))  # nosec # ty: ignore[unresolved-attribute]
     else:
         subprocess.call(("xdg-open", str(file)))
 

manim_slides/utils.py

@@ -4,7 +4,7 @@
 import tempfile
 from collections.abc import Iterator
 from itertools import pairwise
-from multiprocessing import Pool
+from multiprocessing import get_context
 from pathlib import Path
 from typing import Any, TypeVar, cast
 
@@ -194,7 +194,13 @@ def reverse_video_file(
     num_processes: int | None = None,
     **tqdm_kwargs: Any,
 ) -> None:
-    """Reverses a video file, writing the result to `dest`."""
+    """
+    Reverses a video file, writing the result to `dest`.
+
+    Segmented reversal uses spawned processes to avoid inheriting renderer
+    locks. When calling this directly from a script, guard the entry point with
+    ``if __name__ == "__main__":`` so workers can safely import the main module.
+    """
     with av.open(str(src)) as input_container:  # Fast path if file is short enough
         input_stream = input_container.streams.video[0]
         if max_segment_duration is None:
@@ -213,7 +219,7 @@ def reverse_video_file(
         with tempfile.TemporaryDirectory() as tmpdirname:
             tmpdir = Path(tmpdirname)
             with av.open(
-                str(tmpdir / f"%04d.{src.suffix}"),
+                str(tmpdir / f"%04d{src.suffix}"),
                 "w",
                 format="segment",
                 options={"segment_time": str(max_segment_duration)},
@@ -231,12 +237,15 @@ def reverse_video_file(
                     packet.stream = output_stream
                     output_container.mux(packet)
 
-            src_files = list(tmpdir.iterdir())
+            src_files = sorted(tmpdir.iterdir())
             rev_files = [
                 src_file.with_stem("rev_" + src_file.stem) for src_file in src_files
             ]
 
-            with Pool(num_processes, maxtasksperchild=1) as pool:
+            # Forking after the renderer and PyAV have initialized can inherit
+            # locked native state and stall the worker pool.  Fresh interpreters
+            # avoid that state while preserving parallel segment processing.
+            with get_context("spawn").Pool(num_processes, maxtasksperchild=1) as pool:
                 for _ in tqdm(
                     pool.imap_unordered(
                         reverse_video_file_in_one_chunk,

tests/data/reversal_deadlock.py

@@ -0,0 +1,88 @@
+"""
+Force the inherited renderer-lock failure while reversing real video segments.
+
+Run in a subprocess: this deliberately leaves fork workers blocked. The caller
+must enforce a timeout and terminate the process group when testing old code.
+"""
+
+import logging
+import multiprocessing
+import os
+import sys
+from pathlib import Path
+from threading import Event, Thread
+
+import av
+from manim import LEFT, RIGHT, Dot
+from rich.logging import RichHandler
+
+from manim_slides.logger import logger
+from manim_slides.slide.manim import Slide
+from manim_slides.utils import reverse_video_file
+
+
+def install_inherited_renderer_lock() -> None:
+    # Model Linux/Python <= 3.13 even when the host defaults to spawn/forkserver.
+    multiprocessing.set_start_method("fork", force=True)
+    handler = next(h for h in logger.handlers if isinstance(h, RichHandler))
+    console = handler.console
+    libav_logger = logging.getLogger("libav")
+    libav_logger.handlers = [handler]
+    libav_logger.setLevel(logging.INFO)
+    libav_logger.propagate = False
+    av.logging.set_level(av.logging.INFO)
+
+    ready = Event()
+    release = Event()
+    threads: list[Thread] = []
+
+    def hold_renderer_lock() -> None:
+        with console._lock:
+            ready.set()
+            release.wait()
+
+    def before_fork() -> None:
+        ready.clear()
+        release.clear()
+        thread = Thread(target=hold_renderer_lock, daemon=True)
+        threads.append(thread)
+        thread.start()
+        ready.wait()
+
+    def after_fork_parent() -> None:
+        release.set()
+        threads.pop().join()
+
+    # Acquire only at fork time so the parent's segmentation can finish. The
+    # parent releases its copy afterward; the child's owner thread is gone.
+    # No reversal worker or PyAV operation is replaced by a test double.
+    os.register_at_fork(before=before_fork, after_in_parent=after_fork_parent)
+
+
+class ReversalDeadlock(Slide):
+    """A small slide that forces the inherited-lock condition from the reports."""
+
+    max_duration_before_split_reverse = 1.0
+    num_processes = 2
+    disable_caching = True
+
+    def construct(self) -> None:
+        install_inherited_renderer_lock()
+        dot = Dot().shift(LEFT)
+        self.add(dot)
+        # Separate animations supply keyframes at each segment boundary.
+        for direction in (RIGHT, LEFT, RIGHT):
+            self.play(dot.animate.shift(2 * direction), run_time=1)
+        self.next_slide()
+
+
+def reproduce(src: Path, dest: Path, num_processes: int) -> None:
+    install_inherited_renderer_lock()
+    reverse_video_file(
+        src, dest, max_segment_duration=1.0, num_processes=num_processes, disable=True
+    )
+    assert multiprocessing.get_start_method() == "fork"
+
+
+if __name__ == "__main__":
+    reproduce(Path(sys.argv[1]), Path(sys.argv[2]), int(sys.argv[3]))

tests/test_utils.py

@@ -1,12 +1,23 @@
+import multiprocessing
+import os
 import shutil
+import signal
+import subprocess
+import sys
+from collections.abc import Iterator
 from pathlib import Path
 
 import av
+import numpy as np
 import pytest
 from click.testing import CliRunner
 
 from manim_slides.__main__ import cli
-from manim_slides.utils import concatenate_video_files, merge_basenames
+from manim_slides.utils import (
+    concatenate_video_files,
+    merge_basenames,
+    reverse_video_file,
+)
 
 
 def test_merge_basenames(paths: list[Path]) -> None:
@@ -79,3 +90,100 @@ def test_merge_basenames_short_hash() -> None:
     # Stem should be at most 16 hex characters (plus extension)
     assert len(merged.stem) == 16
     assert all(c in "0123456789abcdef" for c in merged.stem)
+
+
+@pytest.fixture
+def reversal_video(tmp_path: Path) -> Path:
+    src = tmp_path / "src.mp4"
+
+    with av.open(str(src), mode="w") as container:
+        stream = container.add_stream("libx264", rate=2)
+        stream.width = 16
+        stream.height = 16
+        stream.pix_fmt = "yuv420p"
+        stream.codec_context.gop_size = 1
+
+        for value in range(6):
+            array = np.full((16, 16, 3), value * 40, dtype=np.uint8)
+            frame = av.VideoFrame.from_ndarray(array, format="rgb24")
+            for packet in stream.encode(frame):
+                container.mux(packet)
+
+        for packet in stream.encode():
+            container.mux(packet)
+
+    return src
+
+
+def assert_reversed_frames(src: Path, dest: Path) -> None:
+    def values(path: Path) -> list[float]:
+        with av.open(str(path)) as container:
+            return [
+                float(frame.to_ndarray(format="gray").mean())
+                for frame in container.decode(video=0)
+            ]
+
+    expected = values(src)[::-1]
+    assert len(expected) == 6
+    # Re-encoding is lossy. Check every frame, including count and brightness,
+    # so an empty, constant, or incomplete output cannot satisfy the test.
+    np.testing.assert_allclose(values(dest), expected, atol=3)
+
+
+@pytest.mark.parametrize("num_processes", [1, 2])
+def test_reverse_video_file_with_unordered_segments(
+    reversal_video: Path, monkeypatch: pytest.MonkeyPatch, num_processes: int
+) -> None:
+    dest = reversal_video.with_name("dest.mp4")
+
+    original_iterdir = Path.iterdir
+
+    def reversed_iterdir(path: Path) -> Iterator[Path]:
+        return iter(reversed(list(original_iterdir(path))))
+
+    with monkeypatch.context() as patch:
+        patch.setattr(Path, "iterdir", reversed_iterdir)
+        reverse_video_file(
+            reversal_video,
+            dest,
+            max_segment_duration=1.0,
+            num_processes=num_processes,
+            disable=True,
+        )
+
+    assert_reversed_frames(reversal_video, dest)
+
+
+@pytest.mark.skipif(
+    "fork" not in multiprocessing.get_all_start_methods(), reason="requires fork"
+)
+@pytest.mark.parametrize("num_processes", [1, 2])
+def test_reverse_video_file_with_inherited_renderer_lock(
+    reversal_video: Path, data_folder: Path, num_processes: int
+) -> None:
+    dest = reversal_video.with_name("dest.mp4")
+    with subprocess.Popen(
+        [
+            sys.executable,
+            str(data_folder / "reversal_deadlock.py"),
+            str(reversal_video),
+            str(dest),
+            str(num_processes),
+        ],
+        stdout=subprocess.PIPE,
+        stderr=subprocess.STDOUT,
+        text=True,
+        start_new_session=True,
+    ) as process:
+        try:
+            output, _ = process.communicate(timeout=30)
+        except subprocess.TimeoutExpired:
+            # Kill the whole pool, not just the parent, so a regression cannot
+            # leave hung workers behind or prevent the remaining tests running.
+            os.killpg(process.pid, signal.SIGKILL)
+            output, _ = process.communicate()
+            pytest.fail(f"Segmented reversal stalled with an inherited lock:\n{output}")
+
+        assert process.returncode == 0, output
+
+    assert_reversed_frames(reversal_video, dest)