From 3f4291bb18668d9ca2ec9319e2ab44940cf2a3f5 Mon Sep 17 00:00:00 2001 From: Thomas Vander Stichele Date: Sun, 24 Feb 2013 11:37:13 +0100 Subject: [PATCH] Handle off-by-1 errors in cdparanoia progress parsing Fixes the exception I got on ripping The Strokes - Someday --- HACKING | 3 + morituri/common/common.py | 2 +- morituri/program/cdparanoia.py | 28 +++--- morituri/test/Makefile.am | 2 + morituri/test/cdparanoia.progress.strokes | 111 ++++++++++++++++++++++ morituri/test/strokes-someday.toc | 12 +++ morituri/test/test_image_toc.py | 18 ++++ morituri/test/test_program_cdparanoia.py | 18 +++- 8 files changed, 181 insertions(+), 13 deletions(-) create mode 100644 morituri/test/cdparanoia.progress.strokes create mode 100644 morituri/test/strokes-someday.toc diff --git a/HACKING b/HACKING index 44723c3..cc25424 100644 --- a/HACKING +++ b/HACKING @@ -41,3 +41,6 @@ CDROMS PLEXTOR CD-R PX-W8432T Read offset of device is: 355. +Test discs +---------- +The Strokes - Someday (promo): has 1 frame silence marked as SILENCE diff --git a/morituri/common/common.py b/morituri/common/common.py index a647ba0..0d91ed2 100644 --- a/morituri/common/common.py +++ b/morituri/common/common.py @@ -30,7 +30,7 @@ from morituri.extern.log import log FRAMES_PER_SECOND = 75 -SAMPLES_PER_FRAME = 588 +SAMPLES_PER_FRAME = 588 # a sample is 2 16-bit values, left and right channel WORDS_PER_FRAME = SAMPLES_PER_FRAME * 2 BYTES_PER_FRAME = SAMPLES_PER_FRAME * 4 diff --git a/morituri/program/cdparanoia.py b/morituri/program/cdparanoia.py index 406d6d9..e8f6c59 100644 --- a/morituri/program/cdparanoia.py +++ b/morituri/program/cdparanoia.py @@ -64,19 +64,21 @@ class ChecksumException(Exception): pass +# example: +# ##: 0 [read] @ 24696 _PROGRESS_RE = re.compile(r""" - ^\#\#: (?P.+)\s # function code - \[(?P.*)\]\s@\s # function name - (?P\d+) # offset + ^\#\#: (?P.+)\s # function code + \[(?P.*)\]\s@\s # [function name] @ + (?P\d+) # offset in words (2-byte one channel value) """, re.VERBOSE) _ERROR_RE = re.compile("^scsi_read error:") # from reading cdparanoia source code, it looks like offset is reported in -# number of single-channel samples, ie. 2 bytes per unit, and absolute +# number of single-channel samples, ie. 2 bytes (word) per unit, and absolute -class ProgressParser(object): +class ProgressParser(log.Loggable): read = 0 # last [read] frame wrote = 0 # last [wrote] frame errors = 0 # count of number of scsi errors @@ -128,13 +130,15 @@ class ProgressParser(object): # set nframes if not yet set if self._nframes is None and self.read != 0: self._nframes = frameOffset - self.read + self.debug('set nframes to %r', self._nframes) # set firstFrames if not yet set if self._firstFrames is None: self._firstFrames = frameOffset - self.start + self.debug('set firstFrames to %r', self._firstFrames) markStart = None - markEnd = None + markEnd = None # the next unread frame (half-inclusive) # verify it either read nframes more or went back for verify if frameOffset > self.read: @@ -165,10 +169,11 @@ class ProgressParser(object): # cdparanoia reads quite a bit beyond the current track before it # goes back to verify; don't count those - if markEnd > self.stop: - markEnd = self.stop - if markStart > self.stop: - markStart = self.stop + # markStart, markEnd of 0, 21 with stop 0 should give 1 read + if markEnd > self.stop + 1: + markEnd = self.stop + 1 + if markStart > self.stop + 1: + markStart = self.stop + 1 self.reads += markEnd - markStart @@ -185,8 +190,9 @@ class ProgressParser(object): Each frame gets read twice. More than two reads for a frame reduce track quality. """ - frames = self.stop - self.start + 1 + frames = self.stop - self.start + 1 # + 1 since stop is inclusive reads = self.reads + self.debug('getTrackQuality: frames %d, reads %d' % (frames, reads)) # don't go over a 100%; we know cdparanoia reads each frame at least # twice diff --git a/morituri/test/Makefile.am b/morituri/test/Makefile.am index e9f0751..298ada2 100644 --- a/morituri/test/Makefile.am +++ b/morituri/test/Makefile.am @@ -44,8 +44,10 @@ EXTRA_DIST = \ track-single.cue \ cdparanoia.progress \ cdparanoia.progress.error \ + cdparanoia.progress.strokes \ cdrdao.readtoc.progress \ silentalarm.result.pickle \ + strokes-someday.toc \ totbl.fast.toc \ track.flac \ cache/result/fe105a11.pickle \ diff --git a/morituri/test/cdparanoia.progress.strokes b/morituri/test/cdparanoia.progress.strokes new file mode 100644 index 0000000..3369ab5 --- /dev/null +++ b/morituri/test/cdparanoia.progress.strokes @@ -0,0 +1,111 @@ +Sending all callbacks to stderr for wrapper script +cdparanoia III release 10.2 (September 11, 2008) + +Ripping from sector 0 (track 0 [0:00.00]) + to sector 0 (track 0 [0:00.00]) + +outputting to cdda.wav + +##: 0 [read] @ 24696 +##: 0 [read] @ 56448 +##: 0 [read] @ 88200 +##: 0 [read] @ 119952 +##: 0 [read] @ 151704 +##: 0 [read] @ 183456 +##: 0 [read] @ 215208 +##: 0 [read] @ 246960 +##: 0 [read] @ 278712 +##: 0 [read] @ 310464 +##: 0 [read] @ 342216 +##: 0 [read] @ 373968 +##: 0 [read] @ 405720 +##: 0 [read] @ 437472 +##: 0 [read] @ 469224 +##: 0 [read] @ 500976 +##: 0 [read] @ 532728 +##: 0 [read] @ 564480 +##: 0 [read] @ 596232 +##: 0 [read] @ 627984 +##: 0 [read] @ 659736 +##: 0 [read] @ 691488 +##: 0 [read] @ 723240 +##: 0 [read] @ 754992 +##: 0 [read] @ 786744 +##: 0 [read] @ 818496 +##: 0 [read] @ 850248 +##: 0 [read] @ 882000 +##: 0 [read] @ 913752 +##: 0 [read] @ 945504 +##: 0 [read] @ 977256 +##: 0 [read] @ 1009008 +##: 0 [read] @ 1040760 +##: 0 [read] @ 1072512 +##: 0 [read] @ 1104264 +##: 0 [read] @ 1136016 +##: 0 [read] @ 1167768 +##: 0 [read] @ 1199520 +##: 0 [read] @ 1231272 +##: 0 [read] @ 1263024 +##: 0 [read] @ 1294776 +##: 0 [read] @ 1326528 +##: 0 [read] @ 1358280 +##: 0 [read] @ 1390032 +##: 0 [read] @ 1410024 +##: 0 [read] @ 23520 +##: 0 [read] @ 55272 +##: 0 [read] @ 87024 +##: 0 [read] @ 118776 +##: 0 [read] @ 150528 +##: 0 [read] @ 182280 +##: 0 [read] @ 214032 +##: 0 [read] @ 245784 +##: 0 [read] @ 277536 +##: 0 [read] @ 309288 +##: 0 [read] @ 341040 +##: 0 [read] @ 372792 +##: 0 [read] @ 404544 +##: 0 [read] @ 436296 +##: 0 [read] @ 468048 +##: 0 [read] @ 499800 +##: 0 [read] @ 531552 +##: 0 [read] @ 563304 +##: 0 [read] @ 595056 +##: 0 [read] @ 626808 +##: 0 [read] @ 658560 +##: 0 [read] @ 690312 +##: 0 [read] @ 722064 +##: 0 [read] @ 753816 +##: 0 [read] @ 785568 +##: 0 [read] @ 817320 +##: 0 [read] @ 849072 +##: 0 [read] @ 880824 +##: 0 [read] @ 912576 +##: 0 [read] @ 944328 +##: 0 [read] @ 976080 +##: 0 [read] @ 1007832 +##: 0 [read] @ 1039584 +##: 0 [read] @ 1071336 +##: 0 [read] @ 1103088 +##: 0 [read] @ 1134840 +##: 0 [read] @ 1166592 +##: 0 [read] @ 1198344 +##: 0 [read] @ 1230096 +##: 0 [read] @ 1261848 +##: 0 [read] @ 1293600 +##: 0 [read] @ 1325352 +##: 0 [read] @ 1357104 +##: 0 [read] @ 1388856 +##: 0 [read] @ 1410024 +##: 1 [verify] @ 0 +##: 3 [correction] @ 1005459 +##: 3 [correction] @ 1005480 +##: 1 [verify] @ 1005480 +##: 1 [verify] @ 1005480 +##: -2 [wrote] @ 1175 +##: -2 [wrote] @ 1176 +##: -1 [finished] @ 1175 + + +Done. + + diff --git a/morituri/test/strokes-someday.toc b/morituri/test/strokes-someday.toc new file mode 100644 index 0000000..bafd8e0 --- /dev/null +++ b/morituri/test/strokes-someday.toc @@ -0,0 +1,12 @@ +CD_DA + + +// Track 1 +TRACK AUDIO +COPY +NO PRE_EMPHASIS +TWO_CHANNEL_AUDIO +SILENCE 00:00:01 +FILE "data.wav" 0 03:06:59 +START 00:00:01 + diff --git a/morituri/test/test_image_toc.py b/morituri/test/test_image_toc.py index 1955d03..0175bed 100644 --- a/morituri/test/test_image_toc.py +++ b/morituri/test/test_image_toc.py @@ -318,3 +318,21 @@ class TOTBLTestCase(common.TestCase): def testCDDBId(self): self.toc.table.absolutize() self.assertEquals(self.toc.table.getCDDBDiscId(), '810b7b0b') + + +# The Strokes - Someday has a 1 frame SILENCE marked as such in toc + + +class StrokesTestCase(common.TestCase): + + def setUp(self): + self.path = os.path.join(os.path.dirname(__file__), + u'strokes-someday.toc') + self.toc = toc.TocFile(self.path) + self.toc.parse() + self.assertEquals(len(self.toc.table.tracks), 1) + + def testIndexes(self): + t = self.toc.table.tracks[0] + self.assertEquals(t.getIndex(0).relative, 0) + self.assertEquals(t.getIndex(1).relative, 1) diff --git a/morituri/test/test_program_cdparanoia.py b/morituri/test/test_program_cdparanoia.py index 58e0bbc..397073d 100644 --- a/morituri/test/test_program_cdparanoia.py +++ b/morituri/test/test_program_cdparanoia.py @@ -25,7 +25,23 @@ class ParseTestCase(common.TestCase): self._parser.parse(line) q = '%.01f %%' % (self._parser.getTrackQuality() * 100.0, ) - self.assertEquals(q, '99.7 %') + self.assertEquals(q, '99.6 %') + +class Parse1FrameTestCase(common.TestCase): + + def setUp(self): + path = os.path.join(os.path.dirname(__file__), + 'cdparanoia.progress.strokes') + self._parser = cdparanoia.ProgressParser(start=0, stop=0) + + self._handle = open(path) + + def testParse(self): + for line in self._handle.readlines(): + self._parser.parse(line) + + q = '%.01f %%' % (self._parser.getTrackQuality() * 100.0, ) + self.assertEquals(q, '100.0 %') class ErrorTestCase(common.TestCase):