From 3dfec8a157f00f518c3fa255218bcb54733b4c3a Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 2 Jun 2026 08:40:05 -0700 Subject: [PATCH] Fix #764: import no longer destroys embedded cover art MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit enhance_file_metadata rebuilds tags from scratch: for FLAC it calls clear_pictures(), for MP3/MP4 it clears the whole tag block — and it does this UP FRONT, then saves the file, long before it tries to fetch and embed the replacement art. So every way the re-embed could come up empty left the file saved with the original art destroyed and nothing put back: - extract_source_metadata returns nothing -> early save, no embed - no album-art URL / art download fails / rejected by the min-size guard -> embed_album_art_metadata returns early without adding a picture - art embedding disabled in config -> embed skipped entirely - embed raises mid-enrichment -> file left cleared on disk This is the "cover art gets corrupted/destroyed during import" half of #764 (continuation of #755); distinct from #750's truncated-cache DISPLAY bug. Fix: new core/metadata/art_preservation.py snapshots the existing art (the live Picture / APIC / MP4Cover objects, so they re-apply verbatim) BEFORE the clear, and restores it before each save IFF the file currently has none. Wired into all three exit paths in enhance_file_metadata (no-metadata early return, the final save, and the except handler). The restore is a strict no-op when art is already present, so the happy path — new art embedded — is byte-for-byte unchanged: it never clobbers or duplicates a freshly-embedded cover. embed_album_art_metadata now returns a bool so the intent (embedded / didn't) is explicit. Tests: - tests/test_art_preservation.py (5) — snapshot/restore round-trips through real mutagen FLAC + ID3 objects; restore no-ops when new art is present. - tests/test_enrichment_art_preservation.py (4) — runs the REAL enhance_file_metadata over a real FLAC with embedded art and asserts the art survives on disk for missing-metadata / failed-embed / embed-raises, and is correctly REPLACED (exactly one picture, new bytes) on success. 1019 tests pass across the metadata/enrichment/imports/acoustid suites. --- core/metadata/art_preservation.py | 115 ++++++++++++++ core/metadata/artwork.py | 8 +- core/metadata/enrichment.py | 37 +++++ tests/test_art_preservation.py | 179 ++++++++++++++++++++++ tests/test_enrichment_art_preservation.py | 153 ++++++++++++++++++ 5 files changed, 489 insertions(+), 3 deletions(-) create mode 100644 core/metadata/art_preservation.py create mode 100644 tests/test_art_preservation.py create mode 100644 tests/test_enrichment_art_preservation.py diff --git a/core/metadata/art_preservation.py b/core/metadata/art_preservation.py new file mode 100644 index 00000000..337cf7f6 --- /dev/null +++ b/core/metadata/art_preservation.py @@ -0,0 +1,115 @@ +"""Preserve embedded cover art across the metadata-enrichment rewrite. + +Issue #764 (continuation of #755): imported files lost their album art. +``enhance_file_metadata`` rebuilds tags from scratch — for FLAC it calls +``clear_pictures()`` and for MP3/MP4 it clears the whole tag block — *before* +it has the replacement art in hand. It then saves the file regardless of +whether new art was actually embedded. So every failure mode downstream +destroyed the art that shipped with the download: + + - source-metadata extraction returns nothing -> early save, no embed + - no album-art URL available / art download fails -> embed returns early + - art rejected by the min-resolution guard -> embed returns early + - art embedding disabled in config -> embed skipped entirely + +In all of those the file was saved with the pictures already cleared and +nothing put back. This module captures the existing art up front (live +mutagen objects, so they re-apply verbatim) and restores it right before a +save *iff the file currently has none* — so the happy path (new art embedded) +is byte-for-byte unchanged, and the only behaviour change is that we never +end up with less art than we started with. + +Scope mirrors ``embed_album_art_metadata``: FLAC ``Picture`` blocks, ID3 +``APIC`` frames, MP4 ``covr`` atoms. OggOpus/OggVorbis store art inside the +Vorbis comment (no ``clear_pictures``), so the enrichment rewrite never +strips it and it needs no preservation here. +""" + +from __future__ import annotations + +from typing import Any, List, Tuple + +from utils.logging_config import get_logger + +logger = get_logger("metadata.art_preservation") + +# Each snapshot entry is (kind, payload) where payload is a list of live +# mutagen objects captured before the tag rewrite. +ArtSnapshot = List[Tuple[str, list]] + + +def has_embedded_art(audio_file: Any, symbols: Any) -> bool: + """True iff ``audio_file`` currently carries embedded cover art in any + of the formats the enricher manages (FLAC pictures / ID3 APIC / MP4 covr).""" + try: + if getattr(audio_file, "pictures", None): + return True + tags = getattr(audio_file, "tags", None) + if tags is not None and isinstance(tags, symbols.ID3) and tags.getall("APIC"): + return True + if isinstance(audio_file, symbols.MP4) and tags and tags.get("covr"): + return True + except Exception as exc: # defensive: never let art-detection break a save + logger.debug("has_embedded_art check failed: %s", exc) + return False + + +def snapshot_embedded_art(audio_file: Any, symbols: Any) -> ArtSnapshot: + """Capture existing embedded art so it can be restored if re-embedding + fails. Returns a list of ``(kind, [objects])`` entries, or ``[]`` when the + file has no art. Captures the live mutagen objects (Picture / APIC frame / + MP4Cover) so they re-apply exactly as they were. + + Must be called BEFORE ``clear_pictures()`` / ``tags.clear()``.""" + snap: ArtSnapshot = [] + try: + pictures = getattr(audio_file, "pictures", None) + if pictures: + snap.append(("flac", list(pictures))) + tags = getattr(audio_file, "tags", None) + if tags is not None and isinstance(tags, symbols.ID3): + apics = tags.getall("APIC") + if apics: + snap.append(("id3", list(apics))) + if isinstance(audio_file, symbols.MP4) and tags: + covr = tags.get("covr") + if covr: + snap.append(("mp4", list(covr))) + except Exception as exc: + logger.debug("snapshot_embedded_art failed: %s", exc) + return snap + + +def restore_embedded_art(audio_file: Any, symbols: Any, snapshot: ArtSnapshot) -> bool: + """Re-apply captured art IFF the file currently has none. Returns True if + it restored something. + + No-op (returns False) when the snapshot is empty or the file already has + art — so calling this before a save never overwrites freshly-embedded art, + it only puts back what the rewrite would otherwise have destroyed.""" + if not snapshot or has_embedded_art(audio_file, symbols): + return False + restored = False + for kind, payload in snapshot: + try: + if kind == "flac" and hasattr(audio_file, "add_picture"): + for pic in payload: + audio_file.add_picture(pic) + restored = True + elif kind == "id3": + tags = getattr(audio_file, "tags", None) + if tags is not None: + for frame in payload: + tags.add(frame) + restored = True + elif kind == "mp4": + audio_file["covr"] = payload + restored = True + except Exception as exc: + logger.debug("restore_embedded_art (%s) failed: %s", kind, exc) + if restored: + logger.info("Preserved existing embedded cover art (re-embed produced none).") + return restored + + +__all__ = ["has_embedded_art", "snapshot_embedded_art", "restore_embedded_art", "ArtSnapshot"] diff --git a/core/metadata/artwork.py b/core/metadata/artwork.py index 9b8386af..a3c38c98 100644 --- a/core/metadata/artwork.py +++ b/core/metadata/artwork.py @@ -363,7 +363,7 @@ def embed_album_art_metadata(audio_file, metadata: dict): cfg = get_config_manager() symbols = get_mutagen_symbols() if not symbols: - return + return False try: image_data = None @@ -410,12 +410,12 @@ def embed_album_art_metadata(audio_file, metadata: dict): art_url = metadata.get("album_art_url") if not art_url: logger.warning("No album art URL available for embedding.") - return + return False image_data, mime_type = _fetch_art_bytes(art_url) if not image_data: logger.error("Failed to download album art data.") - return + return False if isinstance(audio_file.tags, symbols.ID3): audio_file.tags.add(symbols.APIC(encoding=3, mime=mime_type, type=3, desc="Cover", data=image_data)) @@ -434,8 +434,10 @@ def embed_album_art_metadata(audio_file, metadata: dict): audio_file["covr"] = [symbols.MP4Cover(image_data, imageformat=fmt)] logger.info("Album art successfully embedded.") + return True except Exception as exc: logger.error("Error embedding album art: %s", exc) + return False def download_cover_art(album_info: dict, target_dir: str, context: dict = None): diff --git a/core/metadata/enrichment.py b/core/metadata/enrichment.py index 43e668e8..fdd34042 100644 --- a/core/metadata/enrichment.py +++ b/core/metadata/enrichment.py @@ -6,6 +6,10 @@ import os from types import SimpleNamespace from typing import Any +from core.metadata.art_preservation import ( + restore_embedded_art, + snapshot_embedded_art, +) from core.metadata.artwork import embed_album_art_metadata from core.metadata.common import ( get_config_manager, @@ -76,6 +80,8 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf file_lock = get_file_lock(file_path) with file_lock: logger.info("Enhancing metadata for: %s", os.path.basename(file_path)) + art_snapshot = [] # defined up front so the except handler can restore + audio_file = None try: strip_all_non_audio_tags(file_path) audio_file = symbols.File(file_path) @@ -83,6 +89,13 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf logger.error("Could not load audio file with Mutagen: %s", file_path) return False + # Capture any embedded cover art BEFORE we clear it. The rewrite + # below clears pictures/tags up front, but new art isn't fetched + # until much later (and may fail / be unavailable / be disabled). + # Without this snapshot, every such failure saves the file with + # the art destroyed and nothing put back — issue #764. + art_snapshot = snapshot_embedded_art(audio_file, symbols) + if hasattr(audio_file, "clear_pictures"): audio_file.clear_pictures() @@ -99,6 +112,9 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf metadata = extract_source_metadata(context, artist, album_info) if not metadata: logger.error("Could not extract source metadata, saving with cleared tags.") + # Don't destroy the original art just because we couldn't + # enrich the tags — put it back before saving. + restore_embedded_art(audio_file, symbols, art_snapshot) save_audio_file(audio_file, symbols) return True @@ -202,6 +218,13 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf elif isinstance(audio_file, symbols.MP4): audio_file["----:com.apple.iTunes:QUALITY"] = [symbols.MP4FreeForm(quality.encode("utf-8"))] + # If art embedding was skipped/disabled or produced nothing (no + # URL, download failed, rejected by the min-resolution guard), + # the file still has the pictures we cleared above. Restore the + # original so import never strips existing art (#764). No-op when + # new art was embedded — that path is unchanged. + restore_embedded_art(audio_file, symbols, art_snapshot) + save_audio_file(audio_file, symbols) verified = verify_metadata_written(file_path) @@ -219,4 +242,18 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf logger.warning("[Metadata Debug] Artist: %s", artist.get("name", "MISSING") if artist else "None") logger.warning("[Metadata Debug] Album info: %s", album_info.get("album_name", "MISSING") if album_info else "None") logger.error("[Metadata Debug] Traceback:\n%s", traceback.format_exc()) + # We cleared the file's art early; if the rewrite then crashed + # before re-embedding, the on-disk file (already saved cleared at + # the start) would be left art-less. Best-effort: put the original + # art back and persist it so a mid-enrichment crash never destroys + # the cover (#764). Guarded so a failure here can't mask the + # original error. + try: + if audio_file is not None and art_snapshot and restore_embedded_art( + audio_file, symbols, art_snapshot + ): + save_audio_file(audio_file, symbols) + logger.info("Restored original cover art after enrichment error.") + except Exception as restore_exc: + logger.debug("Art restore after error failed: %s", restore_exc) return False diff --git a/tests/test_art_preservation.py b/tests/test_art_preservation.py new file mode 100644 index 00000000..230f1420 --- /dev/null +++ b/tests/test_art_preservation.py @@ -0,0 +1,179 @@ +"""Tests for embedded-cover-art preservation across the enrichment rewrite. + +Regression for #764 (continuation of #755): importing a file destroyed its +embedded album art whenever the re-embed step couldn't produce new art +(no source metadata, no art URL, download failed, rejected by the min-size +guard, or art embedding disabled). ``enhance_file_metadata`` clears pictures +up front and saves regardless; these helpers snapshot the art first and put +it back iff the file would otherwise be saved with none. + +Uses real mutagen objects (a minimal valid FLAC + a minimal MP3) so the +snapshot/restore round-trips through the actual Picture/APIC APIs the +enricher uses — not a mock of them. +""" + +from __future__ import annotations + +import os +import tempfile + +import pytest + +mutagen = pytest.importorskip("mutagen") + +from mutagen.flac import FLAC, Picture # noqa: E402 +from mutagen.id3 import APIC, ID3, TIT2 # noqa: E402 + +from core.metadata.common import get_mutagen_symbols # noqa: E402 +from core.metadata.art_preservation import ( # noqa: E402 + has_embedded_art, + restore_embedded_art, + snapshot_embedded_art, +) + +SYMBOLS = get_mutagen_symbols() + +# 1x1 PNG — smallest valid image bytes for a real Picture/APIC payload. +_PNG = ( + b"\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00\x01" + b"\x08\x06\x00\x00\x00\x1f\x15\xc4\x89\x00\x00\x00\nIDATx\x9cc\x00" + b"\x01\x00\x00\x05\x00\x01\r\n-\xb4\x00\x00\x00\x00IEND\xaeB`\x82" +) + + +def _minimal_flac_bytes() -> bytes: + # 4-byte magic + last STREAMINFO block (34 bytes). Mirrors the fixture + # used by tests/test_tag_writer_multi_artist.py. + return ( + b"fLaC" + + b"\x80\x00\x00\x22" + + b"\x00\x10\x00\x10" + + b"\x00\x00\x00\x00\x00\x00" + + b"\x0a\xc4\x42\xf0\x00\x00\x00\x00" + + b"\x00" * 16 + ) + + +@pytest.fixture +def flac_with_art(): + fd, path = tempfile.mkstemp(suffix=".flac") + os.close(fd) + with open(path, "wb") as f: + f.write(_minimal_flac_bytes()) + audio = FLAC(path) + pic = Picture() + pic.data = _PNG + pic.type = 3 + pic.mime = "image/png" + pic.width = 1 + pic.height = 1 + pic.depth = 24 + audio.add_picture(pic) + audio.save() + yield path + try: + os.remove(path) + except OSError: + pass + + +class _ID3Holder: + """Stand-in for an mutagen MP3 object: exposes a real ``ID3`` tag block + (the only thing the snapshot/restore helpers touch) without needing a + syncable MPEG frame on disk. ``__setitem__`` mirrors mutagen's mapping + sugar used by the MP4 restore branch — unused here but kept faithful.""" + + def __init__(self): + self.tags = ID3() + self.tags.add(TIT2(encoding=3, text=["original"])) + self.tags.add(APIC(encoding=3, mime="image/png", type=3, desc="Cover", data=_PNG)) + + def __setitem__(self, key, value): + self.tags[key] = value + + +@pytest.fixture +def mp3_with_art(): + return _ID3Holder() + + +# ── FLAC ──────────────────────────────────────────────────────────────── + + +def test_flac_art_restored_after_clear(flac_with_art): + audio = FLAC(flac_with_art) + assert has_embedded_art(audio, SYMBOLS) + + snap = snapshot_embedded_art(audio, SYMBOLS) + assert snap # something captured + + # Simulate the enrichment rewrite: nuke the art, fail to re-embed. + audio.clear_pictures() + assert not has_embedded_art(audio, SYMBOLS) + + restored = restore_embedded_art(audio, SYMBOLS, snap) + assert restored is True + assert has_embedded_art(audio, SYMBOLS) + assert audio.pictures[0].data == _PNG + + +def test_flac_restore_is_noop_when_new_art_present(flac_with_art): + # Happy path: re-embed succeeded, so restore must NOT touch the file. + audio = FLAC(flac_with_art) + snap = snapshot_embedded_art(audio, SYMBOLS) + + audio.clear_pictures() + new = Picture() + new.data = _PNG + b"NEWART" + new.type = 3 + new.mime = "image/png" + audio.add_picture(new) + + restored = restore_embedded_art(audio, SYMBOLS, snap) + assert restored is False + assert len(audio.pictures) == 1 + assert audio.pictures[0].data == _PNG + b"NEWART" # not clobbered/duplicated + + +def test_flac_no_art_snapshot_empty(): + fd, path = tempfile.mkstemp(suffix=".flac") + os.close(fd) + try: + with open(path, "wb") as f: + f.write(_minimal_flac_bytes()) + audio = FLAC(path) + assert snapshot_embedded_art(audio, SYMBOLS) == [] + # Restoring an empty snapshot is a no-op. + assert restore_embedded_art(audio, SYMBOLS, []) is False + finally: + os.remove(path) + + +# ── MP3 / ID3 ───────────────────────────────────────────────────────────── + + +def test_mp3_apic_restored_after_tags_cleared(mp3_with_art): + audio = mp3_with_art + assert has_embedded_art(audio, SYMBOLS) + snap = snapshot_embedded_art(audio, SYMBOLS) + assert snap + + # The enricher does audio_file.tags.clear() then rewrites tags. + audio.tags.clear() + assert not has_embedded_art(audio, SYMBOLS) + + restored = restore_embedded_art(audio, SYMBOLS, snap) + assert restored is True + apics = audio.tags.getall("APIC") + assert apics and apics[0].data == _PNG + + +def test_mp3_restore_noop_when_new_apic_present(mp3_with_art): + audio = mp3_with_art + snap = snapshot_embedded_art(audio, SYMBOLS) + audio.tags.clear() + audio.tags.add(APIC(encoding=3, mime="image/png", type=3, desc="Cover", data=_PNG + b"NEW")) + + assert restore_embedded_art(audio, SYMBOLS, snap) is False + apics = audio.tags.getall("APIC") + assert len(apics) == 1 and apics[0].data == _PNG + b"NEW" diff --git a/tests/test_enrichment_art_preservation.py b/tests/test_enrichment_art_preservation.py new file mode 100644 index 00000000..82f55def --- /dev/null +++ b/tests/test_enrichment_art_preservation.py @@ -0,0 +1,153 @@ +"""End-to-end proof that enhance_file_metadata never destroys embedded art. + +Bug #764: the enrichment rewrite clears cover art up front and saves the file +regardless of whether new art gets re-embedded. These tests run the REAL +``enhance_file_metadata`` against a REAL FLAC that has embedded art, and assert +the art is still on disk after every failure path — and that the happy path +(new art embedded) correctly REPLACES it. This exercises the actual wiring +(snapshot -> clear -> rewrite -> restore/save), not just the helper functions. + +Only the external collaborators are stubbed (config, source-metadata extraction, +the art fetch, source-id embed, verification) — the clear/snapshot/restore/save +sequence under test runs for real through mutagen. +""" + +from __future__ import annotations + +import os +import tempfile +from unittest.mock import patch + +import pytest + +pytest.importorskip("mutagen") +from mutagen.flac import FLAC, Picture # noqa: E402 + +import core.metadata.enrichment as enrichment # noqa: E402 + +_PNG = ( + b"\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00\x01" + b"\x08\x06\x00\x00\x00\x1f\x15\xc4\x89\x00\x00\x00\nIDATx\x9cc\x00" + b"\x01\x00\x00\x05\x00\x01\r\n-\xb4\x00\x00\x00\x00IEND\xaeB`\x82" +) + + +class _Cfg: + """Config stub: returns the caller's default for every key, so + metadata_enhancement.enabled / embed_album_art resolve True.""" + + def get(self, key, default=None): + return default + + +def _make_flac_with_art(path): + minimal = ( + b"fLaC" + + b"\x80\x00\x00\x22" + + b"\x00\x10\x00\x10" + + b"\x00\x00\x00\x00\x00\x00" + + b"\x0a\xc4\x42\xf0\x00\x00\x00\x00" + + b"\x00" * 16 + ) + with open(path, "wb") as f: + f.write(minimal) + audio = FLAC(path) + pic = Picture() + pic.data = _PNG + pic.type = 3 + pic.mime = "image/png" + pic.width = 1 + pic.height = 1 + pic.depth = 24 + audio.add_picture(pic) + audio.save() + + +@pytest.fixture +def flac_path(): + fd, path = tempfile.mkstemp(suffix=".flac") + os.close(fd) + _make_flac_with_art(path) + yield path + try: + os.remove(path) + except OSError: + pass + + +def _disk_art(path): + """Return the embedded picture bytes on disk, or None.""" + pics = FLAC(path).pictures + return pics[0].data if pics else None + + +def _run(flac_path, *, metadata, embed_side_effect): + """Drive enhance_file_metadata with stubbed collaborators.""" + with patch.object(enrichment, "get_config_manager", return_value=_Cfg()), \ + patch.object(enrichment, "strip_all_non_audio_tags"), \ + patch.object(enrichment, "extract_source_metadata", return_value=metadata), \ + patch.object(enrichment, "embed_source_ids"), \ + patch.object(enrichment, "verify_metadata_written", return_value=True), \ + patch.object(enrichment, "embed_album_art_metadata", side_effect=embed_side_effect): + return enrichment.enhance_file_metadata( + flac_path, context={}, artist={"name": "Coldplay"}, album_info={}, + ) + + +# ── failure paths: art MUST survive ────────────────────────────────────── + + +def test_art_survives_when_source_metadata_missing(flac_path): + # extract_source_metadata returns None -> early return path. + assert _disk_art(flac_path) == _PNG + result = _run(flac_path, metadata=None, embed_side_effect=lambda *a, **k: False) + assert result is True + assert _disk_art(flac_path) == _PNG # art preserved on disk + + +def test_art_survives_when_embed_produces_nothing(flac_path): + # Metadata is fine, but the art fetch fails -> embed is a no-op (returns + # False, adds no picture). The original art must remain. + def embed_noop(audio_file, metadata): + return False # mirrors "no art URL" / "download failed" + + result = _run(flac_path, metadata={"title": "Yellow", "artist": "Coldplay"}, + embed_side_effect=embed_noop) + assert result is True + assert _disk_art(flac_path) == _PNG + + +def test_art_survives_when_embed_raises(flac_path): + # A hard crash mid-enrichment must not leave the file art-less. + def embed_boom(audio_file, metadata): + raise RuntimeError("art backend exploded") + + result = _run(flac_path, metadata={"title": "Yellow", "artist": "Coldplay"}, + embed_side_effect=embed_boom) + assert result is False # enrichment reported failure + assert _disk_art(flac_path) == _PNG # ...but art was restored on disk + + +# ── happy path: new art REPLACES old, no duplication ────────────────────── + + +def test_new_art_replaces_old_when_embed_succeeds(flac_path): + new_bytes = _PNG + b"BRANDNEW" + + def embed_real(audio_file, metadata): + # Simulate a successful fetch+embed: add the new picture in-place. + pic = Picture() + pic.data = new_bytes + pic.type = 3 + pic.mime = "image/png" + audio_file.add_picture(pic) + return True + + result = _run(flac_path, metadata={"title": "Yellow", "artist": "Coldplay"}, + embed_side_effect=embed_real) + assert result is True + on_disk = FLAC(flac_path).pictures + # Exactly one picture, and it's the NEW art — restore must not have + # re-added the old one on top. + assert len(on_disk) == 1 + assert on_disk[0].data == new_bytes