diff --git a/core/metadata/art_apply.py b/core/metadata/art_apply.py index cee020ed..0da1dcea 100644 --- a/core/metadata/art_apply.py +++ b/core/metadata/art_apply.py @@ -17,7 +17,7 @@ from __future__ import annotations import contextlib import errno import os -from typing import Iterable +from typing import Iterable, Optional from core.metadata.artwork import download_cover_art, embed_album_art_metadata from core.metadata.common import get_mutagen_symbols @@ -81,6 +81,38 @@ def _audio_has_art(audio, symbols) -> bool: return False +def extract_embedded_art(file_path: str) -> Optional[bytes]: + """Return the first embedded cover-art image bytes from an audio file, or + None. Used to write a cover.jpg sidecar from the album's OWN art — no API + call, and the sidecar matches what's embedded (#813/Sokhi).""" + if not file_path or not os.path.isfile(file_path): + return None + symbols = get_mutagen_symbols() + if not symbols: + return None + try: + audio = symbols.File(file_path) + if audio is None: + return None + pics = getattr(audio, "pictures", None) # FLAC / Ogg + if pics: + return bytes(pics[0].data) + if isinstance(audio, symbols.MP4): + covr = audio.get("covr") + if covr: + return bytes(covr[0]) + tags = getattr(audio, "tags", None) + if tags is not None: + with contextlib.suppress(Exception): + if isinstance(tags, symbols.ID3): + apics = tags.getall("APIC") + if apics: + return bytes(apics[0].data) + except Exception as exc: + logger.debug("embedded-art extract failed for %s: %s", file_path, exc) + return None + + def album_has_art_on_disk(rep_file_path: str) -> bool: """Does this album have art on disk? @@ -167,24 +199,41 @@ def apply_art_to_album_files( result["failed"] += 1 target_dir = folder or (os.path.dirname(paths[0]) if paths else None) - if target_dir and os.path.isdir(target_dir): - # download_cover_art swallows its own write errors, so a read-only mount - # (EROFS) on the cover.jpg sidecar would otherwise go undetected here — - # the exact gap that left cover-only albums reporting success on a - # read-only filesystem (Sokhi: tracks already had embedded art, so the - # embed loop above never tripped EROFS). Pass a capture dict and read - # the read-only flag it sets back. - cover_ctx = context if isinstance(context, dict) else {} - try: - # force=True: the Cover Art Filler always writes the cover.jpg sidecar, - # regardless of the import-time "Download cover.jpg" toggle — running - # the art filler is an explicit request for the complete art (Sokhi). - download_cover_art(album_info, target_dir, cover_ctx, force=True) - result["cover_written"] = folder_has_cover_sidecar(target_dir) - except Exception as exc: - if getattr(exc, "errno", None) == errno.EROFS: + if target_dir and os.path.isdir(target_dir) and not folder_has_cover_sidecar(target_dir): + # Prefer the album's OWN embedded art for the cover.jpg sidecar: it's + # always present once the files are arted (we may have just embedded it), + # needs no API call, and the sidecar matches the files exactly + # (#813/Sokhi: files have art, just no cover.jpg). Fall back to a fresh + # download only when there's nothing embedded to extract. + cover_path = os.path.join(target_dir, "cover.jpg") + art_bytes = None + for fp in paths: + art_bytes = extract_embedded_art(fp) + if art_bytes: + break + if art_bytes: + try: + with open(cover_path, "wb") as handle: + handle.write(art_bytes) + result["cover_written"] = True + except OSError as exc: + if getattr(exc, "errno", None) == errno.EROFS: + result["read_only_fs"] = True + logger.warning("cover.jpg sidecar write failed for %s: %s", target_dir, exc) + + if not result["cover_written"] and not result["read_only_fs"]: + # No embedded art to extract → fetch it. download_cover_art swallows + # its own write errors, so it records read-only on the context dict + # (EROFS detection gap, Sokhi). force=True bypasses the import-time + # "Download cover.jpg" toggle — running the filler is an explicit ask. + cover_ctx = context if isinstance(context, dict) else {} + try: + download_cover_art(album_info, target_dir, cover_ctx, force=True) + result["cover_written"] = folder_has_cover_sidecar(target_dir) + except Exception as exc: + if getattr(exc, "errno", None) == errno.EROFS: + result["read_only_fs"] = True + logger.warning("cover.jpg write failed for %s: %s", target_dir, exc) + if cover_ctx.get("_cover_read_only"): result["read_only_fs"] = True - logger.warning("cover.jpg write failed for %s: %s", target_dir, exc) - if cover_ctx.get("_cover_read_only"): - result["read_only_fs"] = True return result diff --git a/core/repair_jobs/missing_cover_art.py b/core/repair_jobs/missing_cover_art.py index a4477c00..dba2c7fc 100644 --- a/core/repair_jobs/missing_cover_art.py +++ b/core/repair_jobs/missing_cover_art.py @@ -3,7 +3,7 @@ import os import re -from core.metadata.art_apply import album_has_art_on_disk +from core.metadata.art_apply import file_has_embedded_art, folder_has_cover_sidecar from core.library.path_resolver import resolve_library_file_path from core.metadata_service import get_client_for_source, get_primary_source, get_source_priority from core.repair_jobs import register_job @@ -177,18 +177,33 @@ class MissingCoverArtJob(RepairJob): config_manager=context.config_manager, ) if rep_path else None has_local = bool(resolved_rep) - disk_missing = has_local and not album_has_art_on_disk(resolved_rep) - # An album whose FILES already have art is not "missing cover art", - # even if the DB thumb_url cache happens to be empty — the library - # shows the file art either way. So for a local album, flag only when - # the files actually lack art. db_missing alone matters only for - # media-server-only albums (no local files), where the DB thumb is - # the sole art. Without this, every local album with an empty - # thumb_url got flagged despite having perfectly good embedded art - # (Boulder: "flags every album, but everything has art"). + # Check embedded art and the cover.jpg sidecar SEPARATELY (not the + # combined album_has_art_on_disk, which returns True if EITHER is + # present). An album can have embedded art but no cover.jpg — and if + # the user wants cover.jpg files, that's still a fixable "missing" + # (Sokhi: scans returned 0 because embedded-art albums were treated + # as fully arted and skipped, so their cover.jpg never got written). + has_embedded = file_has_embedded_art(resolved_rep) if has_local else False + has_sidecar = folder_has_cover_sidecar(os.path.dirname(resolved_rep)) if has_local else False + # cover.jpg sidecars are only a "missing" thing when the user has + # cover.jpg writing enabled (Boulder: "only scan for cover.jpgs when + # they're enabled"). Default on. + cover_sidecar_enabled = bool( + context.config_manager.get('metadata_enhancement.cover_art_download', True) + if context.config_manager else True) + if has_local: - needs_fix = disk_missing + embed_missing = not has_embedded + sidecar_missing = cover_sidecar_enabled and not has_sidecar + # has_embedded + no sidecar → the apply writes cover.jpg from the + # existing embedded art, so it's fixable even if the API finds no + # art. embed_missing still requires API art (nothing to embed). + sidecar_from_embedded = sidecar_missing and has_embedded + needs_fix = embed_missing or sidecar_missing else: + # Media-server-only album: the DB thumb is the only art. + embed_missing = db_missing + sidecar_from_embedded = False needs_fix = db_missing if not needs_fix: result.skipped += 1 @@ -226,10 +241,14 @@ class MissingCoverArtJob(RepairJob): if artwork_url: break - if artwork_url: + # Fixable if we found API art (to embed/write), OR it's just a + # missing cover.jpg on an album that already has embedded art — the + # apply writes the sidecar from that embedded art, no API art needed. + if artwork_url or sidecar_from_embedded: if context.report_progress: context.report_progress( - log_line=f'Found art: {title or "Unknown"}', + log_line=(f'Found art: {title or "Unknown"}' if artwork_url + else f'Will write cover.jpg from embedded art: {title or "Unknown"}'), log_type='success' ) # Also search for an artist image so the finding can offer it as @@ -241,6 +260,9 @@ class MissingCoverArtJob(RepairJob): # Create finding for user to approve if context.create_finding: try: + _desc = (f'Album "{title}" by {artist_name or "Unknown"} has no cover art. ' + + ('Found artwork from API.' if artwork_url + else 'Will write cover.jpg from the existing embedded art.')) inserted = context.create_finding( job_id=self.job_id, finding_type='missing_cover_art', @@ -249,7 +271,7 @@ class MissingCoverArtJob(RepairJob): entity_id=str(album_id), file_path=None, title=f'Missing artwork: {title or "Unknown"}', - description=f'Album "{title}" by {artist_name or "Unknown"} has no cover art. Found artwork from API.', + description=_desc, details={ 'album_id': album_id, 'album_title': title, @@ -268,7 +290,8 @@ class MissingCoverArtJob(RepairJob): # apply can embed into the audio + write cover.jpg. 'album_folder': os.path.dirname(rep_path) if rep_path else None, 'db_missing': db_missing, - 'disk_missing': disk_missing, + 'embed_missing': embed_missing, + 'sidecar_from_embedded': sidecar_from_embedded, 'musicbrainz_release_id': None, } ) diff --git a/core/repair_worker.py b/core/repair_worker.py index 58dc2517..dabc96fc 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -1325,7 +1325,11 @@ class RepairWorker: artist_result = self._fix_artist_art(album_id, details) artwork_url = details.get('found_artwork_url') - if not artwork_url: + # sidecar_from_embedded: the album already has embedded art and just needs + # a cover.jpg sidecar — the apply writes it from the existing embedded art, + # so no API artwork_url is required (Sokhi #813). + sidecar_from_embedded = bool(details.get('sidecar_from_embedded')) + if not artwork_url and not sidecar_from_embedded: # 'both' but no album art — report the artist outcome if that ran. if artist_result is not None: return artist_result diff --git a/tests/test_art_apply.py b/tests/test_art_apply.py index 4e3d2470..e070d522 100644 --- a/tests/test_art_apply.py +++ b/tests/test_art_apply.py @@ -247,3 +247,70 @@ def test_filler_forces_cover_sidecar_write(tmp_path, monkeypatch): aa.apply_art_to_album_files([str(f)], {}, {}, folder=str(tmp_path)) assert captured.get('force') is True + + +# ── cover.jpg from embedded art (#813/Sokhi: files have art, no sidecar) ── + +def test_extract_embedded_art_flac(tmp_path, monkeypatch): + f = tmp_path / 'a.flac'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=[SimpleNamespace(data=b'IMGBYTES')], tags=None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + assert aa.extract_embedded_art(str(f)) == b'IMGBYTES' + + +def test_extract_embedded_art_none_when_no_pictures(tmp_path, monkeypatch): + f = tmp_path / 'a.flac'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=[], tags=None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + assert aa.extract_embedded_art(str(f)) is None + + +def test_apply_writes_cover_jpg_from_embedded_art(tmp_path, monkeypatch): + # Files already have embedded art, no cover.jpg → apply extracts it and + # writes the sidecar WITHOUT an API fetch (consistent + offline). + f = tmp_path / '01.flac'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=[SimpleNamespace(data=b'EMBEDDED')], tags=None, save=lambda: None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda *a, **k: True) + dl_called = [] + monkeypatch.setattr(aa, 'download_cover_art', lambda *a, **k: dl_called.append(1)) + + res = aa.apply_art_to_album_files([str(f)], {}, {}, folder=str(tmp_path)) + + assert (tmp_path / 'cover.jpg').read_bytes() == b'EMBEDDED' + assert res['cover_written'] is True + assert dl_called == [] # used embedded art — no API call + + +def test_apply_falls_back_to_download_when_no_embedded(tmp_path, monkeypatch): + # No embedded art to extract → fetch the sidecar via download_cover_art. + f = tmp_path / '01.flac'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=[], tags=None, add_tags=lambda: None, save=lambda: None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda *a, **k: True) + dl_called = [] + + def fake_dl(album_info, target, ctx=None, **k): + dl_called.append(1) + open(f"{target}/cover.jpg", 'wb').close() + monkeypatch.setattr(aa, 'download_cover_art', fake_dl) + + res = aa.apply_art_to_album_files([str(f)], {'album_art_url': 'http://x/y.jpg'}, + {'album_name': 'B'}, folder=str(tmp_path)) + assert dl_called == [1] # no embedded art → API fetch + assert res['cover_written'] is True + + +def test_apply_skips_cover_when_sidecar_exists(tmp_path, monkeypatch): + # cover.jpg already present → don't extract or download. + (tmp_path / 'cover.jpg').write_bytes(b'EXISTING') + f = tmp_path / '01.flac'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=[SimpleNamespace(data=b'X')], tags=None, save=lambda: None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda *a, **k: True) + dl_called = [] + monkeypatch.setattr(aa, 'download_cover_art', lambda *a, **k: dl_called.append(1)) + + aa.apply_art_to_album_files([str(f)], {}, {}, folder=str(tmp_path)) + assert (tmp_path / 'cover.jpg').read_bytes() == b'EXISTING' # untouched + assert dl_called == [] diff --git a/tests/test_missing_cover_art.py b/tests/test_missing_cover_art.py index 20b4324f..57c841db 100644 --- a/tests/test_missing_cover_art.py +++ b/tests/test_missing_cover_art.py @@ -303,13 +303,14 @@ def test_scan_checks_disk_art_on_resolved_path(monkeypatch): checked = {} monkeypatch.setattr(mca, 'resolve_library_file_path', lambda raw, **k: '/resolved/song.flac' if raw == '/plex/raw/song.flac' else None) - monkeypatch.setattr(mca, 'album_has_art_on_disk', + monkeypatch.setattr(mca, 'file_has_embedded_art', lambda p: checked.update(path=p) or True) + monkeypatch.setattr(mca, 'folder_has_cover_sidecar', lambda d: True) # has cover.jpg too result = mca.MissingCoverArtJob().scan(context) assert checked.get('path') == '/resolved/song.flac' # resolved, not raw - assert result.findings_created == 0 # thumb + disk art → not flagged + assert result.findings_created == 0 # embedded + cover.jpg → not flagged def test_scan_unresolvable_path_not_flagged_disk_missing(monkeypatch): @@ -320,7 +321,8 @@ def test_scan_unresolvable_path_not_flagged_disk_missing(monkeypatch): context = _make_context(conn) monkeypatch.setattr(mca, 'resolve_library_file_path', lambda raw, **k: None) called = [] - monkeypatch.setattr(mca, 'album_has_art_on_disk', lambda p: called.append(p) or False) + monkeypatch.setattr(mca, 'file_has_embedded_art', lambda p: called.append(p) or False) + monkeypatch.setattr(mca, 'folder_has_cover_sidecar', lambda d: called.append(d) or False) result = mca.MissingCoverArtJob().scan(context) @@ -328,22 +330,40 @@ def test_scan_unresolvable_path_not_flagged_disk_missing(monkeypatch): assert called == [] # never checked art on a None path -def test_local_album_with_file_art_not_flagged_despite_empty_thumb(monkeypatch): - # Boulder: every album flagged, but "everything has art". Local files HAVE - # embedded art; only the DB thumb_url cache is empty. That is NOT missing - # cover art — must not be flagged. +def test_local_album_with_embedded_and_sidecar_not_flagged(monkeypatch): + # Has BOTH embedded art AND a cover.jpg — nothing missing, even with an + # empty DB thumb cache. (Boulder: don't flag albums that already have art.) conn = _make_db((1, 'Album', 1, '', None, None, None, None, None)) # empty thumb _add_track(conn, '/music/Album/01.flac') context = _make_context(conn) monkeypatch.setattr(mca, 'resolve_library_file_path', lambda raw, **k: raw) - monkeypatch.setattr(mca, 'album_has_art_on_disk', lambda p: True) # files have art + monkeypatch.setattr(mca, 'file_has_embedded_art', lambda p: True) + monkeypatch.setattr(mca, 'folder_has_cover_sidecar', lambda d: True) result = mca.MissingCoverArtJob().scan(context) - assert result.findings_created == 0 # has file art → not "missing" + assert result.findings_created == 0 # has both → not "missing" assert result.skipped == 1 +def test_embedded_art_but_no_cover_jpg_is_flagged(monkeypatch): + # Sokhi: files HAVE embedded art but no cover.jpg sidecar. With cover.jpg + # enabled (default), it's flagged so the filler writes the sidecar — even + # when the API finds NO art (the apply extracts the embedded art). + conn = _make_db((1, 'Album', 1, 'https://has/thumb', None, None, None, None, None)) + _add_track(conn, '/music/Album/01.flac') + context = _make_context(conn) + monkeypatch.setattr(mca, 'resolve_library_file_path', lambda raw, **k: raw) + monkeypatch.setattr(mca, 'file_has_embedded_art', lambda p: True) # embedded present + monkeypatch.setattr(mca, 'folder_has_cover_sidecar', lambda d: False) # but no cover.jpg + monkeypatch.setattr(mca, 'get_primary_source', lambda: 'spotify') + monkeypatch.setattr(mca, 'get_client_for_source', lambda s: _FakeClient()) # API finds nothing + + result = mca.MissingCoverArtJob().scan(context) + assert result.findings_created == 1 # flagged for the missing sidecar + assert context.findings[0]['details']['sidecar_from_embedded'] is True + + def test_local_album_without_file_art_still_flagged(monkeypatch): # Local album whose files genuinely lack art → still flagged (real case). # Give it a source id + findable art so a finding is created when flagged. @@ -351,7 +371,8 @@ def test_local_album_without_file_art_still_flagged(monkeypatch): _add_track(conn, '/music/Album/01.flac') context = _make_context(conn) monkeypatch.setattr(mca, 'resolve_library_file_path', lambda raw, **k: raw) - monkeypatch.setattr(mca, 'album_has_art_on_disk', lambda p: False) # no art on disk + monkeypatch.setattr(mca, 'file_has_embedded_art', lambda p: False) # no embedded art + monkeypatch.setattr(mca, 'folder_has_cover_sidecar', lambda d: False) monkeypatch.setattr(mca, 'get_primary_source', lambda: 'spotify') monkeypatch.setattr(mca, 'get_client_for_source', lambda s: _FakeClient(album_image='https://img/x'))