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

Save ISRCs from CD TOC #320

Closed
Freso opened this issue Nov 1, 2018 · 5 comments
Closed

Save ISRCs from CD TOC #320

Freso opened this issue Nov 1, 2018 · 5 comments
Labels
Accepted Accepted issue on our roadmap Feature New feature Needed: patch A pull request is required Priority: low Low priority Sprintable Small enough to sprint on Status: in progress Issue/pull request which is currently being worked on
Milestone

Comments

@Freso
Copy link
Member

Freso commented Nov 1, 2018

If the CD TOC contains ISRCs we should save those in the ripped file metadata. We're already reading the CD TOC and thus getting the ISRCs, so we may as well store them.

@JoeLametta
Copy link
Collaborator

Good suggestion!

BTW: ISRC values, if available, are already included in whipper's generated cue sheets, right?

@jtl999
Copy link
Contributor

jtl999 commented Nov 5, 2018

Yes...

REM DISCID FE0F0510
REM COMMENT "whipper 0.7.0"
PERFORMER "Arcade Fire"
TITLE "The Suburbs"
FILE "The Suburbs/01 - The Suburbs.flac" WAVE
  TRACK 01 AUDIO
    PERFORMER "Arcade Fire"
    TITLE "The Suburbs"
    ISRC GBUM71018417
    INDEX 01 00:00:00
FILE "The Suburbs/02 - Ready to Start.flac" WAVE
  TRACK 02 AUDIO
    PERFORMER "Arcade Fire"
    TITLE "Ready To Start "
    ISRC GBUM71021615
    INDEX 01 00:00:00

@JoeLametta JoeLametta added Feature New feature and removed enhancement labels Nov 11, 2018
@Freso
Copy link
Member Author

Freso commented Nov 12, 2018

Also noticed that whipper is using --fast-toc for cdrdao. If we want to save ISRCs, we may want to not do --fast-toc since that can result in "ISRC bleeding". See JonnyJD/musicbrainz-isrcsubmit#120

@JoeLametta
Copy link
Collaborator

Also noticed that whipper is using --fast-toc for cdrdao.

Actually I think we're using both:

# first, read the normal TOC, which is fast
print("Reading TOC...")
self.ittoc = self.program.getFastToc(self.runner, self.device)
# already show us some info based on this
self.program.getRipResult(self.ittoc.getCDDBDiscId())
sys.stdout.write("CDDB disc id: %s\n" % self.ittoc.getCDDBDiscId())
self.mbdiscid = self.ittoc.getMusicBrainzDiscId()
sys.stdout.write("MusicBrainz disc id %s\n" % self.mbdiscid)
sys.stdout.write("MusicBrainz lookup URL %s\n" %
self.ittoc.getMusicBrainzSubmitURL())
self.program.metadata = (
self.program.getMusicBrainz(self.ittoc, self.mbdiscid,
release=self.options.release_id,
country=self.options.country,
prompt=self.options.prompt)
)
if not self.program.metadata:
# fall back to FreeDB for lookup
cddbid = self.ittoc.getCDDBValues()
cddbmd = self.program.getCDDB(cddbid)
if cddbmd:
sys.stdout.write('FreeDB identifies disc as %s\n' % cddbmd)
# also used by rip cd info
if not getattr(self.options, 'unknown', False):
logger.critical("unable to retrieve disc metadata, "
"--unknown not passed")
return -1
self.program.result.isCdr = cdrdao.DetectCdr(self.device)
if (self.program.result.isCdr and
not getattr(self.options, 'cdr', False)):
logger.critical("inserted disc seems to be a CD-R, "
"--cdr not passed")
return -1
# now, read the complete index table, which is slower
self.itable = self.program.getTable(self.runner,
self.ittoc.getCDDBDiscId(),
self.ittoc.getMusicBrainzDiscId(),
self.device, self.options.offset)

Perhaps we should remove the fast-toc phase if we need reliable ISRCs for MusicBrainz metadata selection (and that seeems to cause "ISRC bleeding").

@jtl999
Copy link
Contributor

jtl999 commented Nov 12, 2018

I like the "slow" cdrdao reading TOC phase as I find the Q sub-channel CRC error log a good heuristic of how possibly damaged a CD is before ripping. Probably a "misuse" of this feature but it works for me.

I was thinking for #299 of parsing the cdrdao read-toc output and printing a representation of the TOC data to console/log and something like Track %i done, found %i Q sub-channels with CRC errors for each track?

@JoeLametta JoeLametta added this to the 2.0 milestone Nov 13, 2018
@JoeLametta JoeLametta added Accepted Accepted issue on our roadmap Priority: low Low priority Sprintable Small enough to sprint on Needed: patch A pull request is required labels Nov 13, 2018
JoeLametta added a commit that referenced this issue Sep 18, 2020
Fixes #320.

Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>
JoeLametta added a commit that referenced this issue Sep 18, 2020
Fixes #320.

Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>
JoeLametta added a commit that referenced this issue Sep 18, 2020
Fixes #320.

Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>
@JoeLametta JoeLametta added the Status: in progress Issue/pull request which is currently being worked on label Sep 18, 2020
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 Feature New feature Needed: patch A pull request is required Priority: low Low priority Sprintable Small enough to sprint on Status: in progress Issue/pull request which is currently being worked on
Projects
None yet
Development

No branches or pull requests

3 participants