diff --git a/SKILL.md b/SKILL.md index 024f351..93247cf 100644 --- a/SKILL.md +++ b/SKILL.md @@ -261,6 +261,8 @@ re-encoded to `-b:a 256k`: that added ~128 kbps for no audible gain, ~+10% file - **Never build a verification harness on a stream-copy cut.** `-ss X -c copy` on an H.264 file with B-frames loses packets (measured: a 60 s cut of a 25 fps source yielded 1270 frames instead of 1500, with the content shifted after each dropped packet), so an encode of that cut can never line up with anything and the comparison reports a fake SSIM 0.25 minimum. Compare whole files (or byte-copies of them): a byte copy is faithful, a stream copy is not. Corollary - before believing any picture verdict, make the harness prove itself on a file against ITSELF (must be lo=1.0). - **Time-box a single-file mystery.** When one file resists diagnosis after a couple of experiments, park it (leave it untouched, log it as 'gate could not verify'), report it, and move on - a library run must not stall on one item. Revisit parked files in a batch later. +- **Killing a background session may not kill the process.** The session is a wrapper around the command; killing the session left the real python alive holding its lock, so the next start was refused by a lock that was telling the TRUTH while the log looked like a stale-lock bug. Check the real PID before concluding (`powershell -NoProfile -Command "Get-CimInstance Win32_Process -Filter 'ProcessId=' | Select ProcessId,ParentProcessId,CreationDate,CommandLine"`) and kill that PID. Corollary, now implemented in `Lock`: the lock file is touched by a heartbeat every minute and staleness is decided by age (5 min) as well as pid, and a takeover that cannot remove a file still held by the OS overwrites it in place instead of crashing. + ## Pitfalls - **A RaiDrive/Dokan write can return with NO error while the file is a fraction of what it should be.** Measured: a 403 MB upload left 12.5 MB on the share, and RaiDrive's own "the operation didn't complete within specific time limit" toast arrived *after* the write had already returned success. Never trust a write to a Dokan-backed share: hash the local file, hash the share copy back, compare size AND SHA-256, and only then rename. On a mismatch, retry the upload (it usually works on the second attempt) rather than failing the item. Uploads should use small chunks (4 MB) with a periodic `flush`+`fsync`; a partial `*.new.mp4`/`*.part.mp4` must be deleted, never resumed from `os.path.getsize()` on the share (RaiDrive reports stale sizes, so a resume offset taken from it corrupts the file). diff --git a/scripts/library_run.py b/scripts/library_run.py index 31fc1b1..1352eac 100644 --- a/scripts/library_run.py +++ b/scripts/library_run.py @@ -275,19 +275,25 @@ def ssim_vs_source(src_local, out_local, info, workdir, tag, dur=SSIM_WINDOW): ref = os.path.join(workdir, "ssim_ref.yuv") out = os.path.join(workdir, "ssim_out.yuv") counts = [] - # Seek a little BEFORE the window and cut it with the trim filter on ABSOLUTE timestamps - # (`-copyts`): an input seek snaps to a keyframe, and it snapped to a *different* keyframe for - # the source and the encode (measured: two 1500-frame decodes whose start times differed by - # ~10 s -> SSIM mean 0.45 on a perfectly good encode). Trimming on PTS makes both sides cover - # exactly the same interval no matter where the seek landed. - seek = max(0.0, start - 20.0) + # Decode WITHOUT an input seek, and cut the window with the trim filter on absolute PTS. + # + # Two traps live here, both measured on real files in this library: + # 1. An input seek snaps to a keyframe, and it snaps to a DIFFERENT keyframe for the source + # and for the encode (two 1500-frame decodes whose start times differed by ~10 s), so the + # frames met the wrong partners and a good encode scored mean 0.45. + # 2. Worse, on an H.264 source with open GOPs an input seek decodes the frames after the + # landing point differently from a sequential decode: the SAME file compared with itself, + # once seeked and once decoded in order, scored min 0.25 / mean 0.82. A seek-based + # reference therefore condemns perfectly good encodes. + # Sequential decoding costs a pass over the file up to the window at 480x270 - small next to the + # encode it is judging, and it is the only decode that is guaranteed to match what the encoder + # itself saw. vf = ("trim=start=%.3f:end=%.3f,setpts=PTS-STARTPTS,scale=%s:flags=bilinear,format=yuv420p" % (start, start + dur, cmp_scale)) for path, dstp in ((src_local, ref), (out_local, out)): - r = run([FFMPEG, "-y", "-v", "error", "-copyts", "-ss", "%.2f" % seek, - "-t", "%.2f" % (dur + 40), "-i", path, + r = run([FFMPEG, "-y", "-v", "error", "-i", path, "-map", "0:v:0", "-an", "-vf", vf, - "-fps_mode", "passthrough", "-f", "rawvideo", dstp], timeout=3600) + "-fps_mode", "passthrough", "-f", "rawvideo", dstp], timeout=7200) if r.returncode != 0 or not os.path.exists(dstp) or not os.path.getsize(dstp): return None, None, 0, None counts.append(os.path.getsize(dstp) // (480 * 270 * 3 // 2)) diff --git a/scripts/transcode.py b/scripts/transcode.py index 42bcf58..011377c 100644 --- a/scripts/transcode.py +++ b/scripts/transcode.py @@ -30,6 +30,7 @@ import os import shutil import subprocess import sys +import threading import time FFMPEG = os.environ.get("TRANSCODE_FFMPEG", "ffmpeg") @@ -308,11 +309,29 @@ def out_path(src, args): class Lock: """One encoder at a time. Two concurrent NVENC jobs starve each other and drop frames - - the exact defect this tool exists to prevent - so cron runs must never overlap.""" + the exact defect this tool exists to prevent - so cron runs must never overlap. + + A lock carries a HEARTBEAT: a daemon thread touches the file every minute, so a live run's lock + is always fresh. Staleness is therefore decided by age first and pid second - killing a run + leaves its lock behind and the recorded pid can be REUSED by an unrelated process, which made a + pid-only check refuse to start a perfectly fine run (measured: a stale lock naming pid 29848, + which by then belonged to something else). + """ + + HEARTBEAT_S = 60 + STALE_S = 300 def __init__(self, path): self.path = path self.fd = None + self._stop = None + + def _beat(self): + while not self._stop.wait(self.HEARTBEAT_S): + try: + os.utime(self.path, None) + except OSError: + return def __enter__(self): os.makedirs(os.path.dirname(os.path.abspath(self.path)) or ".", exist_ok=True) @@ -324,17 +343,28 @@ class Lock: pid = int(open(self.path).read().strip() or 0) except Exception: pid = 0 - if pid and _alive(pid): - raise SystemExit("another transcode is already running (pid %d, %s) - refusing " - "to start a second encoder" % (pid, self.path)) - log("stale lock (pid %s gone) - taking it over" % pid) - os.remove(self.path) - self.fd = os.open(self.path, os.O_CREAT | os.O_EXCL | os.O_WRONLY) + age = time.time() - os.path.getmtime(self.path) + if age < self.STALE_S and pid and _alive(pid): + raise SystemExit("another transcode is already running (pid %d, %s, %.0f s old) - " + "refusing to start a second encoder" % (pid, self.path, age)) + log("stale lock (pid %s, %.0f s old) - taking it over" % (pid, age)) + try: + os.remove(self.path) + except OSError: + # A dead process can leave its handle on the file for a while (the remove fails + # with 'Device or resource busy' on Windows). Overwrite in place instead of + # crashing: the content is ours from here and the heartbeat keeps it fresh. + log("could not remove the stale lock file - overwriting it in place") + self.fd = os.open(self.path, os.O_CREAT | os.O_WRONLY | os.O_TRUNC) os.write(self.fd, str(os.getpid()).encode()) + self._stop = threading.Event() + threading.Thread(target=self._beat, daemon=True).start() return self def __exit__(self, *a): try: + if self._stop is not None: + self._stop.set() if self.fd is not None: os.close(self.fd) os.remove(self.path)