From 5561d9a3792013e333c0243add9016472120f0f6 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 5 Oct 2026 03:16:27 +0200 Subject: [PATCH] =?UTF-8?q?feat(downloads):=20every=20ready=20file=20is=20?= =?UTF-8?q?phone-safe=20=E2=80=94=20uploads=20and=20clips=20too?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A share link is opened on whatever the friend holds. Measured on an iPhone and an iPad: AV1, Matroska, HEVC tagged hev1 and MP3 inside an .mp4 fail there; Chrome and Firefox add HEVC and 10-bit / 4:4:4 H.264. The old check judged the codec NAME only and ran on downloads only, so a 10-bit H.264 passed, an upload went out exactly as it came (an iPhone's HEVC .mov, a webm, an mkv) and a fast trim kept its source's codecs. One rule now, `downloads/playable.py`: video is H.264 8-bit 4:2:0 + AAC in .mp4, audio-only is .mp3 or AAC .m4a; `plan` says what must change, `build_cmd` re-encodes only the failing stream (or remuxes). The worker applies it to a download, an editor clip and — new — an upload: media that fails the rule is queued (asset pending, job queued) and the worker converts it, keeping only the converted file (owner's choice). Until then the original sits at rel_path, so Retry starts from it and a delete removes it; an upload that lost its last holder mid-conversion is reaped by the GC. Every libx264 run (edit, concat, conversion) now pins `-pix_fmt yuv420p` — without it a 10-bit source came out High 10. The download cache signature moves to v5 so a cached 10-bit asset re-derives. The upload toast says when the file is still converting (HU/EN). Tests: the plan for every measured class; real ffmpeg turning a 10-bit Matroska + Opus and an mp3-in-mp4 into files the rule accepts (red without the pixel-format pin); the queued upload's delete, cancel-then-GC, failed conversion and real worker conversion (the cleanup tests red without their rules); the toast (red without the branch). Live on localdev: 9 fixture uploads converted, every served file probed H.264 High yuv420p + AAC, or mp3 / m4a. --- backend/app/downloads/edit.py | 117 ++---------- backend/app/downloads/formats.py | 19 +- backend/app/downloads/gc.py | 23 +++ backend/app/downloads/pipeline.py | 171 +++++++++++++----- backend/app/downloads/playable.py | 99 ++++++++++ backend/app/downloads/probe.py | 63 +++++-- backend/app/downloads/runner.py | 4 +- backend/app/downloads/service.py | 12 +- backend/app/downloads/upload.py | 36 ++-- backend/app/routes/downloads.py | 3 +- backend/tests/test_download_deprioritize.py | 14 +- backend/tests/test_download_output.py | 45 +---- backend/tests/test_formats.py | 17 -- backend/tests/test_playable.py | 157 ++++++++++++++++ backend/tests/test_public_unlock_limit.py | 5 +- backend/tests/test_upload.py | 157 +++++++++++++++- .../src/components/UploadDialog.dom.test.tsx | 18 +- frontend/src/components/UploadDialog.tsx | 13 +- frontend/src/i18n/locales/en/downloads.json | 1 + frontend/src/i18n/locales/hu/downloads.json | 1 + frontend/src/lib/api/downloads.ts | 3 +- 21 files changed, 712 insertions(+), 266 deletions(-) create mode 100644 backend/app/downloads/playable.py create mode 100644 backend/tests/test_playable.py diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index d191d14a..38c90f5d 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -188,6 +188,13 @@ def clip_duration(spec: dict, source_duration: int | None) -> int | None: # --- ffmpeg: trim / crop -------------------------------------------------------------------- +# Every libx264 / aac re-encode in this package uses these, so a re-encoded file is always the +# phone-safe shape (`playable`): `-pix_fmt yuv420p` pins 8-bit 4:2:0 — without it libx264 keeps a +# 10-bit or 4:4:4 source's pixel format and writes H.264 High 10 / High 4:4:4, which browsers decode +# only patchily (the one safe H.264 is 8-bit 4:2:0). +X264_ARGS = ["-c:v", "libx264", "-preset", "veryfast", "-crf", "20", "-pix_fmt", "yuv420p"] +AAC_ARGS = ["-c:a", "aac", "-b:a", "192k"] + def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str]: """ffmpeg command for a trim/crop edit. `-progress pipe:1` streams machine-readable progress. @@ -225,18 +232,7 @@ def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str] # the exact cut). A crop adds the filter; an accurate trim re-encodes with no filter. if crop: cmd += ["-vf", f"crop={crop['w']}:{crop['h']}:{crop['x']}:{crop['y']}"] - cmd += [ - "-c:v", - "libx264", - "-preset", - "veryfast", - "-crf", - "20", - "-c:a", - "aac", - "-b:a", - "192k", - ] + cmd += [*X264_ARGS, *AAC_ARGS] else: cmd += ["-c", "copy", "-avoid_negative_ts", "make_zero"] @@ -297,16 +293,8 @@ def build_concat_plan(src: Path, dest: Path, spec: dict, out_ext: str, staging: "[v]", "-map", "[a]", - "-c:v", - "libx264", - "-preset", - "veryfast", - "-crf", - "20", - "-c:a", - "aac", - "-b:a", - "192k", + *X264_ARGS, + *AAC_ARGS, ] else: listp = staging / "concat.txt" @@ -474,14 +462,7 @@ def run_ffmpeg( raise StatedFailure(f"ffmpeg failed ({ret}): {''.join(err_tail)[-300:]}") -# --- ffmpeg: universal browser playability -------------------------------------------------- -# A downloaded file must play in EVERY browser, whatever the source shipped. Universal = H.264 -# video + AAC/MP3 audio in an mp4 container: H.264 is the only video codec all browsers decode -# (VP9/AV1 fail on Safari/iOS; HEVC fails on Chrome/Firefox), and mp4/AAC is the safe pairing. -# Anything else is fixed post-download: re-encode the offending stream(s), or just remux when the -# codecs are already fine but the container isn't. -_BROWSER_SAFE_VCODEC = ("h264",) -_BROWSER_SAFE_ACODEC = ("aac", "mp3") +# --- ffprobe --------------------------------------------------------------------------------- def run_ffprobe_json( @@ -493,8 +474,8 @@ def run_ffprobe_json( ) -> dict | None: """One ffprobe call → its parsed JSON dict, or None on any failure (timeout, non-zero exit, missing binary, unparseable output). The SINGLE ffprobe-JSON-invocation point in the downloads - package: the browser-playability codec probe here and the shared media probe (`probe.probe_media`, - used by both the download worker's finalize and the upload ingest) both go through it, so the + package: the shared media probe (`probe.probe_json`: the stored facts and the phone-safe rule) goes + through it, so the subprocess flags/timeout/error handling live in one place instead of drifting copies.""" args = ["ffprobe", "-v", "error"] if show_streams: @@ -517,35 +498,6 @@ def run_ffprobe_json( return None -def _codecs_from_streams(streams: list) -> tuple[str | None, str | None]: - """(video_codec, audio_codec) lowercased from an ffprobe JSON stream list — the first of each kind. - - Parsed from the JSON `streams` list, NOT ffprobe's flat `default=nw=1` output: there the fields - arrive in ffprobe's own struct order, which puts `codec_name` BEFORE `codec_type`, so a line-by-line - "remember the last codec_type" parser attributes every name to the PREVIOUS stream. Measured on real - downloads: an ordinary H.264+AAC mp4 probed as `v=aac a=None` — the browser-playability check then - believed the video stream was AAC and re-encoded a perfectly fine file on every download. Reading each - stream dict by its own `codec_type` is immune to that ordering.""" - vcodec = acodec = None - for s in streams: - name = (s.get("codec_name") or "").lower() or None - ctype = (s.get("codec_type") or "").lower() - if ctype == "video" and vcodec is None: - vcodec = name - elif ctype == "audio" and acodec is None: - acodec = name - return vcodec, acodec - - -def probe_codecs(path: Path) -> tuple[str | None, str | None]: - """Return (video_codec, audio_codec) lowercased via one ffprobe call; None for a missing - stream or if ffprobe fails (a failed probe is treated as 'leave it alone' upstream).""" - data = run_ffprobe_json(path, entries="stream=codec_type,codec_name") - if data is None: - return None, None - return _codecs_from_streams(data.get("streams") or []) - - def probe_duration(path: Path) -> int: """Media duration in whole seconds via ffprobe, or 0 if unknown — a progress denominator for a re-encode when the yt-dlp info dict carried no `duration` (some URL/live extractions).""" @@ -577,49 +529,6 @@ def probe_duration(path: Path) -> int: return 0 -def browser_transcode_flags( - vcodec: str | None, acodec: str | None, is_mp4: bool -) -> tuple[bool, bool, bool]: - """Decide what (if anything) makes a file universally browser-playable. Returns - (needs_change, reencode_video, reencode_audio). needs_change=False => already H.264 + AAC/MP3 - in mp4. A None codec (no such stream, or a failed probe) is never re-encoded.""" - rv = bool(vcodec) and vcodec not in _BROWSER_SAFE_VCODEC - ra = bool(acodec) and acodec not in _BROWSER_SAFE_ACODEC - return (rv or ra or not is_mp4), rv, ra - - -def build_browser_safe_cmd( - src: Path, dest: Path, reencode_video: bool, reencode_audio: bool, threads: int | None = None -) -> list[str]: - """ffmpeg command that lands `src` as a browser-safe mp4 (faststart): re-encode only the - stream(s) that need it (libx264 / aac), stream-copy the rest, always into mp4. `-progress - pipe:1` streams machine-readable progress for run_ffmpeg. `threads` caps the encoder so a - re-encode can't claim every core on the shared box (see the worker-child de-prioritization).""" - cmd = [ - "ffmpeg", - "-y", - "-hide_banner", - "-loglevel", - "error", - "-nostdin", - "-progress", - "pipe:1", - "-nostats", - "-i", - str(src), - ] - cmd += ( - ["-c:v", "libx264", "-preset", "veryfast", "-crf", "20"] - if reencode_video - else ["-c:v", "copy"] - ) - cmd += ["-c:a", "aac", "-b:a", "192k"] if reencode_audio else ["-c:a", "copy"] - if threads and threads > 0: - cmd += ["-threads", str(threads)] - cmd += ["-movflags", "+faststart", str(dest)] - return cmd - - # --- filmstrip (editor scrub track) --------------------------------------------------------- _SB_COLS = 12 diff --git a/backend/app/downloads/formats.py b/backend/app/downloads/formats.py index a8deef1e..edf5dc8a 100644 --- a/backend/app/downloads/formats.py +++ b/backend/app/downloads/formats.py @@ -182,7 +182,9 @@ _DEFAULTS = { # v4: EVERY download is clamped to the mp4/H.264/AAC compat profile (no more mkv/webm/VP9/AV1/Opus # opt-out), so any asset cached under an old non-compat spec — or a stale non-H.264 mp4 that slipped # the v3 net — must re-derive through the current browser-safe guarantee. -_SIG_VERSION = 4 +# v5: the phone-safe rule (`playable`) also judges the pixel format, so a cached 10-bit / 4:4:4 H.264 +# asset (which v4 let through as "H.264") re-derives as 8-bit 4:2:0 instead of being reused. +_SIG_VERSION = 5 # Audio-only downloads version on their own, so a change to the audio bytes doesn't re-download every # cached video. v5: an extracted .mp3/.m4a is stored as itself instead of remuxed into an .mp4 that # iPad's WebKit refuses to play, so a cached remuxed audio asset must re-derive. @@ -219,12 +221,13 @@ def normalize(spec: dict) -> dict: s["embed_subs"] = False # Browser-playability guarantee: the Download module exists to download → share → PLAY # in-browser, so every output must play in EVERY browser, iOS/Safari/WebKit included. The only - # universally-safe combo is mp4 + H.264 + AAC/MP3, so we CLAMP the spec to it HERE — the single - # point every path (picker, stored profile, raw API) flows through — instead of only hiding - # options in the UI. Non-mp4 containers (mkv doesn't play at all; webm/VP9/AV1 fail iOS) and any - # explicit vcodec are coerced to the compat profile; Opus audio (Safari-flaky as a bare file) - # falls back to AAC/m4a. A source that exposes no H.264 is still caught by the worker's - # browser-safe re-encode (`_ensure_browser_playable`), so playback is guaranteed end-to-end. + # universally-safe combo is mp4 + H.264 + AAC (an .mp3/.m4a for audio), so we CLAMP the spec to + # it HERE — the single point every path (picker, stored profile, raw API) flows through — instead + # of only hiding options in the UI. Non-mp4 containers (mkv plays nowhere on iOS; AV1 fails there, + # and webm/VP9 plays only on recent devices) and any explicit vcodec are coerced to the compat + # profile; Opus audio falls back to AAC/m4a. A source that exposes no H.264 (or a 10-bit one) is + # still caught by the worker's phone-safe step (`downloads.playable`), so playback is guaranteed + # end-to-end. s["container"] = "mp4" s["vcodec"] = None if s["audio_format"] not in ("m4a", "mp3"): @@ -351,7 +354,7 @@ def build_ydl_opts( # inside mp4 — a VP9/AV1-in-mp4 file shows a broken player there — so H.264 is selected at the # SELECTION level, not merely preferred: a sort key can't conjure an H.264 stream a source # doesn't expose as video-only. A source that ships only non-H.264 is caught after download by - # the worker's browser-safe re-encode (`_ensure_browser_playable`). + # the worker's phone-safe step (`downloads.playable`). if s["mode"] == "v": # Video-only: keep it audio-free. Prefer an H.264 video-only stream, then any video-only, # and only fall to a progressive (audio-bearing) `best` as a last resort. diff --git a/backend/app/downloads/gc.py b/backend/app/downloads/gc.py index 60267f98..8218cf40 100644 --- a/backend/app/downloads/gc.py +++ b/backend/app/downloads/gc.py @@ -232,6 +232,29 @@ def run_download_gc(db: Session) -> dict: if orphans: db.commit() + # 4b. Reap unheld uploads. An upload carries no expires_at and is never re-enqueued, so the passes + # above never free one that lost its last holder outside `release_asset` — a conversion that + # finished after its job was cancelled, or one cancelled mid-run (the asset was `downloading` + # then). Its file, ready or still the unconverted original, sits at `rel_path`. + unheld = ( + db.execute( + select(MediaAsset).where( + MediaAsset.source_kind == "upload", + MediaAsset.ref_count == 0, + MediaAsset.status != "downloading", + ) + ) + .scalars() + .all() + ) + for asset in unheld: + if asset.rel_path: # no holder is left to tell, so no notice — just the file and the row + storage.delete_asset_files(settings.download_root, asset.rel_path) + db.delete(asset) + reaped += 1 + if unheld: + db.commit() + # 5. Orphan FILE sweep — remove on-disk cruft no surviving asset owns (old accented dirs, stale # posters). Runs last so the keep-set reflects the assets that survived passes 2-4. swept = _sweep_orphan_files(db, settings.download_root) diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index e2554fbe..3d3510ea 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -13,7 +13,8 @@ from app.config import settings from app.db import SessionLocal from app.downloads import edit as editmod from app.downloads import errors as dlerrors -from app.downloads import adapters, credentials, formats, mediakind, probe, quota, service, storage +from app.downloads import adapters, credentials, formats, mediakind, playable, probe, quota +from app.downloads import service, storage from app.downloads import urlguard from app.downloads.jobdb import ( _edit_staging_dir, @@ -416,48 +417,50 @@ def _download_settings() -> tuple[str, int]: ) -def _ensure_browser_playable(job_id: int, produced: Path, spec: dict, info: dict) -> Path: - """Make an mp4-compat-profile download play in every browser: H.264 + AAC/MP3 in mp4. If the - download (whatever the source shipped) isn't already that, re-encode the offending stream(s) — - or just remux when the codecs are fine but the container isn't. **Scope:** only the mp4 compat - profile is normalized; an explicit `vcodec` or a non-mp4 custom profile is the user opting out - of the guarantee and is left as-is (so a `webm` profile can still ship VP9/Opus). Returns the - file to store (converted, or the original) and corrects `info`'s codec fields to match disk. +def _ensure_phone_safe( + job_id: int, + produced: Path, + out_dir: Path, + phase: str, + total_s: int | None = None, + *, + keep_source: bool = False, +) -> Path: + """Bring a file that is about to become a ready library asset into the phone-safe shape + (`playable`), writing any converted copy into `out_dir`. Returns the file to store — `produced` + itself when it already is that shape, or isn't media at all — and removes `produced` once a + converted copy exists, unless `keep_source` (an upload's original, which must outlive the + conversion until the database points at the new file). The job shows `phase` with real ffmpeg + progress while it converts. - An audio download (mode "a") is never touched: yt-dlp already extracted it to .mp3 / .m4a, which - every browser plays as audio/mpeg / audio/mp4. Remuxing it into .mp4 served an mp3 as video/mp4, - which iPad's WebKit refuses to play (prod job 268).""" - if spec.get("mode") == "a" or spec.get("container") != "mp4" or spec.get("vcodec"): + Every ready path calls this — a download, an upload's worker conversion and an editor clip — so + a share link can only ever serve the one shape every phone and browser plays.""" + p = playable.plan(probe.probe_json(produced), produced.suffix) + if p is None or not p.needs_change: return produced - vcodec, acodec = editmod.probe_codecs(produced) - is_mp4 = produced.suffix.lower() == ".mp4" - needs, reenc_v, reenc_a = editmod.browser_transcode_flags(vcodec, acodec, is_mp4) - if not needs: - return produced - - dest = produced.with_name(produced.stem + ".browser.mp4") - action = "re-encode" if (reenc_v or reenc_a) else "remux" + dest = out_dir / f"{produced.stem}.safe.{p.ext}" + action = "re-encode" if (p.reencode_video or p.reencode_audio) else "remux" log.info( - "job %s: %s (v=%s a=%s mp4=%s) -> browser-safe mp4", job_id, action, vcodec, acodec, is_mp4 + "job %s: %s (video=%s audio=%s) -> phone-safe .%s", + job_id, + action, + p.reencode_video, + p.reencode_audio, + p.ext, + extra=logs.event("download.phone_safe", job_id=job_id, action=action, ext=p.ext), ) - _set_job(job_id, phase="processing", progress=0, speed_bps=None, eta_s=None) - # Prefer the info duration; probe the file when the extractor didn't provide one, so a long - # re-encode still shows real progress instead of a frozen 0%. - total_s = int(info["duration"]) if info.get("duration") else editmod.probe_duration(produced) + _set_job(job_id, phase=phase, progress=0, speed_bps=None, eta_s=None) + # A re-encode with no known length would hold a frozen 0%; probe the file for a denominator. editmod.run_ffmpeg( - editmod.build_browser_safe_cmd( - produced, dest, reenc_v, reenc_a, threads=editmod.encode_thread_cap() - ), - total_s=total_s, - on_progress=_ffmpeg_progress_writer(job_id, "processing"), + playable.build_cmd(produced, dest, p, threads=editmod.encode_thread_cap()), + total_s=total_s or editmod.probe_duration(produced), + on_progress=_ffmpeg_progress_writer(job_id, phase), should_cancel=_cancel_poll(job_id), ) - produced.unlink(missing_ok=True) - # Keep the stored asset metadata truthful about what's now on disk. - if reenc_v: - info["vcodec"] = "h264" - if reenc_a: - info["acodec"] = "aac" + if not dest.exists(): + raise StatedFailure("ffmpeg produced no output file") + if not keep_source: + produced.unlink(missing_ok=True) return dest @@ -522,7 +525,13 @@ def _download( adapter = adapters.resolve_adapter(source_kind, source_ref, info) produced = _produced_file(info, staging) - produced = _ensure_browser_playable(job_id, produced, spec, info) + produced = _ensure_phone_safe( + job_id, + produced, + staging, + "processing", + int(info["duration"]) if info.get("duration") else None, + ) ext = produced.suffix.lstrip(".").lower() # Codecs/dims/duration come from a PROBE of the produced file — the ground truth — NOT yt-dlp's # `info` dict, whose fields are absent for some sources (Facebook among them): deriving @@ -722,9 +731,12 @@ def _log_download_failed(job_id: int, msg: str, code: str | None, exc: BaseExcep ) -def _fail_edit(job_id: int, asset_id: int, msg: str, exc: BaseException | None = None) -> None: - """Fail an edit job, and say so ONCE — every way an edit fails comes through here, so this is - where its line and its event code live. `exc` is the exception when there was one. +def _fail_edit( + job_id: int, asset_id: int, msg: str, exc: BaseException | None = None, *, what: str = "edit" +) -> None: + """Fail a local ffmpeg job — an edit, or an upload's conversion (`what="upload"`) — and say so + ONCE: every way one fails comes through here, so this is where its line and its event code + (`.job.failed`) live. `exc` is the exception when there was one. Its traceback goes with the line unless the message already tells it all (`_failure_stack`). Logged after the write, as on the download error path.""" @@ -746,12 +758,13 @@ def _fail_edit(job_id: int, asset_id: int, msg: str, exc: BaseException | None = # `msg` is the user's copy; ffmpeg's stderr tail in it can name the source file, whose path is # built from the channel and the title. log.warning( - "edit job %s failed: %s", + "%s job %s failed: %s", + what, job_id, formats.log_safe(msg), extra={ **logs.event( - "edit.job.failed", + f"{what}.job.failed", job_id=job_id, asset_id=asset_id, **({"error_type": type(exc).__name__} if exc is not None else {}), @@ -811,6 +824,10 @@ def _process_edit(job_id: int, asset_id: int) -> None: ) if not dest_staging.exists(): raise StatedFailure("ffmpeg produced no output file") + # A fast trim stream-copies the source's codecs and container, so the clip is only as + # playable as its source: hold it to the same rule as every other ready file. + dest_staging = _ensure_phone_safe(job_id, dest_staging, staging, "editing", total_s) + out_ext = dest_staging.suffix.lstrip(".").lower() or out_ext rel = storage.edit_rel_path(user_id, asset_id, out_ext) storage.place_file(dest_staging, settings.download_root, rel) @@ -867,3 +884,73 @@ def _process_edit(job_id: int, asset_id: int) -> None: _fail_edit(job_id, asset_id, str(exc)[:500], exc) finally: shutil.rmtree(staging, ignore_errors=True) + + +def _convert_upload(job_id: int, asset_id: int) -> None: + """Convert a queued upload into the phone-safe shape (`playable`) and make it ready. + + The original sits at the asset's `rel_path` (see `upload`); the converted file replaces it there + under the new extension, and only then is the original removed — so a failed or stopped + conversion leaves the original in place for Retry, and a delete still finds it by `rel_path`.""" + with SessionLocal() as db: + asset = db.get(MediaAsset, asset_id) + old_rel = asset.rel_path if asset else None + duration = asset.duration_s if asset else None + src = storage.safe_abs_path(settings.download_root, old_rel) if old_rel else None + if old_rel is None or src is None or not src.exists(): + _fail_edit(job_id, asset_id, "The uploaded file is missing.", what="upload") + return + staging = _staging_dir(asset_id) + try: + shutil.rmtree(staging, ignore_errors=True) + staging.mkdir(parents=True, exist_ok=True) + safe = _ensure_phone_safe(job_id, src, staging, "processing", duration, keep_source=True) + ext = safe.suffix.lstrip(".").lower() + new_rel = Path(old_rel).with_suffix(f".{ext}").as_posix() + if safe != src: + storage.place_file(safe, settings.download_root, new_rel) + final = storage.abs_path(settings.download_root, new_rel) + vcodec, acodec, width, height, dur = probe.probe_media(final) + with SessionLocal() as db: + asset = db.get(MediaAsset, asset_id) + if asset: + asset.status = "ready" + asset.rel_path = new_rel + asset.container = ext + asset.size_bytes = final.stat().st_size + asset.vcodec, asset.acodec = vcodec, acodec + asset.width, asset.height = width or asset.width, height or asset.height + asset.duration_s = dur or asset.duration_s + asset.error = None + asset.last_access_at = datetime.now(timezone.utc) + job = db.get(DownloadJob, job_id) + owner_id = job.user_id if job else None + title = asset.title if asset else None + finalized = service.finalize_if_running( + db, job_id, status=JobStatus.DONE, progress=100, phase=None + ) + db.commit() + if finalized: + _notify_download_done(db, owner_id, title, job_id, kind="download") + if new_rel != old_rel: + src.unlink(missing_ok=True) + if finalized: + log.info( + "upload job %s converted: asset %s", + job_id, + asset_id, + extra=logs.event("upload.job.done", job_id=job_id, asset_id=asset_id, ext=ext), + ) + except editmod.EditAborted: + # Paused/canceled mid-encode: the original is untouched; free the asset for a later retry. + _reset_asset_pending(asset_id) + log.info( + "upload job %s conversion aborted (pause/cancel)", + job_id, + extra=logs.event("upload.job.aborted", job_id=job_id), + ) + except Exception as exc: # noqa: BLE001 — surface the failure, keep the worker alive + # silent-ok: not silent — `_fail_edit` is the report, and it takes `exc` for its stack. + _fail_edit(job_id, asset_id, str(exc)[:500], exc, what="upload") + finally: + shutil.rmtree(staging, ignore_errors=True) diff --git a/backend/app/downloads/playable.py b/backend/app/downloads/playable.py new file mode 100644 index 00000000..8de66bcb --- /dev/null +++ b/backend/app/downloads/playable.py @@ -0,0 +1,99 @@ +"""The phone-safe rule: what a library file must be so it plays wherever a share link lands. + +A shared `/watch` link is opened by whoever got it, on whatever they hold: an iPhone's WebKit, an +Android Chrome, a desktop Firefox. The one shape every one of them plays: + + * video → H.264, 8-bit 4:2:0 (`yuv420p`, or its full-range twin `yuvj420p`), AAC audio, in .mp4 + * audio only → MP3 in .mp3, or AAC in .m4a + +Measured on the owner's iPhone + iPad (2026-10-05): AV1, Matroska, HEVC tagged `hev1` and MP3 +inside an .mp4 all fail there. VP9/WebM, HEVC `hvc1` and 10-bit H.264 played on those devices, but +Chrome and Firefox do not decode HEVC and browsers decode 10-bit / 4:4:4 H.264 only patchily, so the +rule is the shape every browser plays, not the shape one device plays. + +Every file that becomes a ready library asset passes through here — a download, an upload (the +worker converts it, see `upload`), and an editor clip — so a link can only ever serve this shape. +`plan` says what (if anything) must change; `build_cmd` is the one ffmpeg command that changes it: +re-encode only the stream that fails, copy the rest, or just remux when only the container is wrong. +""" + +from dataclasses import dataclass +from pathlib import Path + +from app.downloads import edit as editmod +from app.downloads import probe + +_SAFE_VCODEC = "h264" +_SAFE_PIX_FMTS = frozenset({"yuv420p", "yuvj420p"}) +_VIDEO_EXT = "mp4" +# An audio-only file: the codec, and the extension it plays in. Anything else becomes AAC in .m4a. +_AUDIO_HOME = {"mp3": "mp3", "aac": "m4a"} +_AUDIO_FALLBACK_EXT = "m4a" + + +@dataclass(frozen=True) +class Plan: + """What makes one file phone-safe. `ext` is the extension the safe file has (no dot).""" + + ext: str + has_video: bool + reencode_video: bool + reencode_audio: bool + needs_change: bool + + +def plan(data: dict | None, ext: str) -> Plan | None: + """The phone-safe plan for a file whose `probe.probe_json` is `data` and whose extension is + `ext`; None when it is not media at all (an unreadable probe, a still image, no audio or video + stream) — such a file is a download-only 'file' and never plays inline, so there is nothing to + make safe.""" + if data is None: + return None + video, audio = probe.primary_streams(data) + ext = ext.lower().lstrip(".") + if video is not None: + rv = (video.get("codec_name") or "").lower() != _SAFE_VCODEC or ( + video.get("pix_fmt") or "" + ).lower() not in _SAFE_PIX_FMTS + ra = audio is not None and (audio.get("codec_name") or "").lower() != "aac" + return Plan(_VIDEO_EXT, True, rv, ra, rv or ra or ext != _VIDEO_EXT) + if audio is not None: + home = _AUDIO_HOME.get((audio.get("codec_name") or "").lower()) + target = home or _AUDIO_FALLBACK_EXT + return Plan(target, False, False, home is None, home is None or ext != target) + return None + + +def build_cmd(src: Path, dest: Path, p: Plan, threads: int | None = None) -> list[str]: + """The ffmpeg command that writes `src` to `dest` (whose suffix is `p.ext`) in the phone-safe + shape: the failing stream(s) re-encoded with the package's shared encoder settings, the rest + stream-copied. An audio-only result drops any cover-art stream. `-progress pipe:1` + streams machine-readable progress; `threads` caps the encoder so a re-encode cannot claim every + core on the shared box (see the worker-child de-prioritization).""" + cmd = [ + "ffmpeg", + "-y", + "-hide_banner", + "-loglevel", + "error", + "-nostdin", + "-progress", + "pipe:1", + "-nostats", + "-i", + str(src), + ] + # Exactly the two streams `plan` judged: `V` is a video stream that is not cover art. Nothing + # else rides along — a subtitle or data track the .mp4 muxer cannot take would fail the run. + if p.has_video: + cmd += ["-map", "0:V:0", "-map", "0:a:0?"] + cmd += editmod.X264_ARGS if p.reencode_video else ["-c:v", "copy"] + else: + cmd += ["-map", "0:a:0"] + cmd += editmod.AAC_ARGS if p.reencode_audio else ["-c:a", "copy"] + if threads and threads > 0: + cmd += ["-threads", str(threads)] + if p.ext in ("mp4", "m4a"): + cmd += ["-movflags", "+faststart"] + cmd += [str(dest)] + return cmd diff --git a/backend/app/downloads/probe.py b/backend/app/downloads/probe.py index 04dd70d3..3e29d157 100644 --- a/backend/app/downloads/probe.py +++ b/backend/app/downloads/probe.py @@ -34,6 +34,28 @@ _IMAGE_FORMATS = { } +def primary_streams(data: dict) -> tuple[dict | None, dict | None]: + """(video stream, audio stream) — the first of each kind — from a NON-None ffprobe JSON dict, by + the same reading the stored facts use: a still image / image-sequence container has neither (it + is a generic 'file'), and a video stream flagged `attached_pic` (an mp3/m4a's cover art) is not a + video track. The phone-safe rule (`playable`) judges exactly these two streams.""" + fmt_names = { + n.strip() for n in ((data.get("format") or {}).get("format_name") or "").split(",") + } + if fmt_names & _IMAGE_FORMATS: + return None, None + video = audio = None + for stream in data.get("streams", []): + kind = stream.get("codec_type") + if kind == "video" and video is None: + if (stream.get("disposition") or {}).get("attached_pic"): + continue # cover art, not a video track + video = stream + elif kind == "audio" and audio is None: + audio = stream + return video, audio + + def _facts_from_probe( data: dict, ) -> tuple[str | None, str | None, int | None, int | None, int | None]: @@ -45,19 +67,11 @@ def _facts_from_probe( fmt_names = {n.strip() for n in (fmt.get("format_name") or "").split(",")} if fmt_names & _IMAGE_FORMATS: return None, None, None, None, None - - vcodec = acodec = None - width = height = None - for stream in data.get("streams", []): - kind = stream.get("codec_type") - if kind == "video" and vcodec is None: - if (stream.get("disposition") or {}).get("attached_pic"): - continue # cover art, not a video track - vcodec = stream.get("codec_name") - width = stream.get("width") - height = stream.get("height") - elif kind == "audio" and acodec is None: - acodec = stream.get("codec_name") + video, audio = primary_streams(data) + vcodec = video.get("codec_name") if video else None + width = video.get("width") if video else None + height = video.get("height") if video else None + acodec = audio.get("codec_name") if audio else None duration = None try: @@ -71,16 +85,29 @@ def _facts_from_probe( return vcodec, acodec, width, height, duration -def probe_media(path: Path) -> tuple[str | None, str | None, int | None, int | None, int | None]: - """(vcodec, acodec, width, height, duration_s) from one ffprobe call; all None if ffprobe fails - or the file isn't playable media (an unprobeable file — or a still image — is treated as a generic - non-media file, so it becomes a download-only 'file' rather than a broken 'video').""" - data = editmod.run_ffprobe_json(path, show_streams=True, show_format=True) +def probe_json(path: Path) -> dict | None: + """The full ffprobe JSON (streams + format) of `path`, or None when ffprobe could not read it — + the one call both the stored facts (`facts`) and the phone-safe rule read from.""" + return editmod.run_ffprobe_json(path, show_streams=True, show_format=True) + + +def facts( + data: dict | None, +) -> tuple[str | None, str | None, int | None, int | None, int | None]: + """(vcodec, acodec, width, height, duration_s) of a `probe_json` result; all None when it is None + (an unprobeable file is treated as a generic non-media file, so it becomes a download-only 'file' + rather than a broken 'video').""" if data is None: return None, None, None, None, None return _facts_from_probe(data) +def probe_media(path: Path) -> tuple[str | None, str | None, int | None, int | None, int | None]: + """(vcodec, acodec, width, height, duration_s) from one ffprobe call; all None if ffprobe fails + or the file isn't playable media (a still image is a generic 'file', see `primary_streams`).""" + return facts(probe_json(path)) + + def resolve_media_facts( path: Path, info: dict ) -> tuple[str | None, str | None, int | None, int | None, int | None]: diff --git a/backend/app/downloads/runner.py b/backend/app/downloads/runner.py index b609e32e..654de3e8 100644 --- a/backend/app/downloads/runner.py +++ b/backend/app/downloads/runner.py @@ -95,12 +95,14 @@ def _run(job_id, asset_id, job_kind, source_kind, source_ref, spec, user_id) -> (`cause=child_exit`).""" # Imported here (not at module top) so the child pays the yt-dlp import cost only when it has # real work, and to keep the import graph obvious: runner is a thin entry over the pipeline. - from app.downloads.pipeline import _download, _process_edit + from app.downloads.pipeline import _convert_upload, _download, _process_edit with formats.hiding(source_ref): try: if job_kind == "edit": _process_edit(job_id, asset_id) + elif job_kind == "upload": + _convert_upload(job_id, asset_id) else: _download(job_id, asset_id, source_kind, source_ref, spec, user_id) except Exception as exc: # noqa: BLE001 — whatever it is, it is reported here or raw diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index f643d04d..22c2a019 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -318,14 +318,22 @@ def release_asset(db: Session, job: DownloadJob) -> None: re-enqueuing the same (source,format) — get_or_create_asset would reset that row to `pending` and +1 it, and deleting it under that would FK-null the new job's asset (a lost update on ref_count). A ready asset is still freed at ref==0 (its file is the point); an orphaned errored row is cheap - (no file) and gets reused+reset by the next enqueue, or reclaimed by a later GC pass.""" + (no file) and gets reused+reset by the next enqueue, or reclaimed by a later GC pass. + + An UPLOAD is the exception: it is never shared or re-enqueued (its ref is unique), and while it + waits for or failed its phone-safe conversion the original file already sits at `rel_path` — so + it is freed at ref==0 whatever its state, except mid-conversion (`downloading`: the worker child + still reads it; the GC upload pass takes it once the child has stopped).""" if not job.asset_id or job.status not in HOLDING: return asset = db.get(MediaAsset, job.asset_id) if asset is None: return new_count = decr_ref_count(db, asset.id) - if new_count == 0 and asset.status == "ready": + freeable = asset.status == "ready" or ( + asset.source_kind == "upload" and asset.status != "downloading" + ) + if new_count == 0 and freeable: if asset.rel_path: storage.delete_asset_files(settings.download_root, asset.rel_path) db.delete(asset) diff --git a/backend/app/downloads/upload.py b/backend/app/downloads/upload.py index ea565c50..e7ebd443 100644 --- a/backend/app/downloads/upload.py +++ b/backend/app/downloads/upload.py @@ -2,14 +2,21 @@ A user-uploaded file joins the SAME library as a downloaded one, so it inherits the whole function set already built (metadata chips, share links, and — for media — in-app play/edit). Unlike a -download it bypasses yt-dlp AND the worker entirely: the file is already on the host, so ingest -probes it, places it on disk, and creates an already-`ready` MediaAsset + `done` DownloadJob in one -step — the same "the file exists, mark it done now" shortcut `service.enqueue` takes on a cache hit. +download it bypasses yt-dlp: the file is already on the host, so ingest probes it and places it on +disk. A file that already is the phone-safe shape (`playable`) — or is not media at all — becomes an +already-`ready` MediaAsset + `done` DownloadJob in one step, the same "the file exists, mark it done +now" shortcut `service.enqueue` takes on a cache hit. + +Media that is NOT phone-safe (an iPhone's HEVC .mov, a VP9 .webm, an .mkv) is placed as it came and +queued: the asset stays `pending` and the job `queued`, and the download worker converts it +(`pipeline._convert_upload`) — so a share link never hands a friend a file their phone cannot play. +Only the converted file is kept (the owner's choice: one file per upload; a 10-bit HDR original +becomes 8-bit SDR). Until then the original sits at the asset's `rel_path`, which is what a failed +conversion's Retry starts from again, and what a delete removes (`service.release_asset`). Kinds (the invariant lives in `mediakind`): a probe with a real video stream -> video, audio-only -> audio, neither -> a generic 'file' (pdf/zip/docx/…) that is download-only and never rendered -inline (see the force-download serving in the file/watch routes). This module is the ingest seam a -future sub-function (e.g. convert) plugs a new adapter into, mirroring how it sidesteps the worker. +inline (see the force-download serving in the file/watch routes). An upload does NOT get an `expires_at`: it's user-provided and may be irreplaceable (there's no source to re-download), so it doesn't age out on the retention clock the way a re-fetchable download @@ -24,7 +31,8 @@ from sqlalchemy.orm import Session from app.config import settings from app.downloads import edit as editmod -from app.downloads import mediakind, probe, quota, service, storage +from app.downloads import mediakind, playable, probe, quota, service, storage +from app.downloads.status import JobStatus from app.models import DownloadJob, MediaAsset, display_text @@ -44,15 +52,19 @@ def ingest_upload( size: int, display_name: str | None = None, ) -> DownloadJob: - """Ingest an already-uploaded file at `src_path` into `user_id`'s library and return the `done` - job. `src_path` is MOVED into place on success (the caller stages it to a temp file first). + """Ingest an already-uploaded file at `src_path` into `user_id`'s library and return its job — + `done`, or `queued` for the worker's phone-safe conversion (see the module docstring). `src_path` + is MOVED into place on success (the caller stages it to a temp file first). Raises quota.QuotaExceeded (holdings/footprint cap) — checked BEFORE any disk write, so a rejected upload leaves nothing behind and the caller just cleans up its temp file.""" quota.check_upload(db, user_id, size) # the file's size is known → charge it up front - vcodec, acodec, width, height, duration = probe.probe_media(src_path) + data = probe.probe_json(src_path) + vcodec, acodec, width, height, duration = probe.facts(data) kind = mediakind.derive(vcodec, acodec) + safe = playable.plan(data, Path(filename).suffix) if kind != mediakind.FILE else None + convert = safe is not None and safe.needs_change # An upload never dedups (each file is its own asset): a random ref makes the cache key unique, # so `format_sig` is a constant marker rather than a format hash. @@ -73,7 +85,7 @@ def ingest_upload( final = storage.abs_path(settings.download_root, rel) actual_size = final.stat().st_size if final.exists() else size - asset.status = "ready" + asset.status = "pending" if convert else "ready" asset.rel_path = rel asset.size_bytes = actual_size asset.container = storage.upload_ext(filename) @@ -100,8 +112,8 @@ def ingest_upload( source_ref=ref, profile_snapshot={}, display_name=display_text(display_name), - status="done", - progress=100, + status=JobStatus.QUEUED if convert else JobStatus.DONE, + progress=0 if convert else 100, queue_pos=service.next_queue_pos(db), ) db.add(job) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index d2e1c5d2..49e656dc 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -502,7 +502,8 @@ def upload_download( (video/audio) gets the full function set, a generic file is download-only. The body IS the file (see the header note above); `spool_upload` has already authenticated the caller and streamed it to a temp path under the download volume, so what is left here is the non-yt-dlp ingest which - probes + places it and returns an already-`done` job. + probes + places it and returns its job — `done`, or `queued` for the worker's phone-safe + conversion (`downloads.upload`). A sync (threadpool) handler on purpose: the ingest does blocking work — an ffprobe of up to 30s plus sync DB writes — so it must NOT run on the event loop. The streaming half is async because diff --git a/backend/tests/test_download_deprioritize.py b/backend/tests/test_download_deprioritize.py index 0264a4c5..1e16a25c 100644 --- a/backend/tests/test_download_deprioritize.py +++ b/backend/tests/test_download_deprioritize.py @@ -8,6 +8,7 @@ import pytest from app.config import settings from app.downloads import edit as editmod +from app.downloads import playable from app.downloads import runner @@ -32,11 +33,12 @@ class TestEncodeThreadCap: assert editmod.encode_thread_cap() == 1 # max(1, 2 - 1) -class TestBrowserSafeThreads: +_REENCODE_VIDEO = playable.Plan("mp4", True, True, False, True) + + +class TestPhoneSafeThreads: def test_threads_flag_added_when_capped(self): - cmd = editmod.build_browser_safe_cmd( - "/in.mkv", "/out.mp4", reencode_video=True, reencode_audio=False, threads=2 - ) + cmd = playable.build_cmd("/in.mkv", "/out.mp4", _REENCODE_VIDEO, threads=2) assert "-threads" in cmd assert cmd[cmd.index("-threads") + 1] == "2" # The cap precedes the output file, so it binds the encoder (not input decoding). @@ -44,9 +46,7 @@ class TestBrowserSafeThreads: @pytest.mark.parametrize("threads", [0, None]) def test_no_threads_flag_when_uncapped(self, threads): - cmd = editmod.build_browser_safe_cmd( - "/in.mkv", "/out.mp4", reencode_video=True, reencode_audio=False, threads=threads - ) + cmd = playable.build_cmd("/in.mkv", "/out.mp4", _REENCODE_VIDEO, threads=threads) assert "-threads" not in cmd diff --git a/backend/tests/test_download_output.py b/backend/tests/test_download_output.py index 9d888b75..4e286817 100644 --- a/backend/tests/test_download_output.py +++ b/backend/tests/test_download_output.py @@ -6,7 +6,7 @@ with "PosixPath('.') has an empty name" instead of downloading.""" import pytest -from app.downloads.edit import _codecs_from_streams, _concat_quote, run_ffprobe_json +from app.downloads.edit import _concat_quote, run_ffprobe_json from app.downloads.pipeline import _media_info, _produced_file from pathlib import Path, PurePosixPath @@ -105,46 +105,9 @@ class TestMediaInfo: assert _media_info(info)["id"] == "vid" -class TestCodecsFromStreams: - # ffprobe emits codec_name BEFORE codec_type, so the old line parser shifted every name onto - # the previous stream and an H.264+AAC mp4 came back as (aac, None) → a needless re-encode. - # _codecs_from_streams reads each stream dict by its own codec_type, immune to that ordering. - def test_reads_each_stream_by_its_own_type(self): - streams = [ - {"codec_name": "h264", "codec_type": "video"}, - {"codec_name": "aac", "codec_type": "audio"}, - ] - assert _codecs_from_streams(streams) == ("h264", "aac") - - def test_audio_first_ordering_is_handled(self): - streams = [ - {"codec_name": "AAC", "codec_type": "audio"}, - {"codec_name": "H264", "codec_type": "video"}, - ] - assert _codecs_from_streams(streams) == ("h264", "aac") - - def test_extra_streams_do_not_win_over_the_first_of_their_kind(self): - streams = [ - {"codec_name": "h264", "codec_type": "video"}, - {"codec_name": "vp9", "codec_type": "video"}, - {"codec_name": "mov_text", "codec_type": "subtitle"}, - {"codec_name": "opus", "codec_type": "audio"}, - ] - assert _codecs_from_streams(streams) == ("h264", "opus") - - def test_audio_only_and_video_only(self): - assert _codecs_from_streams([{"codec_name": "opus", "codec_type": "audio"}]) == ( - None, - "opus", - ) - assert _codecs_from_streams([{"codec_name": "av1", "codec_type": "video"}]) == ("av1", None) - - def test_empty_stream_list(self): - assert _codecs_from_streams([]) == (None, None) - - class TestRunFfprobeJson: - """The single ffprobe-JSON invocation point (shared by probe_codecs + probe.probe_media). A failed + """The single ffprobe-JSON invocation point (`probe.probe_json`, which the stored facts and the + phone-safe rule both read). A failed probe or unparseable output must degrade to None, not raise — the malformed-input safety the old parse_probe_streams string parser used to own now lives here.""" @@ -174,7 +137,7 @@ class TestRunFfprobeJson: monkeypatch.setattr(subprocess, "run", lambda *a, **k: _R()) data = run_ffprobe_json(Path("x"), entries="stream=codec_type,codec_name") - assert _codecs_from_streams(data["streams"]) == ("h264", None) + assert data["streams"] == [{"codec_name": "h264", "codec_type": "video"}] class TestConcatQuote: diff --git a/backend/tests/test_formats.py b/backend/tests/test_formats.py index abf79b1c..ab461d42 100644 --- a/backend/tests/test_formats.py +++ b/backend/tests/test_formats.py @@ -94,23 +94,6 @@ def test_audio_only_unaffected(): assert _opts({"mode": "a", "audio_format": "m4a"})["format"] == "bestaudio/best" -def test_browser_transcode_flags_universal_playability(): - F = editmod.browser_transcode_flags - # Already universal: H.264 + AAC in mp4 → no change. - assert F("h264", "aac", True) == (False, False, False) - assert F("h264", "mp3", True) == (False, False, False) - # Non-H.264 video → full video re-encode (AV1/VP9 fail iOS; HEVC fails Chrome/FF). - assert F("av1", "aac", True) == (True, True, False) - assert F("vp9", "opus", True) == (True, True, True) - assert F("hevc", "aac", True) == (True, True, False) - # H.264 but bad audio → audio-only transcode (video copied). - assert F("h264", "opus", True) == (True, False, True) - # Codecs fine but wrong container → remux only (no re-encode). - assert F("h264", "aac", False) == (True, False, False) - # A failed probe / missing stream is never re-encoded. - assert F(None, None, True) == (False, False, False) - - # --- SEC-13 · one enqueue is one bounded download ------------------------------------------- diff --git a/backend/tests/test_playable.py b/backend/tests/test_playable.py new file mode 100644 index 00000000..ddb58e35 --- /dev/null +++ b/backend/tests/test_playable.py @@ -0,0 +1,157 @@ +"""The phone-safe rule (`downloads.playable`): what a library file must be so a share link plays +on any phone or browser, and the one ffmpeg command that makes it so. + +The plan table is the measured device verdict, class by class: what failed on a real iPhone + iPad +(AV1, Matroska, HEVC, MP3 inside an .mp4) must change, and so must what browsers other than Safari +cannot be trusted with (10-bit / 4:4:4 H.264, VP9, Opus). The ffmpeg cases run the real encoder and +probe its output, because the rule is about the bytes, not the flags. +""" + +import shutil +import subprocess +from pathlib import Path + +import pytest + +from app.downloads import playable, probe + +# conftest turns a missing ffmpeg into a hard error in the gate (SIFTLODE_TEST_REQUIRE_DB), so this +# only ever skips on an explicit pure-logic run. +needs_ffmpeg = pytest.mark.skipif(shutil.which("ffmpeg") is None, reason="ffmpeg not available") + + +def _data(video=None, audio=None, fmt="mov,mp4,m4a,3gp,3g2,mj2", cover=False): + streams = [] + if cover: + streams.append( + {"codec_type": "video", "codec_name": "mjpeg", "disposition": {"attached_pic": 1}} + ) + if video: + codec, pix = video + streams.append({"codec_type": "video", "codec_name": codec, "pix_fmt": pix}) + if audio: + streams.append({"codec_type": "audio", "codec_name": audio}) + return {"streams": streams, "format": {"format_name": fmt}} + + +# (probe, extension) -> (ext, reencode_video, reencode_audio, needs_change) +CASES = { + "h264 8-bit + aac in mp4": (_data(("h264", "yuv420p"), "aac"), "mp4", ("mp4", 0, 0, 0)), + "full-range h264 is still 8-bit 4:2:0": ( + _data(("h264", "yuvj420p"), "aac"), + "mp4", + ("mp4", 0, 0, 0), + ), + "10-bit h264": (_data(("h264", "yuv420p10le"), "aac"), "mp4", ("mp4", 1, 0, 1)), + "4:4:4 h264": (_data(("h264", "yuv444p"), "aac"), "mp4", ("mp4", 1, 0, 1)), + "hevc (an iPhone's .mov)": (_data(("hevc", "yuv420p10le"), "aac"), "mov", ("mp4", 1, 0, 1)), + "vp9 + opus webm": (_data(("vp9", "yuv420p"), "opus"), "webm", ("mp4", 1, 1, 1)), + "av1": (_data(("av1", "yuv420p"), "aac"), "mp4", ("mp4", 1, 0, 1)), + "h264 + aac in mkv: remux only": (_data(("h264", "yuv420p"), "aac"), "mkv", ("mp4", 0, 0, 1)), + "h264 + opus in mp4": (_data(("h264", "yuv420p"), "opus"), "mp4", ("mp4", 0, 1, 1)), + "mp3 beside video in mp4": (_data(("h264", "yuv420p"), "mp3"), "mp4", ("mp4", 0, 1, 1)), + "a silent h264 video": (_data(("h264", "yuv420p")), "mp4", ("mp4", 0, 0, 0)), + "mp3": (_data(audio="mp3", fmt="mp3"), "mp3", ("mp3", 0, 0, 0)), + "mp3 with cover art": (_data(audio="mp3", fmt="mp3", cover=True), "mp3", ("mp3", 0, 0, 0)), + "mp3 inside an mp4: remux to .mp3": (_data(audio="mp3"), "mp4", ("mp3", 0, 0, 1)), + "aac in m4a": (_data(audio="aac"), "m4a", ("m4a", 0, 0, 0)), + "aac audio-only in mp4: remux to .m4a": (_data(audio="aac"), "mp4", ("m4a", 0, 0, 1)), + "opus in ogg": (_data(audio="opus", fmt="ogg"), "ogg", ("m4a", 0, 1, 1)), +} + + +@pytest.mark.parametrize("name", CASES) +def test_the_plan_for_each_class(name): + data, ext, (want_ext, rv, ra, needs) = CASES[name] + p = playable.plan(data, ext) + assert p is not None + assert (p.ext, p.reencode_video, p.reencode_audio, p.needs_change) == ( + want_ext, + bool(rv), + bool(ra), + bool(needs), + ) + + +@pytest.mark.parametrize( + "data", + [ + None, # ffprobe could not read it + {"streams": [], "format": {"format_name": "pdf"}}, + _data(("png", "rgb24"), fmt="png_pipe"), # a still image is a download-only file + ], +) +def test_a_file_that_is_not_media_has_no_plan(data): + assert playable.plan(data, "bin") is None + + +def test_the_extension_is_read_with_or_without_its_dot(): + data = _data(("h264", "yuv420p"), "aac") + assert playable.plan(data, ".MP4") == playable.plan(data, "mp4") + assert not playable.plan(data, ".MP4").needs_change + + +def test_a_video_re_encode_pins_8_bit_420_and_maps_only_the_judged_streams(): + p = playable.Plan("mp4", True, True, False, True) + cmd = playable.build_cmd(Path("/in.mov"), Path("/out.mp4"), p) + assert cmd[cmd.index("-pix_fmt") + 1] == "yuv420p" + assert cmd[cmd.index("-c:v") + 1] == "libx264" + assert cmd[cmd.index("-c:a") + 1] == "copy" + maps = [cmd[i + 1] for i, a in enumerate(cmd) if a == "-map"] + assert maps == ["0:V:0", "0:a:0?"] # never cover art, never a subtitle the muxer refuses + assert cmd[-3:] == ["-movflags", "+faststart", "/out.mp4"] + + +def test_an_audio_remux_copies_the_audio_alone(): + p = playable.Plan("mp3", False, False, False, True) + cmd = playable.build_cmd(Path("/in.mp4"), Path("/out.mp3"), p) + assert [cmd[i + 1] for i, a in enumerate(cmd) if a == "-map"] == ["0:a:0"] + assert "-c:v" not in cmd and "-pix_fmt" not in cmd + assert cmd[cmd.index("-c:a") + 1] == "copy" + assert "-movflags" not in cmd # an .mp3 has no moov atom to move + + +# --- the real encoder ------------------------------------------------------------------------ + + +def _ffmpeg(*args: str) -> None: + subprocess.run(["ffmpeg", "-y", "-loglevel", "error", *args], check=True) + + +def _convert(src: Path, tmp_path: Path) -> Path: + p = playable.plan(probe.probe_json(src), src.suffix) + assert p is not None and p.needs_change + dest = tmp_path / f"out.{p.ext}" + subprocess.run(playable.build_cmd(src, dest, p), check=True, capture_output=True) + return dest + + +@needs_ffmpeg +def test_a_10_bit_matroska_with_opus_comes_out_phone_safe(tmp_path): + src = tmp_path / "in.mkv" + _ffmpeg( + "-f", "lavfi", "-i", "testsrc2=size=160x120:rate=10:duration=1", + "-f", "lavfi", "-i", "sine=duration=1", + "-c:v", "libx264", "-pix_fmt", "yuv420p10le", "-c:a", "libopus", + str(src), + ) # fmt: skip + out = _convert(src, tmp_path) + data = probe.probe_json(out) + video, audio = probe.primary_streams(data) + assert (video["codec_name"], video["pix_fmt"], video["profile"]) == ("h264", "yuv420p", "High") + assert audio["codec_name"] == "aac" + assert out.suffix == ".mp4" + assert not playable.plan(data, out.suffix).needs_change # the rule accepts its own output + + +@needs_ffmpeg +def test_mp3_inside_an_mp4_comes_out_as_an_mp3(tmp_path): + # The one unsafe file prod held: an audio download remuxed into .mp4, which iPhone and iPad + # both refused to play. + src = tmp_path / "in.mp4" + _ffmpeg("-f", "lavfi", "-i", "sine=duration=1", "-c:a", "libmp3lame", "-f", "mp4", str(src)) + out = _convert(src, tmp_path) + data = probe.probe_json(out) + assert out.suffix == ".mp3" + assert probe.primary_streams(data)[1]["codec_name"] == "mp3" + assert not playable.plan(data, out.suffix).needs_change diff --git a/backend/tests/test_public_unlock_limit.py b/backend/tests/test_public_unlock_limit.py index 816575a2..d4fc12d6 100644 --- a/backend/tests/test_public_unlock_limit.py +++ b/backend/tests/test_public_unlock_limit.py @@ -20,7 +20,10 @@ _PW = "open sesame" @pytest.fixture def unlock_url(as_user, make_user, tmp_path, monkeypatch, upload_headers): monkeypatch.setattr(settings, "download_root", str(tmp_path)) - monkeypatch.setattr(probemod, "probe_media", lambda _p: ("h264", "aac", 320, 240, 5)) + video = {"codec_type": "video", "codec_name": "h264", "pix_fmt": "yuv420p"} + audio = {"codec_type": "audio", "codec_name": "aac"} + data = {"streams": [video, audio], "format": {"format_name": "mov,mp4", "duration": "5"}} + monkeypatch.setattr(probemod, "probe_json", lambda _p: data) owner = as_user(make_user()) job = owner.post( "/api/downloads/upload", content=b"fake-video", headers=upload_headers("clip.mp4") diff --git a/backend/tests/test_upload.py b/backend/tests/test_upload.py index 39de2ca2..db0699fd 100644 --- a/backend/tests/test_upload.py +++ b/backend/tests/test_upload.py @@ -7,15 +7,18 @@ guards that keep an upload out of paths that assume a re-fetchable source (edit DB-backed (real Postgres) — skipped when no database is reachable. `settings.download_root` is pointed at a per-test tmp dir so ingest's on-disk moves are isolated. The ingest/route tests -monkeypatch the shared `probe.probe_media` so they don't shell out to ffprobe; the classifier's OWN +monkeypatch the shared `probe.probe_json` so they don't shell out to ffprobe; the classifier's OWN parsing (image → file, cover-art skip, dims, duration) is covered directly by `TestProbeClassifier` -below, which stubs one level down (`run_ffprobe_json`) and exercises the real `probe_media`. +below, which stubs one level down (`run_ffprobe_json`) and exercises the real `probe_media`. The +phone-safe conversion of an upload runs the real ffmpeg (`TestPhoneSafeUpload`). """ import ast import asyncio import io import pathlib +import shutil +import subprocess from pathlib import Path import pytest @@ -25,9 +28,11 @@ from app.config import settings from app.routes._common import capped_stream_to from sqlalchemy import select -from app.downloads import mediakind, quota, storage +from app.downloads import gc as gcmod +from app.downloads import mediakind, pipeline, playable, quota, service, storage from app.downloads import probe as probemod from app.downloads import upload as uploadmod +from app.downloads.stated import StatedFailure from app.models import DISPLAY_TEXT_MAX, DownloadJob, DownloadQuota, MediaAsset @@ -51,9 +56,20 @@ def _root(tmp_path, monkeypatch): def _probe(monkeypatch, result): - # upload ingest now calls the shared `probe.probe_media` (same ground-truth ffprobe the download - # worker uses); patch it at that shared home. - monkeypatch.setattr(probemod, "probe_media", lambda _p: result) + """Stub the shared ffprobe call (`probe.probe_json`, the same ground truth the download worker + reads) with the JSON a file of these (vcodec, acodec, width, height, duration) would give — its + video 8-bit 4:2:0, so an H.264 + AAC .mp4 is already phone-safe and ingests straight to `done`. + All-None is a file ffprobe could not read.""" + v, a, w, h, d = result + streams = [] + if v: + streams.append( + {"codec_type": "video", "codec_name": v, "pix_fmt": "yuv420p", "width": w, "height": h} + ) + if a: + streams.append({"codec_type": "audio", "codec_name": a}) + data = {"streams": streams, "format": {"format_name": "mov,mp4", "duration": d}} + monkeypatch.setattr(probemod, "probe_json", lambda _p: data if streams else None) def test_ingest_generic_file_is_download_only(db, make_user, _root, monkeypatch): @@ -281,7 +297,7 @@ def test_upload_route_generic_forces_download( def test_upload_route_media_html_extension_not_served_as_html( - as_user, make_user, _root, monkeypatch, upload_headers + db, as_user, make_user, _root, monkeypatch, upload_headers ): """The XSS guard: a MEDIA-classified upload (real codecs) named with an executable extension must NOT be served with a guessed text/html type. The served Content-Type comes from the verified @@ -298,6 +314,14 @@ def test_upload_route_media_html_extension_not_served_as_html( assert r.status_code == 200, r.text job = r.json() assert job["asset"]["media_kind"] == "video" and job["asset"]["container"] == "html" + # Media in no phone-safe container is queued for conversion to .mp4, so today it is never served + # as .html at all. The serving guard below is the second line: a row from before that rule (or + # one whose conversion is skipped some day) must still not run as HTML. + assert job["status"] == "queued" + row = db.get(DownloadJob, job["id"]) + db.get(MediaAsset, row.asset_id).status = "ready" + row.status = "done" + db.commit() # Authenticated endpoint: never text/html. fr = c.get(f"/api/downloads/{job['id']}/file") @@ -1014,3 +1038,122 @@ class TestResolveMediaFacts: ) assert (v, a) == (None, "aac") assert mediakind.derive(v, a) == mediakind.AUDIO + + +# --- the phone-safe conversion (an iPhone's HEVC, a webm, an mkv) ------------------------------ + + +def _queued_upload(db, make_user, root, monkeypatch, name="clip.mov"): + """An upload no phone could play as it is: probed as HEVC, so ingest queues it.""" + _probe(monkeypatch, ("hevc", "aac", 640, 360, 6)) + src = root / "src.bin" + src.write_bytes(b"hevc bytes") + job = uploadmod.ingest_upload(db, make_user().id, filename=name, src_path=src, size=10) + return job, db.get(MediaAsset, job.asset_id) + + +class TestPhoneSafeUpload: + def test_an_unsafe_video_is_queued_for_the_worker_not_done( + self, db, make_user, _root, monkeypatch + ): + job, asset = _queued_upload(db, make_user, _root, monkeypatch) + assert (job.status, job.progress, asset.status) == ("queued", 0, "pending") + assert asset.rel_path.endswith(".mov") # the original waits at rel_path for the worker + assert storage.abs_path(str(_root), asset.rel_path).exists() + + def test_a_safe_video_still_lands_done(self, db, make_user, _root, monkeypatch): + _probe(monkeypatch, ("h264", "aac", 640, 360, 6)) + src = _root / "src.bin" + src.write_bytes(b"h264 bytes") + job = uploadmod.ingest_upload( + db, make_user().id, filename="clip.mp4", src_path=src, size=10 + ) + assert job.status == "done" and db.get(MediaAsset, job.asset_id).status == "ready" + + def test_deleting_a_queued_upload_removes_its_original(self, db, make_user, _root, monkeypatch): + job, asset = _queued_upload(db, make_user, _root, monkeypatch) + path, asset_id = storage.abs_path(str(_root), asset.rel_path), asset.id + service.delete(db, job) + assert not path.exists() + assert db.get(MediaAsset, asset_id) is None + + def test_one_cancelled_mid_conversion_is_reaped_once_the_worker_let_go( + self, db, make_user, _root, monkeypatch + ): + job, asset = _queued_upload(db, make_user, _root, monkeypatch) + path, asset_id = storage.abs_path(str(_root), asset.rel_path), asset.id + asset.status = "downloading" # the worker child is reading the original + job.status = "running" + db.commit() + service.cancel(db, job) + assert path.exists() # never pulled out from under a running ffmpeg + db.get(MediaAsset, asset_id).status = "pending" # the worker stopped the child and let go + db.commit() + gcmod.run_download_gc(db) + db.expire_all() + assert not path.exists() + assert db.get(MediaAsset, asset_id) is None + + def test_a_failed_conversion_keeps_the_original_for_retry( + self, db, make_user, _root, monkeypatch + ): + job, asset = _queued_upload(db, make_user, _root, monkeypatch) + path = storage.abs_path(str(_root), asset.rel_path) + job.status, asset.status = "running", "downloading" + db.commit() + + def _ffmpeg_fails(*_a, **_k): + raise StatedFailure("ffmpeg failed (1): unsupported") + + monkeypatch.setattr(pipeline.editmod, "run_ffmpeg", _ffmpeg_fails) + pipeline._convert_upload(job.id, asset.id) + db.expire_all() + assert (db.get(DownloadJob, job.id).status, db.get(MediaAsset, asset.id).status) == ( + "error", + "error", + ) + assert path.exists() + service.resume(db, db.get(DownloadJob, job.id)) + assert db.get(MediaAsset, asset.id).status == "pending" # Retry converts from it again + + @pytest.mark.skipif(shutil.which("ffmpeg") is None, reason="ffmpeg not available") + def test_the_worker_converts_it_and_keeps_only_the_safe_file( + self, db, make_user, _root, monkeypatch + ): + # Real bytes, real probe, real ffmpeg: a 10-bit H.264 + Opus Matroska, the kind of file + # that reached the share page as-is before this rule. + src = _root / "src.mkv" + subprocess.run( + [ + "ffmpeg", "-y", "-loglevel", "error", + "-f", "lavfi", "-i", "testsrc2=size=160x120:rate=10:duration=1", + "-f", "lavfi", "-i", "sine=duration=1", + "-c:v", "libx264", "-pix_fmt", "yuv420p10le", "-c:a", "libopus", str(src), + ], + check=True, + ) # fmt: skip + job = uploadmod.ingest_upload( + db, make_user().id, filename="holiday.mkv", src_path=src, size=src.stat().st_size + ) + asset = db.get(MediaAsset, job.asset_id) + original = storage.abs_path(str(_root), asset.rel_path) + assert job.status == "queued" + job.status, asset.status = "running", "downloading" # what the worker's claim does + db.commit() + + pipeline._convert_upload(job.id, asset.id) + + db.expire_all() + asset = db.get(MediaAsset, job.asset_id) + assert db.get(DownloadJob, job.id).status == "done" + assert (asset.status, asset.container, asset.vcodec, asset.acodec) == ( + "ready", + "mp4", + "h264", + "aac", + ) + final = storage.abs_path(str(_root), asset.rel_path) + assert final.suffix == ".mp4" and final.exists() + assert asset.size_bytes == final.stat().st_size + assert not original.exists() # one file per upload + assert not playable.plan(probemod.probe_json(final), final.suffix).needs_change diff --git a/frontend/src/components/UploadDialog.dom.test.tsx b/frontend/src/components/UploadDialog.dom.test.tsx index 4a3ca217..097d0561 100644 --- a/frontend/src/components/UploadDialog.dom.test.tsx +++ b/frontend/src/components/UploadDialog.dom.test.tsx @@ -10,7 +10,7 @@ import { render, screen, fireEvent, waitFor, cleanup } from "@testing-library/re import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; const h = vi.hoisted(() => ({ - uploadDownload: vi.fn(async (_f: File, _o: unknown) => ({ id: 42 })), + uploadDownload: vi.fn(async (_f: File, _o: unknown) => ({ id: 42, status: "done" })), // The dialog reads the per-file upload cap so it can refuse an over-cap file before spending the // user's bandwidth on it. Generous by default; the cap test below lowers it. downloadUsage: vi.fn(async () => ({ upload_max_bytes: 1_000_000 })), @@ -78,9 +78,25 @@ describe("UploadDialog", () => { expect(passedFile).toBe(f); expect((opts as { displayName?: string }).displayName).toBe("My notes"); await waitFor(() => expect(h.notify).toHaveBeenCalledTimes(1)); + expect(h.notify.mock.calls[0]![0]).toMatchObject({ message: "downloads.upload.added" }); await waitFor(() => expect(onClose).toHaveBeenCalled()); }); + it("says so when the server queued the upload for its phone-safe conversion", async () => { + // A video no phone could play as it is (an iPhone's HEVC .mov) comes back `queued`, not `done`: + // "File uploaded" alone would send the user to a Library row that is still converting. + h.uploadDownload.mockResolvedValueOnce({ id: 43, status: "queued" }); + wrap(); + fireEvent.change(fileInput(), { + target: { files: [new File(["x"], "clip.mov", { type: "video/quicktime" })] }, + }); + fireEvent.click(screen.getByText("downloads.upload.submit")); + await waitFor(() => expect(h.notify).toHaveBeenCalledTimes(1)); + expect(h.notify.mock.calls[0]![0]).toMatchObject({ + message: "downloads.upload.addedConverting", + }); + }); + it("keeps Upload disabled until a file is chosen", () => { wrap(); const btn = screen.getByText("downloads.upload.submit").closest("button")!; diff --git a/frontend/src/components/UploadDialog.tsx b/frontend/src/components/UploadDialog.tsx index d410e70e..414624e4 100644 --- a/frontend/src/components/UploadDialog.tsx +++ b/frontend/src/components/UploadDialog.tsx @@ -19,7 +19,14 @@ import { Input } from "./ui/Input"; // Local-file ingest. A general upload — ANY file type, no format list: media // (video/audio) joins the library with the full function set, a generic file is download-only. The // file is streamed to the server via XHR so this dialog can show real upload progress. On success it -// lands in the Library as an already-`done` job, so we invalidate + point the user there. +// lands in the Library as an already-`done` job — or, for media no phone could play as it is (an +// iPhone's HEVC, a webm, an mkv), as a `queued` job the worker converts first — so we invalidate, +// say which, and point the user there. +// Out here, not inline in the upload handler: a `?:` inside its `try` makes the React Compiler +// skip the whole dialog. +const uploadedKey = (status: string) => + status === "done" ? "downloads.upload.added" : "downloads.upload.addedConverting"; + export default function UploadDialog({ onClose }: { onClose: () => void }) { const { t, i18n } = useTranslation(); const lang = i18n.language; @@ -73,7 +80,7 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { setPct(0); const displayName = name.trim() || undefined; try { - await api.uploadDownload(file, { + const job = await api.uploadDownload(file, { displayName, onProgress: setPct, signal: ac.signal, @@ -81,7 +88,7 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { invalidateDownloads(qc); notify({ level: "success", - message: t("downloads.upload.added"), + message: t(uploadedKey(job.status)), action: { label: t("downloads.dialog.view"), onClick: () => navigateTo("downloads") }, }); onClose(); diff --git a/frontend/src/i18n/locales/en/downloads.json b/frontend/src/i18n/locales/en/downloads.json index a385c9ce..95684f65 100644 --- a/frontend/src/i18n/locales/en/downloads.json +++ b/frontend/src/i18n/locales/en/downloads.json @@ -36,6 +36,7 @@ "uploading": "Uploading… {{pct}}%", "uploadProgress": "Upload progress", "added": "File uploaded", + "addedConverting": "File uploaded — converting it so it plays on any phone", "failed": "Couldn't upload the file" }, "tabs": { diff --git a/frontend/src/i18n/locales/hu/downloads.json b/frontend/src/i18n/locales/hu/downloads.json index 0e9c92e9..8371a869 100644 --- a/frontend/src/i18n/locales/hu/downloads.json +++ b/frontend/src/i18n/locales/hu/downloads.json @@ -36,6 +36,7 @@ "uploading": "Feltöltés… {{pct}}%", "uploadProgress": "Feltöltés folyamata", "added": "Fájl feltöltve", + "addedConverting": "Fájl feltöltve — átalakítjuk, hogy minden telefonon lejátszható legyen", "failed": "Nem sikerült feltölteni a fájlt" }, "tabs": { diff --git a/frontend/src/lib/api/downloads.ts b/frontend/src/lib/api/downloads.ts index 2c05db1d..fcac578d 100644 --- a/frontend/src/lib/api/downloads.ts +++ b/frontend/src/lib/api/downloads.ts @@ -196,7 +196,8 @@ export const downloadsApi = { }): Promise => req("/api/downloads", { method: "POST", body: JSON.stringify(body) }), // Ingest a local file into the library. Sent via XHR rather than `req` so the dialog can // show upload progress; any file type is accepted (media gets the full function set, a generic - // file is download-only). Returns the already-`done` job the ingest creates. + // file is download-only). Returns the job the ingest creates: `done`, or `queued` while the + // worker converts media no phone could play as it is (`downloads/upload.py`). // The body IS the file, and the two names ride as headers. Not a style choice: a multipart part is // a DECLARED body, which FastAPI reads and spools BEFORE the route's `current_user` dependency // runs — so with the old shape an unauthenticated caller could make the server write the whole