diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index 482602f9..ed671a47 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -25,6 +25,7 @@ import json import logging import math import queue +import shutil import subprocess import threading import time @@ -489,24 +490,57 @@ def build_poster_cmd(src: Path, dest: Path, at_s: float) -> list[str]: ] +def build_thumbnail_jpg_cmd(src: Path, dest: Path) -> list[str]: + """Decode a downloaded thumbnail (webp/png/…) to a single baseline JPEG frame.""" + return [ + "ffmpeg", "-y", "-hide_banner", "-loglevel", "error", "-nostdin", + "-i", str(src), "-frames:v", "1", "-update", "1", str(dest), + ] + + +def _run_ffmpeg_best_effort(cmd: list[str], timeout_s: int) -> Exception | None: + """Run a one-shot ffmpeg command, bounded by `timeout_s`. Returns None on success, or the + exception on a timeout/non-zero-exit/OS error — never raises. Shared by the best-effort image + steps (poster / thumbnail / storyboard) so the run+timeout+swallow policy lives in ONE place; + each caller logs its own context from the returned exception. (The download/transcode path uses + the richer `run_ffmpeg` instead, which streams progress and honours cooperative cancel.)""" + try: + subprocess.run(cmd, capture_output=True, timeout=timeout_s, check=True) + return None + except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError) as exc: + return exc + + +def _is_jpeg(path: Path) -> bool: + """True if the file starts with the JPEG SOI marker (FF D8 FF).""" + try: + with path.open("rb") as f: + return f.read(3) == b"\xff\xd8\xff" + except OSError: + return False + + def thumbnail_to_jpg(src: Path | None, dest: Path, timeout_s: int = 30) -> Path | None: - """Convert a downloaded thumbnail (webp / png / jpeg — whatever the source shipped) into a real + """Turn a downloaded thumbnail (webp / png / jpeg — whatever the source shipped) into a real baseline JPEG for the `.jpg` poster + og:image share sidecar. - Uses a plain ffmpeg decode→mjpeg, deliberately NOT yt-dlp's `FFmpegThumbnailsConvertor` (whose - `-bsf:v mjpeg2jpeg` path crashes on some already-JPEG thumbnails, e.g. Reddit's, and used to - fail the entire download). Best-effort: returns None on any failure, so the caller falls back to - a cut video frame via `ensure_poster` and the download still succeeds.""" + Deliberately NOT yt-dlp's `FFmpegThumbnailsConvertor` (whose `-bsf:v mjpeg2jpeg` path crashes on + some already-JPEG thumbnails, e.g. Reddit's, and used to fail the entire download). A thumbnail + that is already JPEG is copied as-is; anything else is decoded via ffmpeg. Best-effort: returns + None on any failure, so the caller falls back to a cut video frame (`ensure_poster`) and the + download still succeeds.""" if src is None or not src.exists(): return None - try: - subprocess.run( - ["ffmpeg", "-y", "-hide_banner", "-loglevel", "error", "-nostdin", - "-i", str(src), "-frames:v", "1", "-update", "1", str(dest)], - capture_output=True, timeout=timeout_s, check=True, - ) - except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError) as exc: - log.info("thumbnail->jpg conversion failed (%s); poster falls back to a cut frame", str(exc)[:120]) + if _is_jpeg(src): # already JPEG (e.g. Facebook) — no decode/re-encode needed + try: + shutil.copyfile(src, dest) + return dest + except OSError as exc: + log.info("thumbnail copy failed (%s); poster falls back to a cut frame", str(exc)[:120]) + return None + err = _run_ffmpeg_best_effort(build_thumbnail_jpg_cmd(src, dest), timeout_s) + if err is not None: + log.info("thumbnail->jpg conversion failed (%s); poster falls back to a cut frame", str(err)[:120]) return None return dest if dest.exists() else None @@ -528,10 +562,9 @@ def ensure_poster(asset, download_root: str, timeout_s: int = 30) -> str | None: if dest.exists(): return rel at = min(3.0, (asset.duration_s or 10) * 0.1) - try: - subprocess.run(build_poster_cmd(src, dest, at), capture_output=True, timeout=timeout_s, check=True) - except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError) as exc: - log.info("poster gen skipped for asset %s: %s", asset.id, str(exc)[:120]) + err = _run_ffmpeg_best_effort(build_poster_cmd(src, dest, at), timeout_s) + if err is not None: + log.info("poster gen skipped for asset %s: %s", asset.id, str(err)[:120]) return None return rel if dest.exists() else None @@ -560,10 +593,9 @@ def ensure_storyboard(asset, download_root: str, timeout_s: int = 90) -> dict | return None dest.parent.mkdir(parents=True, exist_ok=True) cmd = build_storyboard_cmd(src, dest, asset.duration_s, geom) - try: - subprocess.run(cmd, capture_output=True, timeout=timeout_s, check=True) - except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError) as exc: - log.info("storyboard gen skipped for asset %s: %s", asset.id, str(exc)[:120]) + err = _run_ffmpeg_best_effort(cmd, timeout_s) + if err is not None: + log.info("storyboard gen skipped for asset %s: %s", asset.id, str(err)[:120]) return None if not dest.exists(): return None diff --git a/backend/app/downloads/errors.py b/backend/app/downloads/errors.py index 2f694faa..68947a8a 100644 --- a/backend/app/downloads/errors.py +++ b/backend/app/downloads/errors.py @@ -24,7 +24,6 @@ _RULES: tuple[tuple[str, str], ...] = ( # Some extractors (e.g. Reddit) gate content behind a logged-in account: "Account # authentication is required." — an auth wall we deliberately don't cross, not a transient error. ("authentication is required", "login_required"), - ("account authentication", "login_required"), # Geo-restricted. ("in your country", "geo_blocked"), ("available in your location", "geo_blocked"), diff --git a/backend/app/downloads/formats.py b/backend/app/downloads/formats.py index eb575c44..662c85d9 100644 --- a/backend/app/downloads/formats.py +++ b/backend/app/downloads/formats.py @@ -218,6 +218,9 @@ def build_ydl_opts( # The `.jpg` poster sidecar is instead produced robustly by our own pipeline (worker # `_download` → `editmod.thumbnail_to_jpg`, with `ensure_poster` cutting a video frame as a # fallback), so the og:image share preview (Facebook Messenger / MS Teams) keeps working. + # The opt-in EmbedThumbnail below is unaffected: it does its OWN format handling — it embeds a + # jpg/png thumbnail directly and converts anything else to PNG (not the crashing mjpeg2jpeg-to-jpg + # path), so it never needed the convertor and does not reintroduce the crash. if s["embed_thumbnail"]: opts["postprocessors"].append( {"key": "EmbedThumbnail", "already_have_thumbnail": True} diff --git a/backend/tests/test_titles.py b/backend/tests/test_titles.py index 11eaa68e..5d80bd48 100644 --- a/backend/tests/test_titles.py +++ b/backend/tests/test_titles.py @@ -1,4 +1,9 @@ """normalize_title: de-shout / de-clutter video titles without mangling non-English text.""" +from concurrent.futures import ThreadPoolExecutor +from concurrent.futures import TimeoutError as FuturesTimeout + +import pytest + from app.titles import normalize_title @@ -29,20 +34,26 @@ def test_numeric_episode_marker_is_not_treated_as_a_hashtag(): assert normalize_title("My show #3") == "My show #3" -def test_midstring_hashtags_are_fast_and_kept_redos_regression(): +def test_midstring_hashtags_dont_hang_and_are_kept_redos_regression(): # Facebook prepends an engagement prefix and appends a page name, leaving a long hashtag cluster # in the MIDDLE. That is not a trailing SEO cluster, so the tags stay — and, critically, this # exact shape used to trigger catastrophic backtracking (ReDoS) in `_TRAILING_HASHTAGS` that # pinned a download-worker thread at 100% CPU forever inside the yt-dlp progress hook. - import time - raw = ( "398K views · 6.1K reactions | The money should follow 🙄 #reel #reels #meme #memes " "#fun #funny #work #worklife #office #viral #trending #fyp #foryou #foryoupage | Memebonanza4U" ) - t = time.perf_counter() - out = normalize_title(raw) or "" - assert time.perf_counter() - t < 1.0 # linear now; the old regex ran for minutes / never returned + # Run in a worker thread with a generous ceiling: the fixed regex returns in microseconds, so + # only a genuine catastrophic-backtracking regression fails to complete — a hang is caught + # deterministically rather than by asserting on wall-clock slack in the happy path. + ex = ThreadPoolExecutor(max_workers=1) + fut = ex.submit(normalize_title, raw) + try: + out = fut.result(timeout=5) or "" + except FuturesTimeout: + pytest.fail("normalize_title did not return within 5s — catastrophic backtracking regressed") + finally: + ex.shutdown(wait=False) # never join a (hypothetically) wedged thread assert "#foryoupage" in out and "#reel" in out # mid-string hashtags preserved (not trailing) assert "views" not in out # the engagement-count prefix was still stripped