Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MusicBrainz TOC compute per current specs #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jesus2099
Copy link

@jesus2099 jesus2099 commented Apr 24, 2021

Specs: https://wiki.musicbrainz.org/Disc_ID_Calculation

If I take issue #5 problematic example:

$ cd-discid --musicbrainz
5 183 51735 90810 126230 165433 183465

It should be and is now:

$ cd-discid --musicbrainz
1 5 183465 183 51735 90810 126230 165433

I tested, there is no regressions for normal non MusicBrainz discid compute.

Specs: https://wiki.musicbrainz.org/Disc_ID_Calculation

If I take issue taem#5 problematic example:

    5 183 51735 90810 126230 165433 183465

It should be and is now:

    1 5 183465 183 51735 90810 126230 165433

I tested, there is no regressions for normal discid compute.
@wawrzek
Copy link

wawrzek commented Apr 30, 2021

@jesus2099 Do you think the project is alive?

@jesus2099
Copy link
Author

jesus2099 commented May 1, 2021

I don't know.
I hope @taem comes back here someday and merges this, so good.
If you or @TJFOE wants to try this version, you can clone or download my fork's fix-MB-DiscID branch and run make command.

Tell me if issues.

I want to test on special CD (CCCD, Enhanced CD, game CD) if it works correctly or if I need to send a second commit to this PR.

@wawrzek
Copy link

wawrzek commented May 1, 2021

OK. I built it and use with Crux (https://crux.nu/) without problem. I keep my patched port here: https://wawrzek.name/crux/repo/.
I don't think I have any special CDs to run further tests.

λ europe cd-discid → λ git master → cd-discid --musicbrainz /dev/cdrom
1 15 316881 150 17591 41037 62264 82241 105260 132993 156887 175406 201505 225513 232586 251824 274989 285083
λ europe cd-discid → λ git master → cd-discid /dev/cdrom
c3107f0f 15 150 17591 41037 62264 82241 105260 132993 156887 175406 201505 225513 232586 251824 274989 285083 4225

@jesus2099
Copy link
Author

jesus2099 commented May 7, 2021

Mmh.
My fix is great but Enhanced CD (CD Extra) suffer from 2 old known bugs (fixed in official libdiscid):

  1. Last track before data session is seen as 2:32 longer (11400 frames, 152 seconds)
  2. Data session appears as last track (should be omitted)

Example EXPO EXPO.

(Fixed) cd-discid --musicbrainz gives:

1 17 270393 150 5892 26142 32540 54612 74225 95915 99667 120772 146122 149072 175460 180240 206175 231592 235010 269197

Instead of:

1 16 257797 150 5892 26142 32540 54612 74225 95915 99667 120772 146122 149072 175460 180240 206175 231592 235010

So my fix is still good for all Audio only CD.
But cd-discid should use the official libdiscid for --musicbrainz option, to benefit from fixed old bugs. :)
I don't know how it can be done, though, I am not a real coder.


Update:

To make it clear, my pull request still makes a more correct disc ID for Enhanced CD too.
It's just that the 2 above bugs, that are already there, can only be fixed by using the official libdiscid, or something.

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.

None yet

2 participants