Cover Art Filler: write cover.jpg sidecars, even when files already have embedded art (Sokhi #813)
Sokhi: after the earlier flag fix, scans returned 0 matches — albums with embedded art but no cover.jpg were treated as fully arted and skipped, so their cover.jpg never got written. Root cause: the scan used album_has_art_on_disk (True if EITHER embedded art OR a cover.jpg exists), conflating the two. Now it checks them separately: - a local album is flagged if it lacks embedded art, OR it lacks a cover.jpg sidecar AND cover.jpg writing is enabled (metadata_enhancement.cover_art_ download — Boulder: "only scan for cover.jpgs when enabled"). - an album that has embedded art but no sidecar is fixable even when the API finds no art: the apply writes cover.jpg from the EXISTING embedded art. apply_art_to_album_files now writes the cover.jpg sidecar by extracting the album's own embedded art (new extract_embedded_art) — consistent with the files, no API call — and only falls back to download_cover_art when there's nothing embedded to extract. _fix_missing_cover_art no longer bails on a missing artwork_url when sidecar_from_embedded is set. Tests: scan flags embedded-but-no-cover.jpg (incl. when API finds nothing), still skips albums with both, still flags artless albums; apply writes cover.jpg from embedded art (no download), falls back to download when none, skips when a sidecar already exists; extract_embedded_art unit tests. 1344 cover/art/repair tests pass.
This commit is contained in:
BoulderBadgeDad
parent
c654deac17
commit
5352db0a22
5 changed files with 210 additions and 46 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
}
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 == []
|
||||
|
|
|
|||
|
|
@ -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'))
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue