Remove dead code from program.getFastToc #264

mtdcr · 2018-04-21T19:02:50Z

With these changes I can finally use all of my drives.

Should fix or obsolete issues #72 and #173.

I compared read-toc and read-toc --fast-toc with many CDs in six different drives. With two of my drives, cdrdao created bad results, i.e. ISRC numbers with only one nonzero digit and weird time stamps:

MATSHITA DVD-RAM UJ-842   RB01
MATSHITA DVD-R   UJ-857E  ZA0E

For all other drives, read-toc --fast-toc always created the exact same result as without --fast-toc.

mtdcr · 2018-04-21T19:21:35Z

./whipper/program/cdrdao.py:30:9: E128 continuation line under-indented for visual indent

Your CI test complains about a line that wasn't modified.

MerlijnWajer · 2018-04-22T14:58:09Z

Thanks for this. I too have done something similar in a whipper fork and still don't understand why we need this in the first place. I'll need to take a closer look soon, but on first look this looks fine.

JoeLametta · 2018-04-26T09:24:00Z

For all other drives, read-toc --fast-toc always created the exact same result as without --fast-toc.

Hmm... according to cdrdao's manpage the --fast-toc option isn't lossless:

--fast-toc
    Only used for command read-toc. This option suppresses the pre-gap length and index mark extraction which speeds up the read-toc process. Standard 2 second pre-gaps (but no silence!) will be placed into the toc-file. The resulting CD will sound like the source CD. Only the CD player's display will behave slightly different in the transition area between two tracks.

    This option might help, too, if read-toc fails with your drive otherwise.

To me it looks like this option introduces the following limitations:

No real pre-gap detection (forced 2s). [Breaks gapless CDs?]
No additional index marks for each track (INDEX 02-99 not extracted). [Example usage of this feature: link]
No HTOA detection? [Because of 1?]

MerlijnWajer · 2018-04-26T10:07:46Z

HTOA should work with --fast-toc. I think alt. INDEX is also extracted.

mtdcr · 2018-04-27T00:17:04Z

I found one CD containing index marks, and --fast-toc does not extract them, unfortunately.

However, I noticed that the cue sheet of this CD generated by whipper only contains INDEX 01 and INDEX 02, while cdrdao read-toc outputs eight INDEX lines for the very same track.

I'll create a new rip to check for reproducibility.

I think it would be useful if whipper saved the TOC to a file inside of the target directoy, so subsequent invocations for failed rips could reuse them, possibly from different computers via a network share.

The two drives I listed above, which fail to read almost all TOCs without the --fast-toc option, are by far my best drives when it comes to reading discs in bad condition, and both drives are built into notebooks.

MerlijnWajer · 2018-04-27T07:01:22Z

What CD would this be?

As for caching the toc files, that has made my life a real pain at times, and I think whipper might do it, or might have done it at some point. But if it no longer does that, then please let'd not re-add that. It might be nice to save it -along- with the final rip, but not in any intermediate state.

What drives do you use?

mtdcr · 2018-04-27T19:35:32Z

The CD was Martin Buntrock - Meer: https://musicbrainz.org/release/04c6de86-a7f8-41c1-97b2-ab96e2b06dea

I did a new rip, and this time all index marks were added to the cue sheet.

I think whipper currently caches the TOC, but with one cache entry per drive offset, somewhere below ~/.cache/whipper/.

I guess an alternative to caching the TOC would be to make the slow toc reading optional, so I could rip my scratched disks with the good drives, and then try to create a new cue sheet with one of the other drives.

Drives I use, roughly sorted from worst to best error correction capabilities:

TOSHIBA ODD-DVD SD-R6372
HL-DT-ST DVD-RAM GHC0N
PIONEER BD-RW BDR-209M
HL-DT-ST DVDRAM GT30N
MATSHITA DVD-RAM UJ-842
MATSHITA DVD-R UJ-857E

I wonder how CD players read the index marks. They surely don't need to scan the entire disk.

mtdcr · 2018-04-27T19:39:23Z

Btw., maybe you could take just the first commit of this PR („Remove dead code from program.getFastToc“).

JoeLametta · 2018-04-27T22:20:23Z

@MerlijnWajer Using a search engine It's quite easy to find CDs which have index marks > 01: relying on EAC's generated cue sheets, just search for "REM DISCID" "INDEX 02" (include both keywords). Some CDs of this kind are also mentioned in the link included in my previous message.
I've currently got one which has INDEX 01, 02, 03 on a single track.

No real pre-gap detection (forced 2s). [Breaks gapless CDs?]
No HTOA detection? [Because of 1?]

From a quick test it seems I'm wrong (the manual seems to be too). Need to retest with more care ASAP.

No additional index marks for each track (INDEX 02-99 not extracted). [Example usage of this feature: link]

This one seems to be confirmed by what @mtdcr reported.

Ideas for further research:

Some of the information stated in the ToC can differ from what's in the subcode (pre-emphasis, etc.): what happens, in those cases, using cdrdao's fast-toc?
Does cdrdao retrieve the ISRC/MCN in fast-toc mode?
cdda2wav provides a different paranoia integration + other interesting features. It can use C2 pointers for analysis too.

Btw., maybe you could take just the first commit of this PR („Remove dead code from program.getFastToc“).

👍

RecursiveForest

I definitely want to keep accurate pre-gap support. I will do some research as well.

+1 to merging just 168d1f8ba97eb5c8402822f9ff86439c05256695

I like the idea of whipper writing a .toc file with the rip results, or at least logging the contents of it on disk.

whipper at some point in the past has supported caching the the toc & rip tables. However, it's a feature I've wanted to remove even if it's not already broken.

RecursiveForest · 2018-04-29T07:20:05Z

