Skip to content

Conversation

@jasom
Copy link

@jasom jasom commented May 23, 2025

Tl;DR: Makes it possible to do properly do musicbrainz lookups on flacs with embedded cues that have non-standard pregaps

Very slightly longer description:

  1. Without this change, if you embed a flac cuesheet ripped with abcde.mkcue and --wholedisk the musicbrainz lookup will fail for all discs.
  2. Regardless of this change, if you don't use --wholedisk then any disc with a pregap other than 150 frames will also fail the musicbrainz lookup
  3. This change fixes the MB lookup for all disks with --wholedisk (including 3 discs I tried with non-standard pregaps)

More details:

  • The Musicbrainz lookup code adds the pregap to the leadout, so to get proper results when looking up embedded flac cuesheets generated with --wholedisk we need to subtract the pregap from the leadout.
  • I tested on several disks ripped without --wholedisk and confirmed that this did not affect the musicbrainz ID generated, so this change should affect nobody using the default options.

To generate the proper embedded cusheet the following options were used:

    MKCUE=abcde.mkcue
    MKCUEOPTS=--wholedisk
    CUEREADERSYNTAX=abcde.mkcue

The Musicbrainz lookup code adds the pregap to the leadout, so to get
proper results when looking up embedded flac cuesheets generated with
--wholedisk we need to subtract the pregap from the leadout.

With this change I tried 3 different disks that failed to do the MB
lookup with an embedded FLAC cuesheet, and they all worked.  To generate
the proper embedded cusheet the following options were used:

    MKCUE=abcde.mkcue
    MKCUEOPTS=--wholedisk
    CUEREADERSYNTAX=abcde.mkcue
@poddmo poddmo self-assigned this May 27, 2025
@poddmo
Copy link
Owner

poddmo commented May 27, 2025

It's been a long time since I looked at wholedisk/one-track rips and I have only used the default, external mkcue but this patch looks ok to me. As I read do_discid, this patch should only be called when the source is a flac.
How are you generating the source flac file? eg what is your abcde config and command-line
When you are processing the source flac file, where this bug appears that your patch is intended to resolve, what is your abcde config and command-line? I guess at this stage you are wanting to split the original wholedisk/one-track flac into individual tracks?
Can you please give some Musicbrainz/gnudb/discogs links to the releases your working on so I can better replicate your case?
I think one-track flacs with embedded cue sheets are an important archive format so I'm happy to spend some time looking at this one.

@poddmo
Copy link
Owner

poddmo commented May 27, 2025

Your patch seems be trying to fix the same thing as this: Pregap breaks Musicbrainz disc id calculation

johnlane/abcde@9ba1424

Does that help you too? I'm happy to apply whichever you recommend.

@jasom
Copy link
Author

jasom commented May 27, 2025

When you are processing the source flac file, where this bug appears that your patch is intended to resolve, what is your abcde config and command-line? I guess at this stage you are wanting to split the original wholedisk/one-track flac into individual tracks?

I already posted the configuration:

    MKCUE=abcde.mkcue
    MKCUEOPTS=--wholedisk
    CUEREADERSYNTAX=abcde.mkcue

My command-line is abcde -c rip.conf -1 -o flac -a default,cue

Can you please give some Musicbrainz/gnudb/discogs links to the releases your working on so I can better replicate your case?

Here's a sample TOC; note that track #1 starts at 182 instead of 150. https://musicbrainz.org/cdtoc/JoPf8PMpkxndWD.J7gSZ7NXmuXs-

It's relatively rare; I have 191 CDs ripped to FLAC and 7 of them have this issue.

Note that without --wholedisk the pregap does not make it into the FLAC embedded cue, which is probably why this bug has lingered so long. Note that the .cue file and the embedded cue are different; use metaflac --export-cuesheet-to=- $flac_file to view the embedded cue if you want to see it for yourself.

@jasom
Copy link
Author

jasom commented May 27, 2025

Your patch seems be trying to fix the same thing as this: Pregap breaks Musicbrainz disc id calculation

johnlane/abcde@9ba1424

Does that help you too? I'm happy to apply whichever you recommend.

That looks substantially similar to my fix; I'll have to try it tonight.

@jasom
Copy link
Author

jasom commented Jun 13, 2025

Two weeks after I said I would test it, I did.

The fix from johnlane/abcde@9ba1424 also works with my two test discs.

I'm clearly biased, but I think the place where I put the fix is better since it happens at the point of reading the cue from the flac file. John's fix does it later on in the process.

From my point of view, it's the difference between "Calculate the correct numbers" (my fix) and "Check if the numbers come from a known source that generates the wrong numbers and fix them up" (John's fix).

Matthias König posted the patch in John's repository to the mailing list in 2018

Mine was posted 3 years later

So Matthias' patch has been around longer if that matters to the decision.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants