danvics/soulsync
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

Fix #764: import no longer destroys embedded cover art

Browse source 
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.
This commit is contained in:
BoulderBadgeDad 2026-06-02 08:40:05 -07:00
parent de20897f83
commit 3dfec8a157
5 changed files with 489 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

115
core/metadata/art_preservation.py Normal file
View file

@ -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"]

8
core/metadata/artwork.py
View file

@ -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):

37
core/metadata/enrichment.py
View file

@ -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

179
tests/test_art_preservation.py Normal file
View file

@ -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"

153
tests/test_enrichment_art_preservation.py Normal file
View file

@ -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