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

musicbrainz calculation fails on cd with data tracks that are not positioned at the end #289

Closed
CaseOf opened this issue Aug 6, 2018 · 9 comments · Fixed by #435
Closed
Assignees
Labels
Accepted Accepted issue on our roadmap Bug Generic bug: can be used together with more specific labels Needed: discussion More discussion needed before anything can be done (or still no agreement has been reached) Needed: patch A pull request is required Priority: high High priority
Milestone

Comments

@CaseOf
Copy link

CaseOf commented Aug 6, 2018

I tried to rip a Microsoft Age of Empires Gold Edition disc containing at first track data about the game, and on following tracks, game's soundtrack.
Ripping failed as following:

quentin@gentoo_quentin ~/test $ whipper cd rip -U --cdr
Using configured read offset 6
Checking device /dev/sr0
Reading TOC...
CDDB disc id: d30ee90f
Traceback (most recent call last):
  File "/usr/lib/python-exec/python2.7/whipper", line 11, in <module>
    load_entry_point('whipper==0.7.0', 'console_scripts', 'whipper')()
  File "/usr/lib64/python2.7/site-packages/whipper/command/main.py", line 36, in main
    ret = cmd.do()
  File "/usr/lib64/python2.7/site-packages/whipper/command/basecommand.py", line 139, in do
    return self.cmd.do()
  File "/usr/lib64/python2.7/site-packages/whipper/command/basecommand.py", line 139, in do
    return self.cmd.do()
  File "/usr/lib64/python2.7/site-packages/whipper/command/cd.py", line 107, in do
    self.mbdiscid = self.ittoc.getMusicBrainzDiscId()
  File "/usr/lib64/python2.7/site-packages/whipper/image/table.py", line 343, in getMusicBrainzDiscId
    values = self._getMusicBrainzValues()
  File "/usr/lib64/python2.7/site-packages/whipper/image/table.py", line 458, in _getMusicBrainzValues
    assert not self.tracks[-1].audio
AssertionError
@MerlijnWajer MerlijnWajer changed the title musicbrainz calculato fails on cd with data tracks that is not at the end musicbrainz calculation fails on cd with data tracks that are not positioned at the end Aug 6, 2018
@MerlijnWajer MerlijnWajer self-assigned this Aug 6, 2018
@MerlijnWajer
Copy link
Collaborator

The issue here is that morituri's own "musicbrainz id calculation" fails on CDs with data tracks on special places. For my IA fork I decided it wasn't worth my time fixing the custom calculation, and instead I added this commit: https://git.archive.org/archivecd/morituri/commit/8e108e239b15600315128b2e0cba0d4836f044d7 and set the value using the defacto discid library, which does get it right.

We probably do not want to use this hack, but I'm all for just throwing out the custom calculation code from morituri in favour of libdiscid.

@JoeLametta
Copy link
Collaborator

We probably do not want to use this hack, but I'm all for just throwing out the custom calculation code from morituri in favour of libdiscid.

👍

@JoeLametta JoeLametta added the Bug Generic bug: can be used together with more specific labels label Sep 25, 2018
@JoeLametta
Copy link
Collaborator

Related to #170 & #171.

@anarcat
Copy link
Contributor

anarcat commented Oct 5, 2018

@MerlijnWajer that repository is gone - is there a copy anywhere?

@MerlijnWajer
Copy link
Collaborator

I think they're doing a migration. It should be up in a few hours. :)

@anarcat
Copy link
Contributor

anarcat commented Oct 5, 2018

@MerlijnWajer awesome. in the meantime, care the paste the patch somewhere else? :) i'm blocked on this very bug...

@MerlijnWajer
Copy link
Collaborator

It is up again.

@anarcat
Copy link
Contributor

anarcat commented Oct 10, 2018

i see - that just completely disables the musicbrainz stuff, do i read this right?

@MerlijnWajer
Copy link
Collaborator

It removes the custom calculation code and instead just uses libdiscid, which works OK.

@JoeLametta JoeLametta added Accepted Accepted issue on our roadmap Priority: high High priority Needed: patch A pull request is required Needed: discussion More discussion needed before anything can be done (or still no agreement has been reached) labels Nov 13, 2018
@JoeLametta JoeLametta added this to the 2.0 milestone Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Bug Generic bug: can be used together with more specific labels Needed: discussion More discussion needed before anything can be done (or still no agreement has been reached) Needed: patch A pull request is required Priority: high High priority
Projects
None yet
4 participants