Added --keep-going option to cd rip command #524

ghost · 2021-01-30T23:42:43Z

Implemented the option (-k, --keep-going) to continue ripping the CD even if one track fails to rip (as @xmixahlx suggested in #128). So this command:

whipper cd rip --keep-going --max-retries 8 -O some/folder

Would try to rip a problematic track 8 times, then continue on to the next track on the CD if the track fails to rip.

Signed-off-by: blueblots 63152708+blueblots@users.noreply.github.com

github-actions

💖 Thanks for opening your first pull request here! 💖

ghost · 2021-01-31T16:36:37Z

While doing some testing I noticed that if a track gets skipped then the AccurateRip\cuesheet generation stage will crash with a KeyError: "Cannot find file for 'some song.flac'"

Working on fixing this, please don't merge until I do

MerlijnWajer · 2021-01-31T16:51:54Z

Maybe it makes sense to return a non-zero exit code at the end of the process in case some tracks could not be fully ripped. Some tools automate CD ripping with whipper, and would potentially like to know if they should re-try a certain CD later. Either that, or perhaps printing a clear list of failed tracks to stderr at the end of a rip?

Thanks for working on this.

JoeLametta · 2021-01-31T17:06:31Z

Thanks for the pull request!
I think we should clearly mention the fact that the rip is incomplete in the logfile.

ghost · 2021-02-08T11:20:17Z

Hi everyone this should be ready to merge now. @JoeLametta I've added to the logger the capability to report that individual tracks have been skipped, if any tracks are skipped it will be mentioned in the Health report too.

@MerlijnWajer I could also add a non zero exit or a list of skipped tracks to stderr without much trouble, if this would be useful to users I can do this as well. Let me know if you (or anyone else) would like it in this PR.

whipper/command/cd.py

JoeLametta · 2021-02-09T15:39:37Z

blueblots:

Hi everyone this should be ready to merge now.

All right, I'll review and test it as soon as possible (tomorrow probably).

blueblots:

I could also add a non zero exit

I think that would be a worthwhile addition.

@itismadness As you've got more real world experience with whipper log parsing, does this pull request look OK to you?

whipper/command/cd.py

JoeLametta · 2021-02-14T21:27:18Z

@blueblots I've tested the code and it seems to be working. I've added some comments too.
Here are two questions:

What happens when no track can be ripped?
Do incomplete rips include meaningful AccurateRip information (log, stdout)?

One thing that still needs to be done is adding an entry about this option into whipper cd rip's manpage: I'll do it.

JoeLametta · 2021-02-15T10:33:08Z

Hi, I've noticed that the m3u file generated at the end of the rip references all tracks (and the associated files): even the ones which where skipped. Should this be fixed?

Example CD-R with skipped tracks 7, 9, 10 (m3u)

#EXTM3U
#EXTINF:426,./01. Unknown Artist - Unknown Track 1.flac
./01. Unknown Artist - Unknown Track 1.flac
#EXTINF:175,./02. Unknown Artist - Unknown Track 2.flac
./02. Unknown Artist - Unknown Track 2.flac
#EXTINF:301,./03. Unknown Artist - Unknown Track 3.flac
./03. Unknown Artist - Unknown Track 3.flac
#EXTINF:292,./04. Unknown Artist - Unknown Track 4.flac
./04. Unknown Artist - Unknown Track 4.flac
#EXTINF:234,./05. Unknown Artist - Unknown Track 5.flac
./05. Unknown Artist - Unknown Track 5.flac
#EXTINF:252,./06. Unknown Artist - Unknown Track 6.flac
./06. Unknown Artist - Unknown Track 6.flac
#EXTINF:231,./07. Unknown Artist - Unknown Track 7.flac
./07. Unknown Artist - Unknown Track 7.flac
#EXTINF:269,./08. Unknown Artist - Unknown Track 8.flac
./08. Unknown Artist - Unknown Track 8.flac
#EXTINF:360,./09. Unknown Artist - Unknown Track 9.flac
./09. Unknown Artist - Unknown Track 9.flac
#EXTINF:306,./10. Unknown Artist - Unknown Track 10.flac
./10. Unknown Artist - Unknown Track 10.flac
#EXTINF:440,./11. Unknown Artist - Unknown Track 11.flac
./11. Unknown Artist - Unknown Track 11.flac
#EXTINF:274,./12. Unknown Artist - Unknown Track 12.flac
./12. Unknown Artist - Unknown Track 12.flac
#EXTINF:266,./13. Unknown Artist - Unknown Track 13.flac
./13. Unknown Artist - Unknown Track 13.flac
#EXTINF:227,./14. Unknown Artist - Unknown Track 14.flac
./14. Unknown Artist - Unknown Track 14.flac
#EXTINF:283,./15. Unknown Artist - Unknown Track 15.flac
./15. Unknown Artist - Unknown Track 15.flac
#EXTINF:201,./16. Unknown Artist - Unknown Track 16.flac
./16. Unknown Artist - Unknown Track 16.flac

The cue sheet references all tracks too but the skipped ones don't have a corresponding FILE entry: is this all right?

Example CD-R with skipped tracks 7, 9, 10 (cue sheet)

REM DISCID 0211C310
REM COMMENT "whipper 0.9.9"
PERFORMER "Iron Maiden"
TITLE "Greatest Hits (CD1)"
FILE "./01. Unknown Artist - Unknown Track 1.flac" WAVE
  TRACK 01 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Phantom Of The Opera"
    INDEX 01 00:00:00
FILE "./02. Unknown Artist - Unknown Track 2.flac" WAVE
  TRACK 02 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Wrathchild "
    INDEX 01 00:00:00
FILE "./03. Unknown Artist - Unknown Track 3.flac" WAVE
  TRACK 03 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Killers "
    INDEX 01 00:00:00
FILE "./04. Unknown Artist - Unknown Track 4.flac" WAVE
  TRACK 04 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "The Number Of The Beast"
    INDEX 01 00:00:00
FILE "./05. Unknown Artist - Unknown Track 5.flac" WAVE
  TRACK 05 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Run To The Hills "
    INDEX 01 00:00:00
FILE "./06. Unknown Artist - Unknown Track 6.flac" WAVE
  TRACK 06 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "The Trooper "
    INDEX 01 00:00:00
  TRACK 07 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Flight Of Icarus "
    INDEX 01 28:04:09
FILE "./08. Unknown Artist - Unknown Track 8.flac" WAVE
  TRACK 08 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Aces High "
    INDEX 01 00:00:00
  TRACK 09 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "2 Minutes To Midnight "
    INDEX 01 36:25:63
  TRACK 10 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Wasted Years "
    INDEX 01 42:26:27
FILE "./11. Unknown Artist - Unknown Track 11.flac" WAVE
  TRACK 11 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Heaven Can Wait "
    INDEX 01 00:00:00
FILE "./12. Unknown Artist - Unknown Track 12.flac" WAVE
  TRACK 12 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "The Evil That Men Do "
    INDEX 01 00:00:00
FILE "./13. Unknown Artist - Unknown Track 13.flac" WAVE
  TRACK 13 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "The Clairvoyant "
    INDEX 01 00:00:00
FILE "./14. Unknown Artist - Unknown Track 14.flac" WAVE
  TRACK 14 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Holy Smoke "
    INDEX 01 00:00:00
FILE "./15. Unknown Artist - Unknown Track 15.flac" WAVE
  TRACK 15 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Bring Your Daughter To The Slaughter "
    INDEX 01 00:00:00
FILE "./16. Unknown Artist - Unknown Track 16.flac" WAVE
  TRACK 16 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Be Quick Or Be Dead "
    INDEX 01 00:00:00

ghost · 2021-02-15T17:20:15Z

What happens when no track can be ripped?

I don't have a CD I can test this with, but it should just skip over all of them and exit (after exceeding max-retries for each track).

Do incomplete rips include meaningful AccurateRip information (log, stdout)?

For tracks that are skipped, AccurateRip just reports that the track isn't found in the database. Tracks that are ripped are still checked normally. The log output of AccurateRip will state that skipped tracks are not accurate, which seems appropriate to me.

ghost · 2021-02-15T21:44:13Z

Hi, I've noticed that the m3u file generated at the end of the rip references all tracks (and the associated files): even the ones which where skipped. Should this be fixed?

Probably it should be, because someone might try to use it as a playlist and would have missing files from the list.

The cue sheet references all tracks too but the skipped ones don't have a corresponding FILE entry: is this all right?

I don't actually use cue sheets myself, and I'm not sure what exactly the appropriate use for them is currently. Some people say its for playlists, others for ripping the CD and there seems to be varying formats available. I'm not sure if the FILE entry is needed.

JoeLametta · 2021-02-16T15:12:00Z

What happens when no track can be ripped?

I don't have a CD I can test this with, but it should just skip over all of them and exit (after exceeding max-retries for each track).

I may check what happens by purposefully damaging the CD-R I've already used in my previous test...

Do incomplete rips include meaningful AccurateRip information (log, stdout)?

For tracks that are skipped, AccurateRip just reports that the track isn't found in the database. Tracks that are ripped are still checked normally. The log output of AccurateRip will state that skipped tracks are not accurate, which seems appropriate to me.

All right, that's fine.

Hi, I've noticed that the m3u file generated at the end of the rip references all tracks (and the associated files): even the ones which where skipped. Should this be fixed?

Probably it should be, because someone might try to use it as a playlist and would have missing files from the list.

I agree.

The cue sheet references all tracks too but the skipped ones don't have a corresponding FILE entry: is this all right?

I don't actually use cue sheets myself, and I'm not sure what exactly the appropriate use for them is currently. Some people say its for playlists, others for ripping the CD and there seems to be varying formats available. I'm not sure if the FILE entry is needed.

The purpose of the cue sheet in whipper should be to give users a way to burn the ripped audio tracks to a CD recreating the same layout (tracks order, indexes, gaps). Of course that requires that all the tracks must be ripped and available. After #115 will be implemented, then it could also be used as a playlist file.

TL;DR: Cue sheets of incomplete rips are nearly useless.

Some useful resources about cue sheets:

JoeLametta · 2021-02-16T15:33:41Z

Example CD-R with skipped tracks 7, 9, 10 (cue sheet)

REM DISCID 0211C310
REM COMMENT "whipper 0.9.9"
PERFORMER "Iron Maiden"
TITLE "Greatest Hits (CD1)"
FILE "./01. Unknown Artist - Unknown Track 1.flac" WAVE
  TRACK 01 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Phantom Of The Opera"
    INDEX 01 00:00:00
FILE "./02. Unknown Artist - Unknown Track 2.flac" WAVE
  TRACK 02 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Wrathchild "
    INDEX 01 00:00:00
FILE "./03. Unknown Artist - Unknown Track 3.flac" WAVE
  TRACK 03 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Killers "
    INDEX 01 00:00:00
FILE "./04. Unknown Artist - Unknown Track 4.flac" WAVE
  TRACK 04 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "The Number Of The Beast"
    INDEX 01 00:00:00
FILE "./05. Unknown Artist - Unknown Track 5.flac" WAVE
  TRACK 05 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Run To The Hills "
    INDEX 01 00:00:00
FILE "./06. Unknown Artist - Unknown Track 6.flac" WAVE
  TRACK 06 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "The Trooper "
    INDEX 01 00:00:00
  TRACK 07 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Flight Of Icarus "
    INDEX 01 28:04:09
FILE "./08. Unknown Artist - Unknown Track 8.flac" WAVE
  TRACK 08 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Aces High "
    INDEX 01 00:00:00
  TRACK 09 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "2 Minutes To Midnight "
    INDEX 01 36:25:63
  TRACK 10 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Wasted Years "
    INDEX 01 42:26:27
FILE "./11. Unknown Artist - Unknown Track 11.flac" WAVE
  TRACK 11 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Heaven Can Wait "
    INDEX 01 00:00:00
FILE "./12. Unknown Artist - Unknown Track 12.flac" WAVE
  TRACK 12 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "The Evil That Men Do "
    INDEX 01 00:00:00
FILE "./13. Unknown Artist - Unknown Track 13.flac" WAVE
  TRACK 13 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "The Clairvoyant "
    INDEX 01 00:00:00
FILE "./14. Unknown Artist - Unknown Track 14.flac" WAVE
  TRACK 14 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Holy Smoke "
    INDEX 01 00:00:00
FILE "./15. Unknown Artist - Unknown Track 15.flac" WAVE
  TRACK 15 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Bring Your Daughter To The Slaughter "
    INDEX 01 00:00:00
FILE "./16. Unknown Artist - Unknown Track 16.flac" WAVE
  TRACK 16 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Be Quick Or Be Dead "
    INDEX 01 00:00:00

Right now in whipper every FILE entry in the cue sheet corresponds to a relative TRACK entry. With the proposed implementation, when there are skipped tracks, the cue sheets generated are wrong because of the missing FILE entries.

Sample excerpt from the above cue sheet:

FILE "./06. Unknown Artist - Unknown Track 6.flac" WAVE
  TRACK 06 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "The Trooper "
    INDEX 01 00:00:00
  TRACK 07 AUDIO
    PERFORMER "Iron Maiden"
    TITLE "Flight Of Icarus "
    INDEX 01 28:04:09

Because of the missing FILE entry (for track 7), what happens is that both track 6 and 7 are sourced from the ripped file relative to track 6 only (which, of course, has an insufficient length to account for both).

I think there are two ways to "solve" this issue:

Avoid generating the cue sheet when incomplete rips are performed (probably unwanted)
Generate a cue sheet which always include all FILE entries (even for non-ripped tracks) maybe issuing a warning to inform the user about this (who may not know about the consequences)

@MerlijnWajer What's your opinion about this?

ghost · 2021-02-16T15:41:01Z

Wouldn't it be better if the skipped tracks were just omitted from the cue sheet? If the cue sheet references a non existent file then attempts to burn it to a CD might fail.

JuniorJPDJ · 2021-02-16T16:46:33Z

IMO it would be better to maintain rather complete cue sheets, with file entries pointing to non-existing tracks.
At least you'll have info about missing track, like length.
For playback you could just use m3u.

JoeLametta · 2021-02-16T18:44:21Z

Wouldn't it be better if the skipped tracks were just omitted from the cue sheet? If the cue sheet references a non existent file then attempts to burn it to a CD might fail.

I think that may cause issues with gaps (INDEX command).
Personally, apart from issuing a warning, I would prefer whipper to generate a cue sheet as if all tracks were ripped. Restoring the unconditional self.itable.setFile() and adding a warning should work, right?

ghost · 2021-02-17T20:16:28Z

OK I will go back and change this to putting a filename on the cue sheet whether or not the track rips.

ghost · 2021-02-19T11:03:18Z

~~@JoeLametta :~~
~~> Restoring the unconditional self.itable.setFile() and adding a warning should work, right?~~

The problem with that is the self.program.verifyImage() will then fail, because it tries to do a getRealPath() on each file the cue sheet refers to, so it will crash when it doesn't find a file corresponding to a skipped track. This is why I originally made it conditional on the track being ripped successfully.

~~Right now I'm looking for the best place to notify this function (verifyImage) that a track has been skipped, so it can ignore the fact that it has no file.~~

I was wondering if it would be better if there was some form of handling for missing files in general, right now it just crashes as described above. Would it defeat the purpose of verifying the track if a warning that the file is missing was issued instead of a crash?

See the latest commit, the above should no longer be issues.

JoeLametta · 2021-03-11T09:23:32Z

Sorry for the late reply: I still haven't tested the new changes in commit caeeeac. Is this pull request finished for you? If affirmative, I'll review it as soon as possible, add a warning about the non-functioning cue sheet and an entry regarding to this option in whipper's manpage.

ghost · 2021-03-11T20:35:08Z

Yes I'm pretty much finished with it, thank you for reviewing this.

MerlijnWajer · 2021-03-22T20:41:07Z

whipper/command/cd.py

+        if len(self.skipped_tracks) > 0:
+            logger.warning('%d tracks have been skipped from this rip attempt',
+                           len(self.skipped_tracks))
+            return 5


Why 5? I understand this is the exit code, but how did you pick 5?

5 is arbitrary, I just noticed that 5 wasn't being used for anything else.

Maybe exit codes should be documented somewhere user facing (ie., not just in the code)? And maybe it would be better to use powers of two for exit codes so bitmasking could potentially be used to check for multiple error conditions down the line?

@xmixahlx

Implemented the option (`-k`, `--keep-going`) to continue ripping the CD even if one track fails to rip (as @xmixahlx suggested in #128). Requested in #128 Signed-off-by: blueblots <63152708+blueblots@users.noreply.github.com> Changed line-lengths/indentation of some code Travis-CI was failing on account of lines being under-indented or too long, this should correct it. Signed-off-by: blueblots <63152708+blueblots@users.noreply.github.com>

Made changes to fix `KeyError` and implemented logging of skipped tracks. For instance: if any tracks are skipped, the log will show: `Health status: Some tracks were not ripped (skipped)`. the tracks that are skipped will show: `Status: Track not ripped (skipped)`. Signed-off-by: blueblots <63152708+blueblots@users.noreply.github.com>

Added an exit status of 5 when tracks are skipped during a rip attempt. Fixed a TypeError caused by a syntax error in the format string on line 537 in `whipper/command/cd.py`. Changed f-string to printf-style format string on line 493 in `whipper/command/cd.py`. Signed-off-by: blueblots <63152708+blueblots@users.noreply.github.com>

Removed `return None` from the `Rip.doCommand` method as suggested in review comments. Changed logging strings to use logger arguments rather than printf-string, as suggested in review comments. Signed-off-by: blueblots <63152708+blueblots@users.noreply.github.com>

Added conditional to `program.write_m3u()` to ignore skipped tracks. Added skipped_tracks support to the `Program` and `image.ImageVerifyTask` classes to avoid crashing when a file for a skipped track doesn't exist. Added conditional to `accurip.calculate_checksums` to check if a path exists before trying to calculate checksums, this prevents `accuraterip-checksum.c` from emitting an error message (`sf_open failed!`) when a path doesn't exist (as when a track is skipped). Signed-off-by: blueblots <63152708+blueblots@users.noreply.github.com>

This happens when a track fails to be ripped and gets skipped. Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>

Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>

JoeLametta · 2021-03-28T16:26:28Z

Merged, thanks!

wisp3rwind · 2021-05-01T07:48:33Z

While doing some testing I noticed that if a track gets skipped then the AccurateRip\cuesheet generation stage will crash with a KeyError: "Cannot find file for 'some song.flac'"

Working on fixing this, please don't merge until I do

On 84e4ef6

> whipper --version
whipper 0.9.1.dev118+g84e4ef6

this error is still present:

> whipper cd rip  --keep-going --max-retries 8
INFO:whipper.command.cd:using configured read offset 48
INFO:whipper.command.cd:checking device /dev/sr0
WARNING:whipper.program.utils:command 'eject -t /dev/sr0' returned with exit code '1' (b'eject: CD-ROM tray close command failed: Input/output error')
Device /dev/sr0 is mounted, unmounting
CDDB disc id: be0cb50f
MusicBrainz disc id v.0PpdpPZ4klJJLSyhAfJSjUkic-
MusicBrainz lookup URL https://musicbrainz.org/cdtoc/attach?toc=1+15+244194+150+7719+21916+37614+49946+71689+87081+104566+121193+135956+150712+167063+195696+212937+230036&tracks=15&id=v.0PpdpPZ4klJJLSyhAfJSjUkic-
Disc duration: 00:54:13.920, 15 audio tracks

Matching releases:

Artist  : Linkin Park
Title   : Minutes to Midnight (Tour Edition)
Duration: 00:54:14.609
URL     : https://musicbrainz.org/release/630ea5d2-7139-4d30-81b4-f6a8fb59a73a
Release : 630ea5d2-7139-4d30-81b4-f6a8fb59a73a
Type    : Album
Barcode : 093624989349
Country : Europe
Cat no  : 9362-49893-4

Track 1 finished, found 57 Q sub-channels with CRC errors
Track 2 finished, found 126 Q sub-channels with CRC errors
Track 3 finished, found 167 Q sub-channels with CRC errors
Track 4 finished, found 101 Q sub-channels with CRC errors
Track 5 finished, found 98 Q sub-channels with CRC errors
Track 6 finished, found 127 Q sub-channels with CRC errors
Track 7 finished, found 149 Q sub-channels with CRC errors
Track 8 finished, found 100 Q sub-channels with CRC errors
Track 9 finished, found 111 Q sub-channels with CRC errors
Track 10 finished, found 180 Q sub-channels with CRC errors
Track 11 finished, found 239 Q sub-channels with CRC errors
Track 12 finished, found 176 Q sub-channels with CRC errors
Track 13 finished, found 116 Q sub-channels with CRC errors
Track 14 finished, found 180 Q sub-channels with CRC errors
Track 15 finished, found 254 Q sub-channels with CRC errors
INFO:whipper.command.cd:ripping track 1 of 15: 01. Linkin Park - Wake.flac
INFO:whipper.program.cdparanoia:checksums match, 531a14fa
INFO:whipper.command.cd:CRCs match for track 1
Peak level: 0.904724
Rip quality: 100.00%
INFO:whipper.command.cd:ripping track 2 of 15: 02. Linkin Park - Given Up.flac
INFO:whipper.program.cdparanoia:checksums match, 20eed3c4
INFO:whipper.command.cd:CRCs match for track 2
Peak level: 0.976044
Rip quality: 100.00%
INFO:whipper.command.cd:ripping track 3 of 15: 03. Linkin Park - Leave Out All the Rest.flac
INFO:whipper.program.cdparanoia:checksums match, 6fc1910e
INFO:whipper.command.cd:CRCs match for track 3
Peak level: 0.977173
Rip quality: 100.00%
INFO:whipper.command.cd:ripping track 4 of 15: 04. Linkin Park - Bleed It Out.flac
INFO:whipper.program.cdparanoia:checksums match, fc4baea5
INFO:whipper.command.cd:CRCs match for track 4
Peak level: 0.976074
Rip quality: 100.00%
INFO:whipper.command.cd:ripping track 5 of 15: 05. Linkin Park - Shadow of the Day.flac
INFO:whipper.program.cdparanoia:checksums match, 99a2c419
INFO:whipper.command.cd:CRCs match for track 5
Peak level: 0.977203
Rip quality: 100.00%
INFO:whipper.command.cd:ripping track 6 of 15: 06. Linkin Park - What I've Done.flac
INFO:whipper.program.cdparanoia:checksums match, 5f14852d
INFO:whipper.command.cd:CRCs match for track 6
Peak level: 0.977173
Rip quality: 100.00%
INFO:whipper.command.cd:ripping track 7 of 15: 07. Linkin Park - Hands Held High.flac
INFO:whipper.program.cdparanoia:checksums match, 32bf07c9
INFO:whipper.command.cd:CRCs match for track 7
Peak level: 0.977173
Rip quality: 100.00%
INFO:whipper.command.cd:ripping track 8 of 15: 08. Linkin Park - No More Sorrow.flac
INFO:whipper.program.cdparanoia:checksums match, 19e3a3b5
INFO:whipper.command.cd:CRCs match for track 8
Peak level: 0.977173
Rip quality: 100.00%
INFO:whipper.command.cd:ripping track 9 of 15: 09. Linkin Park - Valentine's Day.flac
INFO:whipper.program.cdparanoia:checksums match, 2aa40215
INFO:whipper.command.cd:CRCs match for track 9
Peak level: 0.976074
Rip quality: 100.00%
INFO:whipper.command.cd:ripping track 10 of 15: 10. Linkin Park - In Between.flac
INFO:whipper.program.cdparanoia:checksums match, 3e096776
INFO:whipper.command.cd:CRCs match for track 10
Peak level: 0.972534
Rip quality: 100.00%
INFO:whipper.command.cd:ripping track 11 of 15: 11. Linkin Park - In Pieces.flac
INFO:whipper.program.cdparanoia:checksums do not match, 1f3fa7ca bd2178c4
INFO:whipper.command.cd:ripping track 11 of 15 (try 2): 11. Linkin Park - In Pieces.flac
INFO:whipper.program.cdparanoia:checksums do not match, fcf021a9 0836840d
INFO:whipper.command.cd:ripping track 11 of 15 (try 3): 11. Linkin Park - In Pieces.flac
INFO:whipper.program.cdparanoia:checksums do not match, 0d99c12c 9bfd4a20
INFO:whipper.command.cd:ripping track 11 of 15 (try 4): 11. Linkin Park - In Pieces.flac
INFO:whipper.program.cdparanoia:checksums do not match, 93bc30d3 1071e8b5
INFO:whipper.command.cd:ripping track 11 of 15 (try 5): 11. Linkin Park - In Pieces.flac
INFO:whipper.program.cdparanoia:checksums do not match, f72259bc ca7f5dde
INFO:whipper.command.cd:ripping track 11 of 15 (try 6): 11. Linkin Park - In Pieces.flac
INFO:whipper.program.cdparanoia:checksums do not match, 54c517cc bcd18114
INFO:whipper.command.cd:ripping track 11 of 15 (try 7): 11. Linkin Park - In Pieces.flac
INFO:whipper.program.cdparanoia:checksums do not match, 835f229d 2b56cc29
INFO:whipper.command.cd:ripping track 11 of 15 (try 8): 11. Linkin Park - In Pieces.flac
INFO:whipper.program.cdparanoia:checksums do not match, 5bccd1f2 c7c74482
CRITICAL:whipper.command.cd:giving up on track 11 after 8 times
WARNING:whipper.command.cd:track 11 failed to rip.
Skipping CRC comparison for track 11 due to rip failure
INFO:whipper.command.cd:ripping track 12 of 15: 12. Linkin Park - The Little Things Give You Away.flac
INFO:whipper.program.cdparanoia:checksums match, 203af497
INFO:whipper.command.cd:CRCs match for track 12
Peak level: 0.977112
Rip quality: 100.00%
INFO:whipper.command.cd:ripping track 13 of 15: 13. Linkin Park - No Roads Left.flac
INFO:whipper.program.cdparanoia:checksums match, 517e449f
INFO:whipper.command.cd:CRCs match for track 13
Peak level: 0.998871
Rip quality: 100.00%
INFO:whipper.command.cd:ripping track 14 of 15: 14. Linkin Park - What I've Done (Distorted remix).flac
INFO:whipper.program.cdparanoia:checksums match, efa7d5f8
INFO:whipper.command.cd:CRCs match for track 14
Peak level: 0.988556
Rip quality: 100.00%
INFO:whipper.command.cd:ripping track 15 of 15: 15. Linkin Park - Given Up (Third Encore session).flac
INFO:whipper.program.cdparanoia:checksums match, 67f9e1dd
INFO:whipper.command.cd:CRCs match for track 15
Peak level: 0.988556
Rip quality: 100.00%
WARNING:whipper.command.cd:the generated cue sheet references 1 track(s) which failed to rip so the associated file(s) won't be available
INFO:whipper.image.cue:parsing .cue file 'album/Linkin Park - Minutes to Midnight (Tour Edition)/Linkin Park - Minutes to Midnight (Tour Edition).cue'
Traceback (most recent call last):
  File "/usr/bin/whipper", line 33, in <module>
    sys.exit(load_entry_point('whipper==0.9.1.dev118+g84e4ef6', 'console_scripts', 'whipper')())
  File "/usr/lib/python3.9/site-packages/whipper/command/main.py", line 56, in main
    ret = cmd.do()
  File "/usr/lib/python3.9/site-packages/whipper/command/basecommand.py", line 141, in do
    return self.cmd.do()
  File "/usr/lib/python3.9/site-packages/whipper/command/basecommand.py", line 141, in do
    return self.cmd.do()
  File "/usr/lib/python3.9/site-packages/whipper/command/cd.py", line 195, in do
    ret = self.doCommand()
  File "/usr/lib/python3.9/site-packages/whipper/command/cd.py", line 575, in doCommand
    self.program.verifyImage(self.runner, self.itable)
  File "/usr/lib/python3.9/site-packages/whipper/common/program.py", line 617, in verifyImage
    verifytask = image.ImageVerifyTask(cueImage,
  File "/usr/lib/python3.9/site-packages/whipper/image/image.py", line 151, in __init__
    path = image.getRealPath(index.path)
  File "/usr/lib/python3.9/site-packages/whipper/image/image.py", line 70, in getRealPath
    return self.cue.getRealPath(path)
  File "/usr/lib/python3.9/site-packages/whipper/image/cue.py", line 188, in getRealPath
    return common.getRealPath(self._path, path)
  File "/usr/lib/python3.9/site-packages/whipper/common/common.py", line 252, in getRealPath
    raise KeyError("Cannot find file for %r" % filePath)
KeyError: "Cannot find file for './11. Linkin Park - In Pieces.flac'"

ghost · 2021-05-04T19:09:20Z

@wisp3rwind , thank you for reporting this. I will look into this by the end of the week.

In the meantime, could you let me know how you installed whipper, e.g. is this from Github or a package from a Linux distro?

wisp3rwind · 2021-05-04T20:34:33Z

Hi @blueblots, this is on Arch, whipper is whipper-git from the AUR. Thanks for looking into this, and your work on this features in general!

ghost · 2021-05-09T16:31:34Z

Hi @wisp3rwind , I haven't been able to replicate the crash on my end so far (using both the source and the AUR package on Arch). Are there any more details you can provide that may be relevant?

wisp3rwind · 2021-05-09T19:17:08Z

Hi @wisp3rwind , I haven't been able to replicate the crash on my end so far (using both the source and the AUR package on Arch). Are there any more details you can provide that may be relevant?

Not at the moment, I don't have access to the faulty disc right now to get a debug log. I can probably do so in a week. I skimmed the code, and I'd guess that the problem is somehow related to how paths are stored in the skipped_tracks list: In

whipper/whipper/common/program.py

Lines 617 to 619 in 84e4ef6

    
           verifytask = image.ImageVerifyTask(cueImage, 
        
                                              [os.path.basename(t.filename) 
        
                                               for t in self.skipped_tracks])

basenames are added to this list, i.e. "11. Linkin Park - In Pieces.flac" whereas the check in

whipper/whipper/image/image.py

Lines 152 to 160 in 84e4ef6

    
           except KeyError: 
        
               logger.debug('Path not found; Checking ' 
        
                            'if %s is a skipped track', index.path) 
        
               if index.path in skipped_tracks: 
        
                   logger.warning('Missing file %s due to skipped track', 
        
                                  index.path) 
        
                   continue 
        
               else: 
        
                   raise

compares that path to the one stored in the cue file, which is "./11. Linkin Park - In Pieces.flac", note the initial ./ (at least I couldn't find any code that would strip the ./). I think the existing logger.debug() statements should be sufficient to verify (or reject) that hypthesis once I can try to rip the CD again.

JoeLametta · 2021-05-14T14:03:57Z

@wisp3rwind I've checked and I agree with your diagnosis. It seems that when ripping to an output directory (-O OUTPUT_DIRECTORY, --output-directory OUTPUT_DIRECTORY) index.path contains only the filename but when that option isn't specified (and it defaults to the current directory) or -W WORKING_DIRECTORY, --working-directory WORKING_DIRECTORY is used, then index.path becomes ./ + filename which fails the if check:

whipper/whipper/image/image.py

Line 155 in 84e4ef6

if index.path in skipped_tracks:

The issue can be fixed adding a os.path.basename() to that if check a/o debugging why there's this discrepancy between the two cases...

ghost · 2021-05-15T17:13:22Z

I've looked at it today myself and saw the same issue with that if statement, I also realized that my original tests always used the -O option which avoids the issue.

I'll be sending the fix shortly, once I fully test it.

ghost · 2021-05-15T17:42:51Z

@JoeLametta would it be better to just do the os.path.basename at the if statement or should the code that appends the ./ be changed instead (unless there is a good reason for it)?

Edit: I think the ./ is coming from whipper/common/program.py's getPath() function (line 228 at the return statement).
https://www.github.com/whipper-team/whipper/tree/develop/whipper%2Fcommon%2Fprogram.py

Edit 2: Having read the code further, I think it would be best to fix the if statement rather than edit getPath + everything attached to it.

ghost · 2021-05-16T16:14:23Z

@JoeLametta I've opened a pull request for the fix, its on #537. I was able to reproduce the crash on my end after omitting the -O option.

I haven't pinpointed yet where the ./ is getting added to the index.path, but I will keep looking.

JoeLametta · 2021-05-16T16:47:01Z

@JoeLametta I've opened a pull request for the fix, its on #537. I was able to reproduce the crash on my end after omitting the -O option.

All right, I'm waiting for a test to complete before merging it. 👌

I haven't pinpointed yet where the ./ is getting added to the index.path, but I will keep looking.

Hi, I think it's getting added here:

whipper/whipper/common/program.py

Line 242 in b6338b2

return os.path.join(outdir, template % v_fltr)

When the output directory option isn't specified, it defaults to . and when this path is merged with the populated template it results in something like ./album/Author - Artist/bla bla.
I've already committed a tiny fix in my local repository which I'm going to publish in a few minutes...

JoeLametta · 2021-05-16T17:25:51Z

The fix has been published in the develop branch (commit 731453e).

wisp3rwind · 2021-05-23T10:07:03Z

On current git tip, this specific error appears to be gone. Now I'm getting the following error (which is probably unrelated):

WARNING:whipper.command.cd:the generated cue sheet references 2 track(s) which failed to rip so the associated file(s) won't be available
INFO:whipper.image.cue:parsing .cue file 'album/Linkin Park - Minutes to Midnight (Tour Edition)/Linkin Park - Minutes to Midnight (Tour Edition).cue'
WARNING:whipper.image.image:Missing file 03. Linkin Park - Leave Out All the Rest.flac due to skipped track
WARNING:whipper.image.image:Missing file 11. Linkin Park - In Pieces.flac due to skipped track
Traceback (most recent call last):
  File "/usr/bin/whipper", line 33, in <module>
    sys.exit(load_entry_point('whipper==0.10.1.dev3+g2d34d53', 'console_scripts', 'whipper')())
  File "/usr/lib/python3.9/site-packages/whipper/command/main.py", line 56, in main
    ret = cmd.do()
  File "/usr/lib/python3.9/site-packages/whipper/command/basecommand.py", line 141, in do
    return self.cmd.do()
  File "/usr/lib/python3.9/site-packages/whipper/command/basecommand.py", line 141, in do
    return self.cmd.do()
  File "/usr/lib/python3.9/site-packages/whipper/command/cd.py", line 203, in do
    ret = self.doCommand()
  File "/usr/lib/python3.9/site-packages/whipper/command/cd.py", line 583, in doCommand
    self.program.verifyImage(self.runner, self.itable)
  File "/usr/lib/python3.9/site-packages/whipper/common/program.py", line 648, in verifyImage
    responses = accurip.get_db_entry(table.accuraterip_path())
  File "/usr/lib/python3.9/site-packages/whipper/common/accurip.py", line 152, in get_db_entry
    raw_entry = _download_entry(path)
  File "/usr/lib/python3.9/site-packages/whipper/common/accurip.py", line 140, in _download_entry
    with urlopen(Request(url, headers={'User-Agent': UA})) as resp:
  File "/usr/lib/python3.9/urllib/request.py", line 214, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/lib/python3.9/urllib/request.py", line 517, in open
    response = self._open(req, data)
  File "/usr/lib/python3.9/urllib/request.py", line 534, in _open
    result = self._call_chain(self.handle_open, protocol, protocol +
  File "/usr/lib/python3.9/urllib/request.py", line 494, in _call_chain
    result = func(*args)
  File "/usr/lib/python3.9/urllib/request.py", line 1375, in http_open
    return self.do_open(http.client.HTTPConnection, req)
  File "/usr/lib/python3.9/urllib/request.py", line 1350, in do_open
    r = h.getresponse()
  File "/usr/lib/python3.9/http/client.py", line 1345, in getresponse
    response.begin()
  File "/usr/lib/python3.9/http/client.py", line 307, in begin
    version, status, reason = self._read_status()
  File "/usr/lib/python3.9/http/client.py", line 276, in _read_status
    raise RemoteDisconnected("Remote end closed connection without"
http.client.RemoteDisconnected: Remote end closed connection without response

However, my network is pretty stable, and the same error occured again when repeating the rip.

ghost · 2021-05-23T15:47:37Z

I've never had an error like that before... is it possible that Accuraterip's server was down/under maintenance?

In any case it would probably be better if there was some try/catch clause to prevent it from crashing when accurip doesn't respond for some reason.

JoeLametta · 2021-05-26T18:18:08Z

is it possible that Accuraterip's server was down/under maintenance?

Don't know but it seems possible (Server: Microsoft-IIS/10.0, X-Powered-By: ASP.NET).

@wisp3rwind Maybe it's better to discuss this in a new issue.

Similar to #522.

github-actions bot reviewed Jan 30, 2021

View reviewed changes

JoeLametta reviewed Feb 9, 2021

View reviewed changes

whipper/command/cd.py Outdated Show resolved Hide resolved