Skip to content

scrub: don't abort import on illegal MP4 album art - #7047

Open
MwC-Trexx wants to merge 3 commits into
beetbox:masterfrom
MwC-Trexx:fix/scrub-illegal-mp4-art
Open

MwC-Trexx wants to merge 3 commits into
beetbox:masterfrom
MwC-Trexx:fix/scrub-illegal-mp4-art

Conversation

@MwC-Trexx

Copy link
Copy Markdown

Bug Description

Fixes #2498

scrub extracts embedded album art, strips tags, then writes the art back. For MP4/M4A files whose cover is not JPEG or PNG, mediafile can read the image but raises ValueError: MP4 files only supports PNG and JPEG images on restore. That exception was uncaught, so a single bad cover aborted the entire import.

Reproduced on beets 2.14.1 during a quiet bulk import of a large library (same traceback as #2498 / #2713).

Root Cause

beetsplug/scrub.py _scrub_item only caught mediafile.UnreadableFileError around mf.images = images. mediafile.storage.mp4.MP4ImageStorageStyle.serialize raises ValueError for any other MIME type.

Fix

Catch ValueError, OSError, and mutagen.MutagenError on art restore, log the path, and continue without art. Tags are still rewritten.

How to Verify

  1. Embed a TIFF (or GIF) as an MP4 covr atom.
  2. Run beet import / beet scrub with scrub.auto enabled.
  3. Import should complete; the file keeps database-tracked tags and drops the illegal art.

Test Plan

  • Added regression test test_illegal_mp4_art_does_not_crash_restore (embeds TIFF into min.m4a, asserts _scrub_item does not raise)
  • Confirmed the new test fails on unpatched scrub.py with the original ValueError
  • pytest test/plugins/test_scrub.py — 4 passed
  • Changelog + scrub plugin docs updated

Risk Assessment

Low — only the art-restore failure path changes; successful JPEG/PNG restore is unchanged. Worst case for illegal art is the previous implicit outcome (no art) without taking down the import.

@MwC-Trexx
MwC-Trexx requested a review from a team as a code owner September 21, 2026 12:41
@github-actions github-actions Bot added the scrub scrub plugin label Sep 21, 2026
@codecov

codecov Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.41%. Comparing base (5e6d9ad) to head (4ab8b07).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
beetsplug/scrub.py 83.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7047      +/-   ##
==========================================
- Coverage   77.42%   77.41%   -0.02%     
==========================================
  Files         163      163              
  Lines       21870    21882      +12     
  Branches     3374     3377       +3     
==========================================
+ Hits        16933    16939       +6     
- Misses       4120     4124       +4     
- Partials      817      819       +2     
Files with missing lines Coverage Δ
beetsplug/scrub.py 64.70% <83.33%> (+3.06%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@semohr

semohr commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Hi! Thanks for your interest in our project. Please note our AI_POLICY!

A few thoughts:

  • I’m hesitant to broadly catch ValueError, OSError, and MutagenError, as this could hide unrelated issues.
  • Ideally, this should be fixed directly in Mutagen. If there isn’t an issue yet, I’d suggest opening one :)
  • If an upstream fix is likely to take some time, we could add a specific error in mediafile for unsupported artwork and handle that explicitly in scrub. Since we maintain mediafile, moving the fix there shouldn’t be a problem.

Review: a broad except (ValueError, OSError, MutagenError) could hide
unrelated write failures. Filter JPEG/PNG by sniffed MIME before restore
so illegal covers are dropped without swallowing other errors.
@MwC-Trexx
MwC-Trexx force-pushed the fix/scrub-illegal-mp4-art branch from f5213f7 to 4ab8b07 Compare September 23, 2026 10:49

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scrub scrub plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

scrub: Catch error when re-embedding illegal album art

2 participants