kenzuya/unshackle
1
0
Fork 2
mirror of https://github.com/unshackle-dl/unshackle.git synced 2026-06-10 03:02:09 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(dl): make a failed subtitle non-fatal under --skip-subtitle-errors

Browse Source 
A single failing subtitle track previously aborted the whole download. Add an opt-in
--skip-subtitle-errors flag: when set, a Subtitle failure is logged and the track dropped from
the mux while the video/audio still complete (Video/Audio failures stay fatal; default
behaviour is unchanged).

Done at the right layer to avoid the shared-event race: a failed track sets the process-global
DOWNLOAD_CANCELLED event, which makes other in-flight tracks early-return without raising — so a
skipped subtitle could otherwise silently truncate the video/audio that still got muxed. The
download is split into two passes (download_tracks_in_passes): the fatal tracks download
concurrently first, then the skippable subtitles run in a separate sequential pass once nothing
else is in flight, with the event reset before each and at the start of every title. Skipped
languages are recorded on the dl instance (skipped_subtitles) for callers to surface.

Adds tests for the cancel-event interaction (a failing subtitle no longer truncates the
video/audio), the good/bad subtitle mix, the flag-off fatal path, and the per-title reset.
This commit is contained in:
Avi Cohen
2026-06-04 07:55:03 +03:00
parent 3dcc584b2f
commit 41fef5d33a
2 changed files with 230 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files

141
tests/tracks/test_subtitle_skip.py Normal file
View File

@@ -0,0 +1,141 @@
"""Tests for ``download_tracks_in_passes`` — the two-pass track download used by
``dl.result()`` when ``--skip-subtitle-errors`` is set.
The behaviour under test is the cancel-event interaction: a failed track sets the
process-global ``DOWNLOAD_CANCELLED`` event, which makes other in-flight tracks
early-return without raising. Running the fatal video/audio tracks to completion
*before* the skippable subtitles removes that race, so a subtitle failure can no
longer truncate the video/audio that still gets muxed.
"""
from __future__ import annotations
import pytest
from unshackle.commands.dl import download_tracks_in_passes
from unshackle.core.constants import DOWNLOAD_CANCELLED
from unshackle.core.tracks import Audio, Subtitle, Video
def make_video(track_id: str = "v") -> Video:
return Video(
id_=track_id,
url=f"https://example.test/{track_id}.m3u8",
language="en",
codec=Video.Codec.AVC,
range_=Video.Range.SDR,
width=1920,
height=1080,
bitrate=5_000_000,
)
def make_audio(track_id: str = "a") -> Audio:
return Audio(
id_=track_id,
url=f"https://example.test/{track_id}.m3u8",
language="en",
codec=Audio.Codec.AAC,
bitrate=128_000,
)
def make_subtitle(track_id: str, language: str) -> Subtitle:
return Subtitle(
id_=track_id,
url=f"https://example.test/{track_id}.vtt",
language=language,
codec=Subtitle.Codec.WebVTT,
)
class Harness:
"""Mimics how ``dl.result`` drives the helper, with a controllable downloader.
``run_one`` mirrors ``Track.download``: it early-returns (without recording a
completion) when the cancel event is already set, and a track flagged to fail
sets the event and raises — exactly what the real cancel sites do.
"""
def __init__(self, fail_ids: set[str]):
self.fail_ids = fail_ids
self.completed: list[str] = []
self.early_returned: list[str] = []
self.skipped: list[Subtitle] = []
def run_one(self, track, _index):
if DOWNLOAD_CANCELLED.is_set():
self.early_returned.append(track.id)
return
if track.id in self.fail_ids:
DOWNLOAD_CANCELLED.set()
raise RuntimeError(f"{track.id} failed")
self.completed.append(track.id)
def on_subtitle_skipped(self, track):
self.skipped.append(track)
@pytest.fixture(autouse=True)
def _clear_event():
DOWNLOAD_CANCELLED.clear()
yield
DOWNLOAD_CANCELLED.clear()
def test_failed_subtitle_does_not_truncate_video_or_audio():
"""A subtitle that fails *and sets the cancel event* must not stop the video/audio."""
video, audio = make_video(), make_audio()
sub = make_subtitle("s-he", "he")
h = Harness(fail_ids={"s-he"})
download_tracks_in_passes(
[video, audio, sub], 4, h.run_one,
skip_subtitle_errors=True, on_subtitle_skipped=h.on_subtitle_skipped,
)
assert set(h.completed) == {"v", "a"} # both fatal tracks fully downloaded
assert h.early_returned == [] # nothing early-returned because of the subtitle
assert h.skipped == [sub] # the subtitle was recorded as skipped
def test_good_subtitle_kept_bad_subtitle_skipped():
video = make_video()
good, bad = make_subtitle("s-en", "en"), make_subtitle("s-fr", "fr")
h = Harness(fail_ids={"s-fr"})
download_tracks_in_passes(
[video, good, bad], 4, h.run_one,
skip_subtitle_errors=True, on_subtitle_skipped=h.on_subtitle_skipped,
)
assert "s-en" in h.completed # the available subtitle still downloaded
assert h.skipped == [bad] # only the failing one was skipped
def test_subtitle_failure_stays_fatal_without_flag():
"""Default behaviour (flag off) is unchanged: a subtitle failure aborts the title."""
video = make_video()
sub = make_subtitle("s-he", "he")
h = Harness(fail_ids={"s-he"})
with pytest.raises(RuntimeError):
download_tracks_in_passes(
[video, sub], 4, h.run_one,
skip_subtitle_errors=False, on_subtitle_skipped=h.on_subtitle_skipped,
)
def test_cancel_event_is_reset_between_titles():
"""A cancel left set by a previous title must not skip this title's tracks."""
DOWNLOAD_CANCELLED.set()
video, audio = make_video(), make_audio()
h = Harness(fail_ids=set())
download_tracks_in_passes(
[video, audio], 4, h.run_one,
skip_subtitle_errors=True, on_subtitle_skipped=h.on_subtitle_skipped,
)
assert set(h.completed) == {"v", "a"}
assert h.early_returned == []

