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

Fails on CDs with CD-Text #17

Closed
chirlu opened this issue Aug 29, 2012 · 7 comments

Comments

@chirlu
Copy link
Contributor

commented Aug 29, 2012

Here is an extract from a cdrdao TOC (of http://musicbrainz.org/release/f1c048a3-69cc-4906-9ce1-3bfbfc76f951):

// Track 1
TRACK AUDIO
NO COPY
NO PRE_EMPHASIS
TWO_CHANNEL_AUDIO
ISRC "DED811023010"
CD_TEXT {
  LANGUAGE 0 {
    TITLE "Get The Party Started"
    PERFORMER "VIVA VOCE"
    ISRC ""
  }
}
FILE "data.wav" 0 02:46:58


// Track 2
TRACK AUDIO
NO COPY
NO PRE_EMPHASIS
TWO_CHANNEL_AUDIO
ISRC "DED811023020"
CD_TEXT {
  LANGUAGE 0 {
    TITLE "Die Welt, ein Dorf"
    PERFORMER "VIVA VOCE"
    ISRC "DED811023020"
  }
}
FILE "data.wav" 02:46:58 04:00:59

As you’ll see, the CD has CD-Text which also includes ISRCs for each track (though, for some reason, it is not set on track 1). The parser doesn’t expect two lines containing “ISRC” and fails:

found new ISRC for track 1: DED811023010
Traceback (most recent call last):
  File "./isrcsubmit.py", line 658, in <module>
    track = tracks[trackNumber + trackOffset - 1]
TypeError: unsupported operand type(s) for +: 'NoneType' and 'int'

Probably the simplest fix would be to ignore all lines starting with a space.

@chirlu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2012

I was able to fix it with this change:

--- isrcsubmit.py   2012-08-29 14:08:03.000000000 +0200
+++ isrcsubmit_mod_f_cdtext.py  2012-08-29 16:17:09.000000000 +0200
@@ -313,7 +313,7 @@
             with open(tmpfile, "r") as toc:
                 for line in toc:
                     words = line.split()
-                    if len(words) > 0:
+                    if not line.startswith(" ") and len(words) > 0:
                         if words[0] == "//":
                             trackNumber = int(words[2])
                         elif words[0] == "ISRC":

Note that this is the very first time I “develop” in Python and I found out about the syntax by looking around in the script, so it’s certainly not optimal. (Why first split the line, then test whether to ignore it?)

@JonnyJD

This comment has been minimized.

Copy link
Owner

commented Oct 13, 2012

Sorry, I must have overlooked this issue/notification mail.
Thank you for the contribution.

Now there is also pull request #18 and I received a patch per email.
I will fix it quite soon.

@ghost ghost assigned JonnyJD Oct 13, 2012

@Freso

This comment has been minimized.

Copy link
Collaborator

commented Oct 14, 2012

Heh. I need to remember to check issues on GitHub. :)

Anyway, AFAICT, the proposed edit here simply ignores ISRCs stored in CD-TEXT. If there is a CD which doesn't use the "ISRC" field on its own but stores it in CD-TEXT->ISRC, this code change would prevent it from being used. My pull request just makes it disregard assigning ISRCs to trackNumber None. JonnyJD knows the code best though, so whatever he decides is the right way to fix it, I'll trust that decision.

@JonnyJD

This comment has been minimized.

Copy link
Owner

commented Oct 14, 2012

Jim Patterson sent me another case with ISRC in CD-Text, but with hyphens.
http://musicbrainz.org/release/3e0b924d-ccaa-46f1-a29b-94fab98f4f4c

// Track 1
TRACK AUDIO
NO COPY
NO PRE_EMPHASIS
TWO_CHANNEL_AUDIO
ISRC "USSM19806736"
CD_TEXT {
  LANGUAGE 0 {
    TITLE "Ready To Run"
    PERFORMER "DIXIE CHICKS"
    DISC_ID ""
    ISRC "US-SM1-98-06736"
  }
}
FILE "data.wav" 0 03:52:40


// Track 2
TRACK AUDIO
NO COPY
NO PRE_EMPHASIS
TWO_CHANNEL_AUDIO
ISRC "USSM19901547"
CD_TEXT {
  LANGUAGE 0 {
    TITLE "If I Fall You're Going Down With Me"
    PERFORMER "DIXIE CHICKS"
    DISC_ID ""
    ISRC "US-SM1-99-01547"
  }
}
FILE "data.wav" 03:52:40 03:05:02
@JonnyJD

This comment has been minimized.

Copy link
Owner

commented Oct 14, 2012

Even more important: Track 13 on the same disc has an ISRC given only in CD-Text, but is not found "the normal way".

// Track 13
TRACK AUDIO
NO COPY
NO PRE_EMPHASIS
TWO_CHANNEL_AUDIO
CD_TEXT {
  LANGUAGE 0 {
    TITLE "Ain't No Thang But A Chickin' Wang"
    PERFORMER "DIXIE CHICKS"
    ISRC "US-SM1-99-07390"
  }
}
FILE "data.wav" 44:54:67 00:06:00
@Freso

This comment has been minimized.

Copy link
Collaborator

commented Oct 14, 2012

I've added a fix for the hyphens to my pull request. (Not tested though, so please do to make sure it actually works. :p)

And the case about the ISRC should be handled fine by my cahnge too, unlike the code in comment #1. In the pull request, it should just add the ISRC in the CD-TEXT to the trackNumber without any issue.

@JonnyJD JonnyJD closed this in 603aeb3 Oct 14, 2012

@JonnyJD

This comment has been minimized.

Copy link
Owner

commented Oct 14, 2012

Please tell me if there are still issues after my octupus merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.