whipper/common/program.py

+        from pkg_resources import parse_version as V
+        version = cdrdao.getCDRDAOVersion()
+        if V(version) < V('1.2.3rc2'):
+            sys.stdout.write('Warning: cdrdao older than 1.2.3 has a '


nitpick: indent the continued string lines below

RecursiveForest · 2018-04-29T07:22:56Z

whipper/common/program.py

-            ptoc.persist(t.table)
-        toc = ptoc.object
+        t = cdrdao.ReadTOCTask(device)
+        toc = t.table


toc = cdrdao.ReadTOCTask(device).table

MerlijnWajer · 2018-04-29T10:12:16Z

@JoeLametta IA uses --fast-toc exclusively and I'm pretty sure it can rip HTOA and also get ISRC codes and such. I've spent quite some time doing vimdiff's and such of the various modes on most of the CDs I have. I can do it in a bit on this 101 bird songs CD. Actually, I'll do that now.

I have not tested if it also works for CDs that have many tracks on other indices, but I would be surprised if it doesn't work. Yes, if you care about accurate pregaps, then you will have no option but to not use --fast-toc.

I'm against saving the toc (or anything really) with the intent to use it to resume ripping (other purposes - great). IMHO that's just a hack and makes the code more dirty/complex. (I know we currently support resuming rips, kind of)

MerlijnWajer · 2018-04-29T12:57:11Z

OK, just tested it. Indeed with --fast-toc the four indexes on track 99 are not seen. Instead, they are all mended into a single track, so you still seemingly get all the audio, but some metadata is lost)

That's a good argument to not use --fast-toc for whipper. (As for IA, I am not sure if we are likely to change this, given the immense perf hit). HTOA works though.

The caching mechanism didn's have any effect. The inline function 'function' wasn't used anymore.

mtdcr · 2018-05-07T18:08:18Z

I've update my branch:

Dropped the patch removing slow toc
Adjusted the clean-up patch to include suggestions by @RecursiveForest

RecursiveForest · 2018-05-07T18:51:10Z

This changeset looks good to me.
I don't care very much about resuming rips.
I like storing the toc for archival purposes only.

MerlijnWajer

LGTM

JoeLametta · 2018-05-08T06:46:54Z

I've update my branch:

Dropped the patch removing slow toc
Adjusted the clean-up patch to include suggestions by @RecursiveForest

Merged, thanks!

RecursiveForest reviewed Apr 29, 2018

View reviewed changes

Remove dead code from program.getFastToc

7325157

The caching mechanism didn's have any effect. The inline function 'function' wasn't used anymore.

mtdcr force-pushed the master branch from 09ee334 to 7325157 Compare May 7, 2018 18:05

RecursiveForest approved these changes May 7, 2018

View reviewed changes

MerlijnWajer self-requested a review May 7, 2018 19:08

MerlijnWajer approved these changes May 7, 2018

View reviewed changes

JoeLametta changed the title ~~Remove lengthy read-toc operation~~ Remove dead code from program.getFastToc May 8, 2018

JoeLametta merged commit eb16904 into whipper-team:master May 8, 2018

mtdcr mentioned this pull request May 9, 2018

Add drive option to force using fast TOC #270

Closed

JoeLametta mentioned this pull request Dec 13, 2018

Refactor cdrdao toc/table functions into Task and provide progress output #345

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dead code from program.getFastToc #264

Remove dead code from program.getFastToc #264

mtdcr commented Apr 21, 2018

mtdcr commented Apr 21, 2018

MerlijnWajer commented Apr 22, 2018 •

edited

Loading

JoeLametta commented Apr 26, 2018 •

edited

Loading

MerlijnWajer commented Apr 26, 2018

mtdcr commented Apr 27, 2018

MerlijnWajer commented Apr 27, 2018

mtdcr commented Apr 27, 2018

mtdcr commented Apr 27, 2018

JoeLametta commented Apr 27, 2018 •

edited

Loading

RecursiveForest left a comment

RecursiveForest Apr 29, 2018

RecursiveForest Apr 29, 2018

MerlijnWajer commented Apr 29, 2018

MerlijnWajer commented Apr 29, 2018 •

edited

Loading

mtdcr commented May 7, 2018

RecursiveForest commented May 7, 2018

MerlijnWajer left a comment

JoeLametta commented May 8, 2018

Remove dead code from program.getFastToc #264

Remove dead code from program.getFastToc #264

Conversation

mtdcr commented Apr 21, 2018

mtdcr commented Apr 21, 2018

MerlijnWajer commented Apr 22, 2018 • edited Loading

JoeLametta commented Apr 26, 2018 • edited Loading

MerlijnWajer commented Apr 26, 2018

mtdcr commented Apr 27, 2018

MerlijnWajer commented Apr 27, 2018

mtdcr commented Apr 27, 2018

mtdcr commented Apr 27, 2018

JoeLametta commented Apr 27, 2018 • edited Loading

RecursiveForest left a comment

Choose a reason for hiding this comment

RecursiveForest Apr 29, 2018

Choose a reason for hiding this comment

RecursiveForest Apr 29, 2018

Choose a reason for hiding this comment

MerlijnWajer commented Apr 29, 2018

MerlijnWajer commented Apr 29, 2018 • edited Loading

mtdcr commented May 7, 2018

RecursiveForest commented May 7, 2018

MerlijnWajer left a comment

Choose a reason for hiding this comment

JoeLametta commented May 8, 2018

MerlijnWajer commented Apr 22, 2018 •

edited

Loading

JoeLametta commented Apr 26, 2018 •

edited

Loading

JoeLametta commented Apr 27, 2018 •

edited

Loading

MerlijnWajer commented Apr 29, 2018 •

edited

Loading