71
unshackle/commands/dl.py
View File

@@ -42,7 +42,7 @@ from unshackle.core.cdm import DecryptLabsRemoteCDM
from unshackle.core.cdm.detect import is_playready_cdm, is_widevine_cdm
from unshackle.core.config import config
from unshackle.core.console import console
from unshackle.core.constants import DOWNLOAD_LICENCE_ONLY, AnyTrack, context_settings
from unshackle.core.constants import DOWNLOAD_CANCELLED, DOWNLOAD_LICENCE_ONLY, AnyTrack, context_settings
from unshackle.core.credential import Credential
from unshackle.core.drm import DRM_T, MonaLisa, PlayReady, Widevine
from unshackle.core.events import events
@@ -70,6 +70,41 @@ from unshackle.core.utils.subprocess import ffprobe
from unshackle.core.vaults import Vaults
def download_tracks_in_passes(tracks, max_concurrent, run_one, *, skip_subtitle_errors, on_subtitle_skipped):
"""Download a title's tracks, keeping a skippable subtitle failure from corrupting the rest.
A failed track sets the process-global ``DOWNLOAD_CANCELLED`` event, which makes other
in-flight tracks early-return without raising — so catching a subtitle failure after the fact
can leave a half-downloaded video/audio that still gets muxed. When ``skip_subtitle_errors``
is set the fatal tracks (video/audio) are therefore downloaded concurrently first, and the
skippable subtitles run in a separate sequential pass once nothing else is in flight, with the
event reset before each so one failure can't poison the next. Video/Audio failures stay fatal.
The event is also reset up front so a cancel from a previous title can't carry over.
``run_one(track, index)`` downloads a single track; ``on_subtitle_skipped(track)`` records a
subtitle whose download raised (the exception is handled here).
"""
DOWNLOAD_CANCELLED.clear()
indexed = list(enumerate(tracks))
if skip_subtitle_errors:
primary = [(i, t) for i, t in indexed if not isinstance(t, Subtitle)]
skippable = [(i, t) for i, t in indexed if isinstance(t, Subtitle)]
else:
primary, skippable = indexed, []
with ThreadPoolExecutor(max_concurrent) as pool:
future_to_track = {pool.submit(run_one, track, i): track for i, track in primary}
for download in futures.as_completed(future_to_track):
download.result() # a video/audio failure is fatal
for i, track in skippable:
DOWNLOAD_CANCELLED.clear()
try:
run_one(track, i)
except Exception:
on_subtitle_skipped(track)
class dl:
@staticmethod
def truncate_pssh_for_display(pssh_string: str, drm_type: str) -> str:
@@ -474,6 +509,9 @@ class dl:
@click.option("-S", "--subs-only", is_flag=True, default=False, help="Only download subtitle tracks.")
@click.option("-C", "--chapters-only", is_flag=True, default=False, help="Only download chapter markers.")
@click.option("-ns", "--no-subs", is_flag=True, default=False, help="Do not download subtitle tracks.")
@click.option("--skip-subtitle-errors", is_flag=True, default=False,
help="If a subtitle track fails to download, skip it and continue instead of "
"aborting the whole title (video/audio failures stay fatal).")
@click.option("-na", "--no-audio", is_flag=True, default=False, help="Do not download audio tracks.")
@click.option("-nc", "--no-chapters", is_flag=True, default=False, help="Do not download chapter markers.")
@click.option("-nv", "--no-video", is_flag=True, default=False, help="Do not download video tracks.")
@@ -609,6 +647,9 @@ class dl:
self.log = logging.getLogger("download")
self.completed_files: list[Path] = []
# Subtitles skipped under --skip-subtitle-errors, recorded so an embedding caller can
# report which weren't available without parsing the console output.
self.skipped_subtitles: list[str] = []
if not config.output_template:
raise click.ClickException(
@@ -1051,6 +1092,7 @@ class dl:
subs_only: bool,
chapters_only: bool,
no_subs: bool,
skip_subtitle_errors: bool,
no_audio: bool,
no_chapters: bool,
no_video: bool,
@@ -2205,11 +2247,9 @@ class dl:
try:
with Live(Padding(download_table, (1, 5)), console=console, refresh_per_second=5):
with ThreadPoolExecutor(downloads) as pool:
for download in futures.as_completed(
(
pool.submit(
track.download,
def download_track(track, i):
track.download(
session=track.session or service.session,
no_proxy_download=no_proxy_download,
prepare_drm=partial(
@@ -2236,10 +2276,23 @@ class dl:
max_workers=workers,
progress=tracks_progress_callables[i],
)
for i, track in enumerate(title.tracks)
def on_subtitle_skipped(track):
lang = str(getattr(track, "language", "") or "")
self.log.warning(f"Subtitle {lang} failed to download, skipping it.")
self.skipped_subtitles.append(lang)
try:
title.tracks.subtitles.remove(track)
except (ValueError, AttributeError):
pass
download_tracks_in_passes(
title.tracks,
downloads,
download_track,
skip_subtitle_errors=skip_subtitle_errors,
on_subtitle_skipped=on_subtitle_skipped,
)
):
download.result()
except KeyboardInterrupt:
console.print(Padding(":x: Download Cancelled...", (0, 5, 1, 5)))