#806: MusicBrainz cover art at native resolution (Cover Art Archive /front)
The CAA branch of _upgrade_art_url capped art at the /front-1200 thumbnail — a deliberate flakiness trade-off, but the policy had rotted into inconsistency: iTunes art already shipped at 3000x3000, and bare /front URLs (release-group lookups — exactly what the Re-tag flow produces) bypassed the cap entirely, which is how Sokhi observed retag delivering full-res while downloads got 1200. CAA URLs now upgrade to the bare /front ORIGINAL (native res, frequently 3000px+). The flakiness concern that motivated the old cap is handled where it belongs, in the fetch: _fetch_art_bytes now walks an attempt chain — original -> /front-1200 midpoint -> the original sized thumbnail — so a flaky archive.org degrades to the old 1200px behavior, never below it. Tests updated to the new contract (+3 chain tests: native-first, flaky degrades to 1200 not 250, full chain ends at the thumbnail). 623 metadata + 1267 art-path tests pass.
This commit is contained in:
BoulderBadgeDad
parent
b7fc6c3361
commit
c6d1dede2b
2 changed files with 100 additions and 38 deletions
|
|
@ -289,13 +289,15 @@ def _upgrade_art_url(art_url: str) -> str:
|
|||
elif "coverartarchive.org" in art_url:
|
||||
# MusicBrainz art arrives as Cover Art Archive thumbnails
|
||||
# (/front-250 — see musicbrainz_search._cover_art_url). Upgrade to the
|
||||
# 1200px thumbnail: a huge jump from 240p yet still served by CAA's own
|
||||
# CDN. Deliberately NOT the bare /front original — that redirects to
|
||||
# archive.org, which is flaky (intermittent 500s/timeouts) and can be
|
||||
# multi-MB, nasty to embed in every track. 1200 is the sweet spot of
|
||||
# quality + reliability. `_fetch_art_bytes` falls back to the original
|
||||
# sized URL if /front-1200 is ever refused.
|
||||
return re.sub(r"/front-\d+", "/front-1200", art_url)
|
||||
# bare /front ORIGINAL — native resolution, frequently 3000px+ (#806:
|
||||
# the old /front-1200 cap left MusicBrainz as the one source still
|
||||
# below native while iTunes already shipped 3000x3000 — and bare
|
||||
# /front URLs from release-group lookups bypassed the cap anyway,
|
||||
# so the policy was inconsistent in practice). The original redirects
|
||||
# to archive.org, which can be flaky, so `_fetch_art_bytes` inserts a
|
||||
# /front-1200 midpoint fallback before the original-size URL:
|
||||
# flakiness degrades to the old 1200px behavior, never below it.
|
||||
return re.sub(r"/front(-\d+)?$", "/front", art_url)
|
||||
return art_url
|
||||
|
||||
|
||||
|
|
@ -312,22 +314,27 @@ def _fetch_art_bytes(art_url: str):
|
|||
if not art_url:
|
||||
return None, None
|
||||
upgraded = _upgrade_art_url(art_url)
|
||||
try:
|
||||
with urllib.request.urlopen(upgraded, timeout=10) as response:
|
||||
return response.read(), (response.info().get_content_type() or "image/jpeg")
|
||||
except Exception as fetch_err:
|
||||
if upgraded != art_url:
|
||||
logger.info(
|
||||
"Upgraded art URL refused (%s); retrying original size", fetch_err
|
||||
)
|
||||
try:
|
||||
with urllib.request.urlopen(art_url, timeout=10) as response:
|
||||
return response.read(), (response.info().get_content_type() or "image/jpeg")
|
||||
except Exception as retry_err:
|
||||
logger.error("Art fetch failed after fallback: %s", retry_err)
|
||||
return None, None
|
||||
logger.error("Art fetch failed: %s", fetch_err)
|
||||
return None, None
|
||||
attempts = [upgraded]
|
||||
# CAA's bare /front original redirects to archive.org, which can be flaky.
|
||||
# Midpoint fallback: the 1200px CDN thumbnail (the pre-#806 behavior),
|
||||
# tried BEFORE the original sized URL so a flaky archive.org degrades to
|
||||
# 1200px — never all the way down to the 250px thumbnail.
|
||||
if "coverartarchive.org" in upgraded and upgraded.endswith("/front"):
|
||||
attempts.append(upgraded + "-1200")
|
||||
if art_url not in attempts:
|
||||
attempts.append(art_url)
|
||||
|
||||
last_err = None
|
||||
for i, candidate in enumerate(attempts):
|
||||
try:
|
||||
with urllib.request.urlopen(candidate, timeout=10) as response:
|
||||
return response.read(), (response.info().get_content_type() or "image/jpeg")
|
||||
except Exception as fetch_err:
|
||||
last_err = fetch_err
|
||||
if i < len(attempts) - 1:
|
||||
logger.info("Art URL refused (%s); falling back to next size", fetch_err)
|
||||
logger.error("Art fetch failed after %d attempt(s): %s", len(attempts), last_err)
|
||||
return None, None
|
||||
|
||||
|
||||
def _min_size_art_validator(min_px):
|
||||
|
|
|
|||
|
|
@ -45,26 +45,24 @@ class TestUpgradeArtUrl:
|
|||
url = 'https://cdn-images.dzcdn.net/images/cover/abc/1000x1000-000000-80-0-0.jpg'
|
||||
assert '1900x1900' in _upgrade_art_url(url)
|
||||
|
||||
def test_caa_thumbnail_upgraded_to_1200(self):
|
||||
# MusicBrainz art arrives as the /front-250 thumbnail; upgrade to the
|
||||
# 1200px CDN thumbnail (NOT the flaky bare /front original).
|
||||
def test_caa_thumbnail_upgraded_to_native_original(self):
|
||||
# #806: MusicBrainz art arrives as the /front-250 thumbnail; upgrade
|
||||
# to the bare /front ORIGINAL (native res, frequently 3000px+).
|
||||
# Flakiness of the original is handled by _fetch_art_bytes' 1200px
|
||||
# midpoint fallback, not by capping the URL here.
|
||||
url = 'https://coverartarchive.org/release/abc-123/front-250'
|
||||
assert _upgrade_art_url(url) == 'https://coverartarchive.org/release/abc-123/front-1200'
|
||||
assert _upgrade_art_url(url) == 'https://coverartarchive.org/release/abc-123/front'
|
||||
|
||||
def test_caa_500_scope_and_idempotent(self):
|
||||
def test_caa_all_sizes_and_scopes_upgrade_to_original(self):
|
||||
assert _upgrade_art_url('https://coverartarchive.org/release/x/front-500') \
|
||||
== 'https://coverartarchive.org/release/x/front-1200'
|
||||
# release-group scope works the same.
|
||||
== 'https://coverartarchive.org/release/x/front'
|
||||
assert _upgrade_art_url('https://coverartarchive.org/release/x/front-1200') \
|
||||
== 'https://coverartarchive.org/release/x/front'
|
||||
# release-group scope works the same (the URL shape from #806).
|
||||
assert _upgrade_art_url('https://coverartarchive.org/release-group/y/front-250') \
|
||||
== 'https://coverartarchive.org/release-group/y/front-1200'
|
||||
# Idempotent — an already-1200 URL stays put.
|
||||
assert _upgrade_art_url('https://coverartarchive.org/release-group/y/front-1200') \
|
||||
== 'https://coverartarchive.org/release-group/y/front-1200'
|
||||
== 'https://coverartarchive.org/release-group/y/front'
|
||||
|
||||
def test_caa_bare_front_left_alone(self):
|
||||
# The bare /front original is intentionally NOT what we want; the
|
||||
# sized-thumbnail regex doesn't touch it (and it never reaches the
|
||||
# helper in practice — _cover_art_url always emits /front-250).
|
||||
def test_caa_bare_front_idempotent(self):
|
||||
url = 'https://coverartarchive.org/release/abc/front'
|
||||
assert _upgrade_art_url(url) == url
|
||||
|
||||
|
|
@ -163,3 +161,60 @@ class TestFetchArtBytes:
|
|||
def test_empty_url_returns_none(self):
|
||||
assert _fetch_art_bytes('') == (None, None)
|
||||
assert _fetch_art_bytes(None) == (None, None)
|
||||
|
||||
# ── #806: CAA native-res chain — original → 1200px midpoint → thumbnail ──
|
||||
|
||||
def test_caa_fetches_native_original_first(self, monkeypatch):
|
||||
calls = []
|
||||
|
||||
def fake_urlopen(url, timeout=None):
|
||||
calls.append(url)
|
||||
return _FakeResponse(b'native-3000px')
|
||||
|
||||
monkeypatch.setattr('core.metadata.artwork.urllib.request.urlopen', fake_urlopen)
|
||||
|
||||
data, _ = _fetch_art_bytes('https://coverartarchive.org/release/r1/front-250')
|
||||
|
||||
assert data == b'native-3000px'
|
||||
assert calls == ['https://coverartarchive.org/release/r1/front']
|
||||
|
||||
def test_caa_flaky_original_degrades_to_1200_not_250(self, monkeypatch):
|
||||
"""archive.org flakiness lands on the old 1200px behavior — the
|
||||
midpoint sits BEFORE the tiny original thumbnail in the chain."""
|
||||
calls = []
|
||||
|
||||
def fake_urlopen(url, timeout=None):
|
||||
calls.append(url)
|
||||
if url.endswith('/front'):
|
||||
raise Exception('503 archive.org is having a day')
|
||||
return _FakeResponse(b'cdn-1200px')
|
||||
|
||||
monkeypatch.setattr('core.metadata.artwork.urllib.request.urlopen', fake_urlopen)
|
||||
|
||||
data, _ = _fetch_art_bytes('https://coverartarchive.org/release/r1/front-250')
|
||||
|
||||
assert data == b'cdn-1200px'
|
||||
assert calls == [
|
||||
'https://coverartarchive.org/release/r1/front',
|
||||
'https://coverartarchive.org/release/r1/front-1200',
|
||||
]
|
||||
|
||||
def test_caa_full_chain_ends_at_the_original_thumbnail(self, monkeypatch):
|
||||
calls = []
|
||||
|
||||
def fake_urlopen(url, timeout=None):
|
||||
calls.append(url)
|
||||
if url.endswith('/front') or url.endswith('-1200'):
|
||||
raise Exception('refused')
|
||||
return _FakeResponse(b'tiny-250px')
|
||||
|
||||
monkeypatch.setattr('core.metadata.artwork.urllib.request.urlopen', fake_urlopen)
|
||||
|
||||
data, _ = _fetch_art_bytes('https://coverartarchive.org/release/r1/front-250')
|
||||
|
||||
assert data == b'tiny-250px'
|
||||
assert calls == [
|
||||
'https://coverartarchive.org/release/r1/front',
|
||||
'https://coverartarchive.org/release/r1/front-1200',
|
||||
'https://coverartarchive.org/release/r1/front-250',
|
||||
]
|
||||
|
|
|
|||
Loading…
Reference in a new issue