From 9b202fec2dd3ec22d02cbd6ea854bf7ec101610f Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Mon, 13 Jul 2026 09:19:01 +0300 Subject: [PATCH] Preview stability round 4: never serve mid-write video, signature-keyed remux/segment caches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three holes left after the /result faststart remux (e296fe0), matching the still-live symptoms (decode errors in console, blinking clips, playback restarting from 0, stale previews): 1. Mid-write serving: /result hit while VHS is still writing the mp4 found no moov, failed the remux, and served the raw moov-less bytes anyway — undecodable, and the player's cache-busted retries kept blinking against it. 2. Byte-switching under one URL: once the write finished, the same URL switched from raw to remuxed bytes with different offsets/length. A browser mid-Range stream mixes offsets (PIPELINE_ERROR_DECODE) or fails If-Range revalidation and falls back to a full 200 — resetting the element and restarting the clip from the beginning. 3. Cache collisions/staleness: the remux cache was keyed by path only (a rewritten source kept serving the old remux) and the remux output was named by basename only (two sources sharing a basename overwrote each other's remuxed copy with -y, possibly while it was being streamed). Fix: _is_faststart becomes _moov_position (front/end/none) so "remux me" (moov at end = complete VHS file) is distinguished from "come back later" (no moov = still being written or aborted save). No-moov sources now get 503 + Retry-After from both /result and preview-segment instead of undecodable bytes; the player retry loop already handles that. The remux cache is keyed on the source's (mtime_ns, size) signature, the output name is derived from path+signature (no basename collisions), the remuxed file is validated (nonzero, moov at front) before caching, and a stable-but-unremuxable file is remembered so ffmpeg isn't re-run per request. The preview-segment cache key also gains the source signature so a re-rendered source at the same path can't serve segments encoded from the old bytes. Verified against real ffmpeg files: end/front/none discrimination on complete, faststart, truncated-mid-write, and empty mp4s; remux of a truncated file raises instead of caching garbage. All 160 movie_editor tests pass. Co-Authored-By: Claude Fable 5 --- movie_editor/server.py | 123 ++++++++++++++++++++++++++++++++--------- 1 file changed, 96 insertions(+), 27 deletions(-) diff --git a/movie_editor/server.py b/movie_editor/server.py index 71f0a3d9..2171c3d8 100644 --- a/movie_editor/server.py +++ b/movie_editor/server.py @@ -209,17 +209,19 @@ def _ffmpeg_trim_clip(src_path: str, out_path: str, inn, dur, fast: bool = False raise RuntimeError((proc.stderr or "ffmpeg failed")[-1000:]) -def _is_faststart(path: str) -> bool: - """True if the MP4's moov atom comes before mdat (so browsers can start decoding from a - progressive/Range fetch without seeking to the tail first). ComfyUI's own VHS_VideoCombine - output has no +faststart, so its moov sits at the end — deep-seeking that raw file is what - the preview-segment path already routes around; this lets /result fix it for everyone else - (scrubbing a "shallow" clip, or just opening one) without needing a segment at all.""" +def _moov_position(path: str) -> str: + """Where the MP4's moov atom sits: "front" (before mdat — browsers can decode from a + progressive/Range fetch), "end" (after mdat — ComfyUI's own VHS_VideoCombine output; a + complete file, but deep-seeking it raw is undecodable in the browser), or "none" (no moov + anywhere — the file is still being written, or a save was aborted; ffmpeg can't read it + either, so it must never be served as video). Distinguishing "end" from "none" is what + lets /result tell "remux me" apart from "come back later".""" import os try: with open(path, "rb") as f: size = os.fstat(f.fileno()).st_size offset = 0 + seen_mdat = False while offset + 8 <= size: f.seek(offset) header = f.read(8) @@ -228,22 +230,31 @@ def _is_faststart(path: str) -> bool: box_size = int.from_bytes(header[:4], "big") box_type = header[4:8] if box_type == b"moov": - return True + return "end" if seen_mdat else "front" if box_type == b"mdat": - return False + seen_mdat = True if box_size == 1: ext = f.read(8) if len(ext) < 8: break box_size = int.from_bytes(ext, "big") elif box_size == 0: - return False # box runs to EOF (typically mdat) without a moov seen yet + # Box runs to EOF without a size (mdat still being streamed to disk — + # ffmpeg backfills the size on finalize): nothing after it, no moov. + return "none" if box_size < 8: break offset += box_size except OSError: - return False - return False + pass + return "none" + + +def _file_sig(path: str) -> tuple: + """Change detector for cache keys: same (mtime_ns, size) ⇒ same bytes for our purposes.""" + import os + st = os.stat(path) + return (st.st_mtime_ns, st.st_size) def _remux_faststart(src_path: str, out_path: str) -> None: @@ -331,7 +342,8 @@ def _scene_playback_clip_spec( _preview_segment_cache: dict[str, str] = {} _preview_segment_locks: dict = {} # cache key -> asyncio.Lock (dedupe concurrent encodes) -_faststart_cache: dict[str, str] = {} # source path -> remuxed (faststart) path +_faststart_cache: dict[str, tuple] = {} # source path -> (source sig, remuxed faststart path) +_faststart_raw_ok: dict[str, tuple] = {} # source path -> sig a stable file failed to remux at (serve raw, don't re-run ffmpeg per request) _faststart_locks: dict = {} # source path -> asyncio.Lock (dedupe concurrent remuxes) @@ -1811,37 +1823,80 @@ if web is not None and PromptServer is not None: anything that already has moov up front, e.g. FunPack's own segment/export/final-render output, which already sets +faststart itself. MP4/MOV only (moov/mdat is an ISO-BMFF concept — a non-.mp4 container falling through the caller's extension gate would just - get echoed back as-is here, not silently mis-remuxed into a mismatched container).""" + get echoed back as-is here, not silently mis-remuxed into a mismatched container). + + A file with NO moov (still being written, or an aborted save) raises 503 instead of + being served: raw moov-less bytes are undecodable, and once the write finishes the + same URL would start serving different bytes — a browser mid-Range-stream then mixes + offsets (PIPELINE decode errors) or fails If-Range and restarts the clip from 0. + Caches are keyed on the source's (mtime, size) signature and the remux output name is + derived from path+signature, so a rewritten source can't serve a stale copy and two + sources sharing a basename can't overwrite each other's remux mid-stream.""" import asyncio + import hashlib import os if os.path.splitext(src_path)[1].lower() != ".mp4": return src_path + try: + sig = _file_sig(src_path) + except OSError: + return src_path # caller re-checks existence / falls through to 404 cached = _faststart_cache.get(src_path) - if cached and os.path.isfile(cached): - return cached - if _is_faststart(src_path): - return src_path + if cached and cached[0] == sig and os.path.isfile(cached[1]): + return cached[1] lock = _faststart_locks.setdefault(src_path, asyncio.Lock()) async with lock: + try: + sig = _file_sig(src_path) + except OSError: + return src_path cached = _faststart_cache.get(src_path) - if cached and os.path.isfile(cached): - return cached + if cached and cached[0] == sig and os.path.isfile(cached[1]): + return cached[1] + pos = _moov_position(src_path) + if pos == "front": + return src_path + if pos == "none": + # Mid-write (or truncated junk): never serve — undecodable, and the bytes + # will change under this URL. 503 lets the player's retry loop try again. + raise web.HTTPServiceUnavailable( + reason="Video file is still being written.", headers={"Retry-After": "2"}) + if _faststart_raw_ok.get(src_path) == sig: + return src_path try: import folder_paths tempdir = folder_paths.get_temp_directory() except Exception: return src_path - base = os.path.splitext(os.path.basename(src_path))[0] - out_path = os.path.join(tempdir, f"funpack_faststart_{base}.mp4") + digest = hashlib.md5(f"{src_path}|{sig[0]}|{sig[1]}".encode()).hexdigest()[:16] + out_path = os.path.join(tempdir, f"funpack_faststart_{digest}.mp4") + err = None try: # to_thread: same event-loop-freeze hazard as preview-segment encodes below — # this is a stream copy (no re-encode) so it's fast, but still blocking I/O. await asyncio.to_thread(_remux_faststart, src_path, out_path) + ok = os.path.getsize(out_path) > 0 and _moov_position(out_path) == "front" except Exception as e: # noqa: BLE001 - print(f"[FunPack] faststart remux failed for {src_path}: {e}") - return src_path - _faststart_cache[src_path] = out_path - return out_path + ok = False + err = e + try: + sig_now = _file_sig(src_path) + except OSError: + sig_now = None # deleted under us — retry will 404 definitively + if ok and sig_now == sig: + _faststart_cache[src_path] = (sig, out_path) + return out_path + try: + os.remove(out_path) + except OSError: + pass + if sig_now != sig: + # Source changed under the remux — it's being (re)written right now. + raise web.HTTPServiceUnavailable( + reason="Video file is still being written.", headers={"Retry-After": "2"}) + print(f"[FunPack] faststart remux failed for {src_path}: {err}") + _faststart_raw_ok[src_path] = sig # stable but un-remuxable: serve raw, once + return src_path @routes.get(UI_PREFIX + "/api/projects/{pid}/result") async def _result(req): @@ -1927,7 +1982,22 @@ if web is not None and PromptServer is not None: clip = _ghost_playback_clip_spec(req.query) if clip is None: return web.json_response({"detail": str(e)}, status=404) - key = _preview_segment_cache_key(pid, scene_id, clip) + try: + src_path = _resolve_clip_src_path(clip) + src_sig = _file_sig(src_path) + except FileNotFoundError as e: + return web.json_response({"detail": str(e)}, status=400) + except OSError as e: + return web.json_response({"detail": str(e)}, status=503) + if os.path.splitext(src_path)[1].lower() == ".mp4" and _moov_position(src_path) == "none": + # No moov ⇒ still being written (VHS finalizes it last) or an aborted save: + # ffmpeg can't trim it and the segment would come out truncated/undecodable. + return web.json_response( + {"detail": "Source video is still being written."}, + status=503, headers={"Retry-After": "2"}) + # Source signature in the key: a re-rendered source at the SAME path must not keep + # serving the segment encoded from the old bytes (stale previews). + key = _preview_segment_cache_key(pid, scene_id, clip) + f":{src_sig[0]}:{src_sig[1]}" # Serialize per segment: the pool preloads every clip at once after a run, and a # scrub can re-request the same URL — without this lock each request would start # its own duplicate encode of the same segment. @@ -1947,7 +2017,6 @@ if web is not None and PromptServer is not None: f"funpack_preview_{pid[:8]}_{scene_id}_{int(_time.time() * 1000)}.mp4", ) try: - src_path = _resolve_clip_src_path(clip) # to_thread: this encode takes seconds — run inline it would freeze the ONE # aiohttp event loop ComfyUI has, stalling every playing /result stream and # the whole editor API for its duration (stale/black previews on scrub).