fix(downloads): address code-review findings on the R2.5 mini-epic
- redownload: never clobber a co-held (deduped) asset. Drop the force-reset-to-pending; _release_asset already deletes+recreates a solo asset (genuine re-fetch), while a shared ready asset is left intact and the re-queued job re-points to it via _finish_from_cache — so one user's re-download can't delete another's file mid-serve (finding #1). - redownload: reset queue_pos to the tail so a re-download doesn't front-run newer queued jobs on the worker's `ORDER BY queue_pos` claim (finding #3). Made service.next_queue_pos public. - formats: video-only mp4 selector prefers every video-only option before any progressive `best`, so a "Video only" download stays audio-free whenever a video-only stream exists (#2). - titles: a title that is only the engagement prefix no longer resurrects the stripped stats (return the stripped text / empty, not the raw) (#4). - worker: probe the file's duration when yt-dlp gave none, so a long re-encode shows real progress instead of a frozen 0% (#5); tighten the ensure_browser_playable docstring to its actual mp4-profile scope (#6).
This commit is contained in:
@@ -303,6 +303,20 @@ def probe_codecs(path: Path) -> tuple[str | None, str | None]:
|
|||||||
return vcodec, acodec
|
return vcodec, acodec
|
||||||
|
|
||||||
|
|
||||||
|
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)."""
|
||||||
|
try:
|
||||||
|
out = subprocess.run(
|
||||||
|
["ffprobe", "-v", "error", "-show_entries", "format=duration",
|
||||||
|
"-of", "default=nw=1:nk=1", str(path)],
|
||||||
|
capture_output=True, text=True, timeout=30, check=True,
|
||||||
|
).stdout.strip()
|
||||||
|
return int(float(out)) if out else 0
|
||||||
|
except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError, ValueError):
|
||||||
|
return 0
|
||||||
|
|
||||||
|
|
||||||
def browser_transcode_flags(vcodec: str | None, acodec: str | None, is_mp4: bool) -> tuple[bool, bool, bool]:
|
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
|
"""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
|
(needs_change, reencode_video, reencode_audio). needs_change=False => already H.264 + AAC/MP3
|
||||||
|
|||||||
@@ -152,9 +152,12 @@ def build_ydl_opts(
|
|||||||
compat = s["container"] == "mp4" and s["vcodec"] is None
|
compat = s["container"] == "mp4" and s["vcodec"] is None
|
||||||
if s["mode"] == "v":
|
if s["mode"] == "v":
|
||||||
if compat:
|
if compat:
|
||||||
|
# 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 — same audio-free-when-possible guarantee as the old `bestvideo/best`.
|
||||||
opts["format"] = (
|
opts["format"] = (
|
||||||
f"bestvideo[vcodec^=avc1]{hcap}/best[vcodec^=avc1]{hcap}"
|
f"bestvideo[vcodec^=avc1]{hcap}/bestvideo{hcap}"
|
||||||
f"/bestvideo{hcap}/best{hcap}"
|
f"/best[vcodec^=avc1]{hcap}/best{hcap}"
|
||||||
)
|
)
|
||||||
else:
|
else:
|
||||||
opts["format"] = f"bestvideo{hcap}/best{hcap}"
|
opts["format"] = f"bestvideo{hcap}/best{hcap}"
|
||||||
|
|||||||
@@ -106,7 +106,7 @@ def populate_from_catalog(db: Session, asset: MediaAsset) -> bool:
|
|||||||
return True
|
return True
|
||||||
|
|
||||||
|
|
||||||
def _next_queue_pos(db: Session) -> int:
|
def next_queue_pos(db: Session) -> int:
|
||||||
return (db.execute(select(func.coalesce(func.max(DownloadJob.queue_pos), 0))).scalar() or 0) + 1
|
return (db.execute(select(func.coalesce(func.max(DownloadJob.queue_pos), 0))).scalar() or 0) + 1
|
||||||
|
|
||||||
|
|
||||||
@@ -138,7 +138,7 @@ def enqueue(
|
|||||||
profile_snapshot=spec,
|
profile_snapshot=spec,
|
||||||
display_name=display_name or (asset.title[:255] if asset.status == "ready" and asset.title else None),
|
display_name=display_name or (asset.title[:255] if asset.status == "ready" and asset.title else None),
|
||||||
status="queued",
|
status="queued",
|
||||||
queue_pos=_next_queue_pos(db),
|
queue_pos=next_queue_pos(db),
|
||||||
)
|
)
|
||||||
db.add(job)
|
db.add(job)
|
||||||
asset.ref_count = (asset.ref_count or 0) + 1
|
asset.ref_count = (asset.ref_count or 0) + 1
|
||||||
@@ -207,7 +207,7 @@ def enqueue_edit(
|
|||||||
profile_snapshot={},
|
profile_snapshot={},
|
||||||
display_name=display_name or asset.title,
|
display_name=display_name or asset.title,
|
||||||
status="queued",
|
status="queued",
|
||||||
queue_pos=_next_queue_pos(db),
|
queue_pos=next_queue_pos(db),
|
||||||
)
|
)
|
||||||
db.add(job)
|
db.add(job)
|
||||||
asset.ref_count = (asset.ref_count or 0) + 1
|
asset.ref_count = (asset.ref_count or 0) + 1
|
||||||
|
|||||||
@@ -465,17 +465,15 @@ def redownload_download(
|
|||||||
if job.status in ("queued", "running", "paused"):
|
if job.status in ("queued", "running", "paused"):
|
||||||
raise HTTPException(status_code=409, detail="This download is still in progress.")
|
raise HTTPException(status_code=409, detail="This download is still in progress.")
|
||||||
|
|
||||||
# Drop this job's hold on its current asset (deletes the file if it was the last holder), then
|
# Drop this job's hold on its current asset. `_release_asset` deletes the file + row when THIS
|
||||||
# resolve the asset for the current spec and force a genuinely fresh fetch even on a cache hit.
|
# job was the last holder (ref→0, ready) — so a solo re-download genuinely re-fetches. When the
|
||||||
|
# asset is still shared by another job/user we must NOT delete it out from under them: leaving it
|
||||||
|
# `ready` means the re-queued job just re-points to the shared file (the worker short-circuits a
|
||||||
|
# ready asset via `_finish_from_cache`), which is why there's no force-reset-to-pending here.
|
||||||
_release_asset(db, job)
|
_release_asset(db, job)
|
||||||
spec = formats.normalize(job.profile_snapshot)
|
spec = formats.normalize(job.profile_snapshot)
|
||||||
sig = formats.format_sig(spec)
|
sig = formats.format_sig(spec)
|
||||||
asset = service.get_or_create_asset(db, job.source_kind, job.source_ref, sig)
|
asset = service.get_or_create_asset(db, job.source_kind, job.source_ref, sig)
|
||||||
if asset.status == "ready":
|
|
||||||
if asset.rel_path:
|
|
||||||
storage.delete_asset_files(settings.download_root, asset.rel_path)
|
|
||||||
asset.rel_path = None
|
|
||||||
asset.status = "pending"
|
|
||||||
service.populate_from_catalog(db, asset)
|
service.populate_from_catalog(db, asset)
|
||||||
|
|
||||||
asset.ref_count = (asset.ref_count or 0) + 1
|
asset.ref_count = (asset.ref_count or 0) + 1
|
||||||
@@ -486,6 +484,7 @@ def redownload_download(
|
|||||||
job.error = None
|
job.error = None
|
||||||
job.speed_bps = None
|
job.speed_bps = None
|
||||||
job.eta_s = None
|
job.eta_s = None
|
||||||
|
job.queue_pos = service.next_queue_pos(db) # tail of the queue, not its stale original slot
|
||||||
db.commit()
|
db.commit()
|
||||||
return _serialize_job(db, job)
|
return _serialize_job(db, job)
|
||||||
|
|
||||||
|
|||||||
@@ -58,8 +58,9 @@ def normalize_title(raw: str | None) -> str | None:
|
|||||||
return raw
|
return raw
|
||||||
t = unicodedata.normalize("NFKC", raw)
|
t = unicodedata.normalize("NFKC", raw)
|
||||||
t = "".join(ch for ch in t if unicodedata.category(ch) not in _DROP_CATEGORIES)
|
t = "".join(ch for ch in t if unicodedata.category(ch) not in _DROP_CATEGORIES)
|
||||||
t = _ENGAGEMENT_PREFIX.sub("", t)
|
stripped = _ENGAGEMENT_PREFIX.sub("", t)
|
||||||
t = _TRAILING_HASHTAGS.sub("", t)
|
prefix_removed = stripped != t
|
||||||
|
t = _TRAILING_HASHTAGS.sub("", stripped)
|
||||||
|
|
||||||
multi = [r for r in _LETTER_RUN.findall(t) if len(r) >= 2]
|
multi = [r for r in _LETTER_RUN.findall(t) if len(r) >= 2]
|
||||||
caps = [r for r in multi if r.isupper()]
|
caps = [r for r in multi if r.isupper()]
|
||||||
@@ -87,4 +88,7 @@ def normalize_title(raw: str | None) -> str | None:
|
|||||||
if ch.isalpha():
|
if ch.isalpha():
|
||||||
t = t[:i] + ch.upper() + t[i + 1:]
|
t = t[:i] + ch.upper() + t[i + 1:]
|
||||||
break
|
break
|
||||||
return t or raw
|
# Nothing survived cleaning: fall back to the engagement-stripped text (never resurrect a
|
||||||
|
# stats prefix). Only use the untouched raw when no prefix was there to strip — e.g. an
|
||||||
|
# all-emoji title whose glyphs were dropped, where returning the original is the lesser evil.
|
||||||
|
return t or stripped.strip() or ("" if prefix_removed else raw)
|
||||||
|
|||||||
+10
-6
@@ -399,11 +399,12 @@ def _download_settings() -> tuple[str, int]:
|
|||||||
|
|
||||||
|
|
||||||
def _ensure_browser_playable(job_id: int, produced: Path, spec: dict, info: dict) -> Path:
|
def _ensure_browser_playable(job_id: int, produced: Path, spec: dict, info: dict) -> Path:
|
||||||
"""Guarantee the stored file plays in EVERY browser: H.264 + AAC/MP3 in mp4. If the download
|
"""Make an mp4-compat-profile download play in every browser: H.264 + AAC/MP3 in mp4. If the
|
||||||
(whatever the source shipped) isn't already that, re-encode the offending stream(s) — or just
|
download (whatever the source shipped) isn't already that, re-encode the offending stream(s) —
|
||||||
remux when the codecs are fine but the container isn't. Gated on the mp4 compat profile, so an
|
or just remux when the codecs are fine but the container isn't. **Scope:** only the mp4 compat
|
||||||
explicit `vcodec` or non-mp4 custom profile opts out. Returns the file to store (converted, or
|
profile is normalized; an explicit `vcodec` or a non-mp4 custom profile is the user opting out
|
||||||
the original) and corrects `info`'s codec/container fields to match what's actually on disk."""
|
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."""
|
||||||
if spec.get("container") != "mp4" or spec.get("vcodec"):
|
if spec.get("container") != "mp4" or spec.get("vcodec"):
|
||||||
return produced
|
return produced
|
||||||
vcodec, acodec = editmod.probe_codecs(produced)
|
vcodec, acodec = editmod.probe_codecs(produced)
|
||||||
@@ -416,9 +417,12 @@ def _ensure_browser_playable(job_id: int, produced: Path, spec: dict, info: dict
|
|||||||
action = "re-encode" if (reenc_v or reenc_a) else "remux"
|
action = "re-encode" if (reenc_v or reenc_a) else "remux"
|
||||||
log.info("job %s: %s (v=%s a=%s mp4=%s) -> browser-safe mp4", job_id, action, vcodec, acodec, is_mp4)
|
log.info("job %s: %s (v=%s a=%s mp4=%s) -> browser-safe mp4", job_id, action, vcodec, acodec, is_mp4)
|
||||||
_set_job(job_id, phase="processing", progress=0, speed_bps=None, eta_s=None)
|
_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)
|
||||||
editmod.run_ffmpeg(
|
editmod.run_ffmpeg(
|
||||||
editmod.build_browser_safe_cmd(produced, dest, reenc_v, reenc_a),
|
editmod.build_browser_safe_cmd(produced, dest, reenc_v, reenc_a),
|
||||||
total_s=int(info["duration"]) if info.get("duration") else 0,
|
total_s=total_s,
|
||||||
on_progress=lambda pct: _set_job(job_id, progress=int(pct), phase="processing"),
|
on_progress=lambda pct: _set_job(job_id, progress=int(pct), phase="processing"),
|
||||||
should_cancel=lambda: _job_status(job_id) not in ("running", None),
|
should_cancel=lambda: _job_status(job_id) not in ("running", None),
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -26,6 +26,9 @@ def test_video_only_mp4_selects_avc1():
|
|||||||
fmt = _opts({"mode": "v", "container": "mp4"})["format"]
|
fmt = _opts({"mode": "v", "container": "mp4"})["format"]
|
||||||
assert fmt.startswith("bestvideo[vcodec^=avc1]")
|
assert fmt.startswith("bestvideo[vcodec^=avc1]")
|
||||||
assert "+bestaudio" not in fmt # video-only never merges audio
|
assert "+bestaudio" not in fmt # video-only never merges audio
|
||||||
|
# A video-only download stays audio-free: every `bestvideo` (video-only) option is preferred
|
||||||
|
# over any `best` (progressive, audio-bearing) fallback.
|
||||||
|
assert fmt.index("bestvideo") < fmt.index("best[vcodec^=avc1]") < fmt.rindex("/best")
|
||||||
|
|
||||||
|
|
||||||
def test_max_height_caps_every_branch():
|
def test_max_height_caps_every_branch():
|
||||||
|
|||||||
@@ -61,3 +61,10 @@ def test_engagement_prefix_does_not_touch_normal_titles():
|
|||||||
# A pipe that isn't an engagement prefix must survive.
|
# A pipe that isn't an engagement prefix must survive.
|
||||||
assert normalize_title("Vlog 12 | A nagy nap") == "Vlog 12 | A nagy nap"
|
assert normalize_title("Vlog 12 | A nagy nap") == "Vlog 12 | A nagy nap"
|
||||||
assert normalize_title("How I built a boat") == "How I built a boat"
|
assert normalize_title("How I built a boat") == "How I built a boat"
|
||||||
|
|
||||||
|
|
||||||
|
def test_engagement_only_title_does_not_resurrect_the_stats():
|
||||||
|
# A title that is ONLY the stats prefix must not fall back to the raw (stats-bearing) string.
|
||||||
|
out = normalize_title("1.6M views · 66K reactions |")
|
||||||
|
assert out == ""
|
||||||
|
assert "views" not in (out or "") and "reactions" not in (out or "")
|
||||||
|
|||||||
Reference in New Issue
Block a user