Skip to content

Add WavPack MD5 metadata support - #717

Open
audiomuze wants to merge 3 commits into
quodlibet:mainfrom
audiomuze:wavpackmd5
Open

Add WavPack MD5 metadata support#717
audiomuze wants to merge 3 commits into
quodlibet:mainfrom
audiomuze:wavpackmd5

Conversation

@audiomuze

Copy link
Copy Markdown

This adds WavPack MD5 checksum reading to WavPackInfo.md5_signature, matching the FLAC API shape for compatibility.

It supports both WavPack MD5 block IDs (0x26 and 0x29) and scans later blocks when the checksum is not stored in the first block.

Validation:

  • Added focused tests for the MD5 parser
  • Verified against real-world .wv files by comparing Mutagen output with wvunpack -f

- Parse WavPack MD5 metadata sub-blocks from real files
- Support both standard and alternate MD5 block IDs
- Scan later blocks when MD5 is not stored in the first block
- Match Mutagen's FLAC md5_signature API for compatibility
- Add focused tests plus real-world validation coverage
@audiomuze

Copy link
Copy Markdown
Author

I investigated the 2 remaining failing checks (ubuntu-latest 3.10 and ubuntu-latest 3.12) and they appear unrelated to this WavPack change.

Both are the Build docs step, failing with:

duplicate object description of mutagen.flac.FLAC.info, other instance in api/flac

I reproduced this on the PR branch and on origin/main with the same command:

uv sync --group docs && uv run sphinx-build -W -a -E -b html -n docs docs/_build

Result is identical on main, so this looks like a pre-existing docs CI issue rather than a regression introduced here.

For this PR specifically:

  • uv run coverage run --branch setup.py test passes
  • uv run mypy . passes
  • uv run flake8 passes

Could this be merged with awareness of the existing docs CI issue?

@audiomuze
audiomuze marked this pull request as ready for review June 25, 2026 23:16
@audiomuze
audiomuze marked this pull request as draft June 27, 2026 06:34
@phw

phw commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

I investigated the 2 remaining failing checks (ubuntu-latest 3.10 and ubuntu-latest 3.12) and they appear unrelated to this WavPack change.

@audiomuze Unfortunately not, and that is where we finally need some decision. The build fails because the docstring of the new attribute references mutagen.flac.StreamInfo.md5_signature. Which I think is great.

Problem is that flac.StreamInfo.md5_signature itself is undocumented. This property exists in a limbo where it is around for a long time, likely used in existing code, but never officially being part of the mutagen API. See also the comment at #485 (comment)

I personally think we actually should make this part of the API, though, especially if we have a second implementation of it. I think this is metadata for those files that is commonly used. And the property has been around for such a long time and is not going away anyway, so why not also make it official part of the API?

Alternatively for this PR we could just not have the docstring referencing flac.StreamInfo.md5_signature.

@lazka , what's your take here?

@lazka

lazka commented Jun 27, 2026

Copy link
Copy Markdown
Member

I would just not reference it, since I don't think it was meant to be public and is just there to write it back.

On this PR: What is the motivation for reading the MD5 in the first place? We don't read audio data in mutagen, so I don't see much use for it.

@audiomuze

audiomuze commented Jun 27, 2026

Copy link
Copy Markdown
Author

My motivation for reading the MD5 is the same for FLAC and WavPack. As it's a MD5 of the audio stream it can be used to reliably detect duplicated tracks across a user's music collection irrespective of tagging, filenames, file size etc., whether searching for individual tracks or albums . For albums I concatenate the sorted MD5's of all tracks in a folder and compare it against the same for every other folder in a directory tree. Puddletag also uses it to be able to show duplicate tracks in the tagger.

In addition having the MD5's stored in a database along with other tag metadata e.g. a bliss_analysis tag derived from the output of an audio analyser enables user communities to share analysis metadata, avoiding the need to re-perform analysis someone else has carried out previously.

I vaguely recall a time many years back when I believe it was documented for FLAC - I recall seeing it in what I think is this page.

I personally think we actually should make this part of the API, though, especially if we have a second implementation of it. I think this is metadata for those files that is commonly used. And the property has been around for such a long time and is not going away anyway, so why not also make it official part of the API?

I agree, I can't think of any reason not to document it, and I think its inclusion enhances rather than detracts from Mutagen . But if If I must remove the reference in docstring to get the PR merged I'll do so.

Either way, I'd appreciate a final steer to move this forward? @lazka?

@audiomuze

Copy link
Copy Markdown
Author

As requested, I have updated this PR to remove the FLAC attribute cross-reference from the WavPack docstring.
The WavPack attribute remains documented on its own terms.

@audiomuze
audiomuze marked this pull request as ready for review July 6, 2026 05:24

@phw phw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A +1 on this from my side. @lazka ?

(And I even think we should document the attribute for FLAC as well, but this is a separate issue)

@lazka

lazka commented Jul 12, 2026

Copy link
Copy Markdown
Member

ok, some way to identify duplicates sounds like a valid use case we can support, as long as it doesn't mean parsing the whole file by default and slowing down normal metadata operations too much.

@audiomuze

Copy link
Copy Markdown
Author

So this going to be merged? I’ve got downstream code that would like to leverage it.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants