AccurateRip V2 support #187
Conversation
RecursiveForest
commented
Sep 5, 2017
RecursiveForest
commented
- output path no longer has fallbacks
- refactor accuraterip cache
- use requests to download accuraterip entries
- add tests for accuraterip functionality
- remove gobject support from accuraterip-checksum calculation
- default track template now includes extension
- begin to remove support for continuing rip
- begin to use print instead of sys.stdout.write() throughout
- output path no longer has fallbacks - refactor accuraterip cache - use requests to download accuraterip entries - add tests for accuraterip functionality - remove gobject support from accuraterip-checksum calculation - default track template now includes extension - begin to remove support for continuing rip - begin to use print instead of sys.stdout.write() throughout
|
I pressed 'return' by accident before I could add more text to the PR, so: I've consolidated a lot of spread-out pieces of code into one location. I'm trying to incrementally move away from the current method of organisation into the current by writing code in a more function style, organised by subsystem. This means moving some code that potentially ought to belong on an object eventually into modules as functions in order to make the existing objects easier to modify or delete. Some files will have a mix of styles during the refactor, but I think this will do well to serve as a mark of what areas need attention. I think we should adhere to the PEP8 79-character line limit. At the very least, it makes lining up code on my monitors into vertical panels extremely easy. ;) I also think that henceforth we should either require or strive for unit tests with any new or reworked functionality. |
|
There are still more tests to be written, and some of the ideas and organisation in this PR haven't gone to their logical conclusion (in particular the |
|
I should also note that this has a change which now actually requires pycdio as a full dependency. |
|
Good work. Lots of changes. Notes from IRC: 09:43 Wizzup> Why does it change the default track template? We only support flac. |
I didn't like hardcoding '.flac' to the end of the file. It just felt more right to include it in the track template, to make the behaviour more explicit and transparent for the user.
Correct, it just compares list references. I believe the only place it's used is comparing two variables potentially holding the same references, but I can make it a deep comparison if you want.
That was the point. It's not a direct "bolt on v2 support to the existing mess", but as stated above a step towards refactoring the program into something easier to work with overall. Some of the other changes, like removing getPath's disambiguation code, simplified some path handling logic in related functions regarding verification enough that I decided it was better to keep it in this branch than to try and factor it out. |
|
As an aside: I developed this changeset in a very experimental manner where I just started hacking away on my checkout of HEAD without trying to factor out different changes into different branches or tracking my progress along the way with individual commits in order to familiarise myself with some of the less common features of git. I don't think it made for a very readable PR (as Wizzup noted), and thus won't be developing like this again. |
|
Please don't take my comments as criticism. I'm happy with the PR and I think we should merge it after some testing. But splitting up the refactoring and actually adding the support for V2 might have made it a bit more readable. And I have to admin I only looked at the whole diff, not per commits, so maybe that paints a better picture. :) |
|
As for the eq, I think that if you actually mean the compare the values, then a deep comparison would make sense. The assumption that they point to the same list could break with any chance, and then there'll be extra unexpected breakage. |
|
I remember making the decision at the time that references were sufficient, but I'm not sure of that anymore. Criticism is fine, it's what review is for! There's no per-commit history for this, unfortunately. |
|
I experimented around in the REPL and in the tests ( |
|
Here are the results of the static analysis. I've tried to filter out the warnings which aren't relevant to this PR or consist of FPs but a few surely slipped through...
|
|
What is an FP? |
|
Good reminder, though, that I broke the accuraterip command; I knew I was forgetting something… the lack of useful tests is misleading. Anyways, almost all of those are definitely not relevant to the scope of this PR. It did find one leftover 'exit' from before I decided to raise an exception in that case though, so that's good. |
I meant false positive. |
RecursiveForest
force-pushed
the
rewrite-accuraterip
branch
from
5a856ba to
4126ec5
Compare
|
Understood. I believe I've addressed all the feedback relevant to this PR. |
Thanks, I'll field-test this PR later today and compute the updated coverage report.
|
NewsUpdated coverage reportField-test reportI've caught two tracebacks: the former using DAE Drive Features Database's test CD-R including HTOA, the latter using a standard pressed CD (without HTOA). CD with HTOA$ LANG=en_US.UTF-8 whipper cd rip -p -o 6 -W . --cdr
Checking device /dev/sr0
eject: CD-ROM tray close command failed: Input/output error
CDDB disc id: 0a00ea01
MusicBrainz disc id x5VvXYKDeFC.M0SGfeZXM6tlx8k-
MusicBrainz lookup URL https://musicbrainz.org/cdtoc/attach?toc=1+1+31833+14275&tracks=1&id=x5VvXYKDeFC.M0SGfeZXM6tlx8k-
Disc duration: 00:03:54.106, 1 audio tracks
Matching releases:
Artist : [unknown]
Title : DAE Drive Features Database
Duration: 00:03:54.106
URL : https://musicbrainz.org/release/401a9ca2-cb62-4d9f-8f5f-20aa4ccb7e17
Release : 401a9ca2-cb62-4d9f-8f5f-20aa4ccb7e17
Type : Album
creating output directory ./album/[unknown] - DAE Drive Features Database
found Hidden Track One Audio from frame 0 to 14124
Traceback (most recent call last):
File "/usr/local/bin/whipper", line 9, in <module>
load_entry_point('whipper==0.5.1', 'console_scripts', 'whipper')()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/main.py", line 32, in main
ret = cmd.do()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/basecommand.py", line 124, in do
return self.cmd.do()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/basecommand.py", line 124, in do
return self.cmd.do()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/cd.py", line 190, in do
self.doCommand()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/cd.py", line 474, in doCommand
_ripIfNotRipped(0)
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/cd.py", line 363, in _ripIfNotRipped
track_number=number)
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/common/program.py", line 249, in getPath
return os.path.join(outdir, template % v)
KeyError: u't'Pressed CDAll tracks have been ripped but it crashed after having generated the cue sheet (while generating the m3u file) so that the log file is missing. Stdout + Stderr: $ time LANG=en_US.UTF-8 whipper cd rip -o 6 -p -W .
Checking device /dev/sr0
eject: CD-ROM tray close command failed: Input/output error
CDDB disc id: 9409710a
MusicBrainz disc id 6RUIx_6VkCmJ2VlmKb.IaINLagY-
MusicBrainz lookup URL https://musicbrainz.org/cdtoc/attach?toc=1+10+181450+150+22252+56042+70572+84872+97275+114635+131212+148077+163817&tracks=10&id=6RUIx_6VkCmJ2VlmKb.IaINLagY-
Disc duration: 00:40:17.333, 10 audio tracks
Matching releases:
Artist : Lucio Battisti
Title : Vol. 4
Duration: 00:40:18.903
URL : https://musicbrainz.org/release/fd565c86-96a1-3ef6-9948-9f20d9aa5c7a
Release : fd565c86-96a1-3ef6-9948-9f20d9aa5c7a
Type : Album
Barcode : 8003614148832
Cat no : CDMRL 6484
creating output directory ./album/Lucio Battisti - Vol. 4
Ripping track 1 of 10: 01. Lucio Battisti - Le tre verità.flac
CRCs match for track 1
Peak level: 95.94%
Rip quality: 100.00%
Ripping track 2 of 10: 02. Lucio Battisti - Dio mio no.flac
CRCs match for track 2
Peak level: 100.00%
Rip quality: 100.00%
Ripping track 3 of 10: 03. Lucio Battisti - Adesso sì.flac
CRCs match for track 3
Peak level: 98.50%
Rip quality: 100.00%
Ripping track 4 of 10: 04. Lucio Battisti - La mia canzone per Maria.flac
CRCs match for track 4
Peak level: 100.00%
Rip quality: 100.00%
Ripping track 5 of 10: 05. Lucio Battisti - Luisa Rossi.flac
CRCs match for track 5
Peak level: 91.83%
Rip quality: 100.00%
Ripping track 6 of 10: 06. Lucio Battisti - Pensieri e parole.flac
CRCs match for track 6
Peak level: 87.98%
Rip quality: 100.00%
Ripping track 7 of 10: 07. Lucio Battisti - Mi ritorni in mente.flac
CRCs match for track 7
Peak level: 89.65%
Rip quality: 100.00%
Ripping track 8 of 10: 08. Lucio Battisti - Insieme a te sto bene.flac
CRCs match for track 8
Peak level: 90.98%
Rip quality: 100.00%
Ripping track 9 of 10: 09. Lucio Battisti - 29 settembre.flac
CRCs match for track 9
Peak level: 92.99%
Rip quality: 100.00%
Ripping track 10 of 10: 10. Lucio Battisti - Io vivrò (senza te).flac
CRCs match for track 10
Peak level: 90.15%
Rip quality: 100.00%
Traceback (most recent call last):
File "/usr/local/bin/whipper", line 9, in <module>
load_entry_point('whipper==0.5.1', 'console_scripts', 'whipper')()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/main.py", line 32, in main
ret = cmd.do()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/basecommand.py", line 124, in do
return self.cmd.do()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/basecommand.py", line 124, in do
return self.cmd.do()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/cd.py", line 190, in do
self.doCommand()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/cd.py", line 490, in doCommand
self.program.write_m3u(discName, htoapath)
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/common/program.py", line 618, in write_m3u
common.FRAMES_PER_SECOND))
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/common/program.py", line 607, in writeFile
f.write(u'#EXTINF:%d,%s\n' % (length, target_path))
UnicodeEncodeError: 'ascii' codec can't encode character u'\xe0' in position 45: ordinal not in range(128)
real 19m26.203s
user 0m40.508s
sys 0m6.824sCue sheet: M3U file: |
|
I ran stdout: stdout after running the same command on master codebase: |
whipper/command/offset.py
Outdated
| @@ -161,17 +146,18 @@ def match(archecksum, track, responses): | |||
|
|
|||
| # now try and rip all other tracks as well, except for the | |||
| # last one (to avoid readers that can't do overread | |||
| for track in range(2, (len(table.tracks) + 1) - 1): | |||
| # for track in range(2, (len(table.tracks) + 1) - 1): | |||
| for track in range(2, (len(table.tracks) + 1)): | |||
There was a problem hiding this comment.
this should be reverted back to for track in range(2, (len(table.tracks) + 1) - 1):
whipper/common/program.py
Outdated
| @@ -215,59 +215,38 @@ def getPath(self, outdir, template, mbdiscid, i, disambiguate=False): | |||
| v['x'] = 'flac' | |||
| v['X'] = v['x'].upper() | |||
| v['y'] = '0000' | |||
| if track_number: | |||
There was a problem hiding this comment.
Re the HTOA bug: when track_number == 0, this will evaluate to False. Thus line 222 is dead code. Maybe change to if track_number is not None: ?
|
Joe, can you let me know if the commit above fixes your unicode error? I really need some CDs to test with... |
|
@RecursiveForest - sorry for offtopic, but I wanted to say that I forgot about the CDs that I was going to upload. I have about 20 packed in my bag, and I am planning to make a static page on my server with various kind of CDs. Let's coordinate (and push me!) to get that done ASAP. I have some interesting ones for AR code. |
Yeah, that seems to be fixed (tested with the same CD). Pressed CD without HTOA$ time LANG=en_US.UTF-8 whipper cd rip -o 6 -p -W .
Checking device /dev/sr0
eject: CD-ROM tray close command failed: Input/output error
CDDB disc id: 9409710a
MusicBrainz disc id 6RUIx_6VkCmJ2VlmKb.IaINLagY-
MusicBrainz lookup URL https://musicbrainz.org/cdtoc/attach?toc=1+10+181450+150+22252+56042+70572+84872+97275+114635+131212+148077+163817&tracks=10&id=6RUIx_6VkCmJ2VlmKb.IaINLagY-
Disc duration: 00:40:17.333, 10 audio tracks
Matching releases:
Artist : Lucio Battisti
Title : Vol. 4
Duration: 00:40:18.903
URL : https://musicbrainz.org/release/fd565c86-96a1-3ef6-9948-9f20d9aa5c7a
Release : fd565c86-96a1-3ef6-9948-9f20d9aa5c7a
Type : Album
Barcode : 8003614148832
Cat no : CDMRL 6484
creating output directory ./album/Lucio Battisti - Vol. 4
Ripping track 1 of 10: 01. Lucio Battisti - Le tre verità.flac
CRCs match for track 1
Peak level: 95.94%
Rip quality: 100.00%
Ripping track 2 of 10: 02. Lucio Battisti - Dio mio no.flac
CRCs match for track 2
Peak level: 100.00%
Rip quality: 100.00%
Ripping track 3 of 10: 03. Lucio Battisti - Adesso sì.flac
CRCs match for track 3
Peak level: 98.50%
Rip quality: 100.00%
Ripping track 4 of 10: 04. Lucio Battisti - La mia canzone per Maria.flac
CRCs match for track 4
Peak level: 100.00%
Rip quality: 100.00%
Ripping track 5 of 10: 05. Lucio Battisti - Luisa Rossi.flac
CRCs match for track 5
Peak level: 91.83%
Rip quality: 100.00%
Ripping track 6 of 10: 06. Lucio Battisti - Pensieri e parole.flac
CRCs match for track 6
Peak level: 87.98%
Rip quality: 100.00%
Ripping track 7 of 10: 07. Lucio Battisti - Mi ritorni in mente.flac
CRCs match for track 7
Peak level: 89.65%
Rip quality: 100.00%
Ripping track 8 of 10: 08. Lucio Battisti - Insieme a te sto bene.flac
CRCs match for track 8
Peak level: 90.98%
Rip quality: 100.00%
Ripping track 9 of 10: 09. Lucio Battisti - 29 settembre.flac
CRCs match for track 9
Peak level: 92.99%
Rip quality: 100.00%
Ripping track 10 of 10: 10. Lucio Battisti - Io vivrò (senza te).flac
CRCs match for track 10
Peak level: 90.15%
Rip quality: 100.00%
rip verified as accurate
track 1: rip accurate (max confidence 3) v1 [0ac1a3e9], v2 [d3e7c200], DB [d3e7c200]
track 2: rip accurate (max confidence 3) v1 [dc955a0e], v2 [13c63f55], DB [13c63f55]
track 3: rip accurate (max confidence 3) v1 [004ea2b2], v2 [69d82eef], DB [69d82eef]
track 4: rip accurate (max confidence 3) v1 [ad9f2dc0], v2 [66a0f024], DB [66a0f024]
track 5: rip accurate (max confidence 3) v1 [3c944309], v2 [cf72d808], DB [cf72d808]
track 6: rip accurate (max confidence 3) v1 [92634ec4], v2 [b97cf148], DB [b97cf148]
track 7: rip accurate (max confidence 3) v1 [54415e90], v2 [89fda814], DB [89fda814]
track 8: rip accurate (max confidence 3) v1 [3d545587], v2 [68f5c941], DB [68f5c941]
track 9: rip accurate (max confidence 3) v1 [08b1d558], v2 [e7d98b7d], DB [e7d98b7d]
track 10: rip accurate (max confidence 3) v1 [be671022], v2 [a25dab58], DB [a25dab58]
real 19m54.714s
user 0m49.680s
sys 0m7.084sHere's the M3U file (file classifies it as "M3U playlist, UTF-8 Unicode text"): Generated logfile: CD with HTOAIf I try to rip the CD which includes the HTOA, I get a new traceback (IndexError): $ time LANG=en_US.UTF-8 whipper cd rip -p -o 6 -W . --cdr
Checking device /dev/sr0
eject: CD-ROM tray close command failed: Input/output error
CDDB disc id: 0a00ea01
MusicBrainz disc id x5VvXYKDeFC.M0SGfeZXM6tlx8k-
MusicBrainz lookup URL https://musicbrainz.org/cdtoc/attach?toc=1+1+31833+14275&tracks=1&id=x5VvXYKDeFC.M0SGfeZXM6tlx8k-
Disc duration: 00:03:54.106, 1 audio tracks
Matching releases:
Artist : [unknown]
Title : DAE Drive Features Database
Duration: 00:03:54.106
URL : https://musicbrainz.org/release/401a9ca2-cb62-4d9f-8f5f-20aa4ccb7e17
Release : 401a9ca2-cb62-4d9f-8f5f-20aa4ccb7e17
Type : Album
creating output directory ./album/[unknown] - DAE Drive Features Database
found Hidden Track One Audio from frame 0 to 14124
Ripping track 0 of 1: 00. [unknown] - Hidden Track One Audio.flac
CRCs match for track 0
Peak level: 100.00%
Rip quality: 87.46%
Ripping track 1 of 1: 01. [unknown] - Music (In Pregap and Track).flac
CRCs match for track 1
Peak level: 100.00%
Rip quality: 100.00%
Traceback (most recent call last):
File "/usr/local/bin/whipper", line 9, in <module>
load_entry_point('whipper==0.5.1', 'console_scripts', 'whipper')()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/main.py", line 32, in main
ret = cmd.do()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/basecommand.py", line 124, in do
return self.cmd.do()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/basecommand.py", line 124, in do
return self.cmd.do()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/cd.py", line 190, in do
self.doCommand()
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/command/cd.py", line 489, in doCommand
self.program.write_m3u(discName, htoapath)
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/common/program.py", line 619, in write_m3u
(self.result.table.getTrackLength(i + 1) /
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/image/table.py", line 238, in getTrackLength
return self.getTrackEnd(number) - self.getTrackStart(number) + 1
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/image/table.py", line 203, in getTrackStart
track = self.tracks[number - 1]
IndexError: list index out of range
real 7m1.600s
user 0m14.220s
sys 0m1.684s
You can get the test one I'm using (which includes the HTOA) from here. 😉 |
|
Okay, glad the UTF-8 problem is fixed. Now that I can test real HTOA (I only had a false positive disc) locally I'll make sure I properly fix the HTOA bug. I definitely made the assumption that we don't support other hidden tracks (which I thought we didn't), so I'm interested to see where the above breaks! The command/image errors are indeed real problems which I should have under control today as well. I'm so glad we're testing this stuff instead of letting me break everything with each commit! |
- accurip.py: raise exception if accuraterip db entry not found - program.py: verify image with only one table, remove redundant check
RecursiveForest
force-pushed
the
rewrite-accuraterip
branch
from
048f4bf to
f897acf
Compare
RecursiveForest
force-pushed
the
rewrite-accuraterip
branch
from
f897acf to
311084c
Compare
|
I've updated whipper from your branch and tested it against the previous CDs and it worked as expected. Then I've ran the static analysis check again:
As you've slightly touched the code related to pycdio, I'm including the two following warnings too (the first one includes a TODO):
Looking forward to merge this PR. 😜 |
|
I don't believe the RipResult warning in common/cache or the two cdio warnings are within scope of this PR. I have a separate PR to fully require cdio/pycdio ready to go once this is merged into master, and plan on addressing the caching situation eventually. Next priority is the cdrdao & config regressions. |
|
@RecursiveForest All right. If everything is done for this PR, I'm going to merge it today: just let me know. EDIT: Maybe you should add those two things ("output path no longer has fallbacks", "begin to remove support for continuing rip") here too. Thanks |
|
That's a good question and a good idea. I think this PR is ready to merge though, especially if resuming a failed rip is already broken. |
Understood. |
