bleskodev / rubyripper

Fork of the original rubyripper from code.google.com/p/rubyripper + some bugs fixes
126 stars 21 forks source link

Rubyripper may fail to identify incorrect rips #528

Open GoogleCodeExporter opened 8 years ago

GoogleCodeExporter commented 8 years ago
While running through my library and comparing output with AccurateRip results 
(Using flac_arcrc here: 
http://www.hydrogenaudio.org/forums/index.php?showtopic=61574) I have noticed 
that, on at least one file, AccurateRip reported an incorrect rip despite 
Rubyripper succeeding to rip.  After reripping with commit 
8c38d19fbdbed765c5b1cafb1efcd48f0a76aac0 and obtaining the same result.  I 
compared the rip with a fresh rip of the same CD using EAC (which reported no 
errors and a good rip according to AccurateRip), which revealed that the 
results did, in fact, differ on the relevant tracks.

Closer examination reveals that cdparanoia encounters an error in "stage 1" 
that is corrected in "stage 2" followed by a region where jitter correction was 
"required" (due to the corrections generating a temporary read offset of 39 
samples [156 bytes]).  This results in a file that is generated consistently by 
cdparanoia despite differing from the "perfect" EAC rip.

This raises concerns about cdparanoia's error correction, as it seems to 
actually retain errors despite ostensibly creating a standard rip.  The most 
appropriate course of action would seem to be that additional logging of the 
cdparanoia output is required so as to detect (and reject) any rips which have 
been "corrected" by cdparanoia as uncorrectable (as cdparanoia will rip these 
discs consistently, yet do so incorrectly)

I have attached the relevant range rip logs from Rubyripper (commit 
8c38d19fbdbed765c5b1cafb1efcd48f0a76aac0) and EAC, as well as the "log" from 
cdparanoia and the results of flac_arcrc on the (split) rip from Rubyripper.

The command run by Rubyripper (and run by myself from the command-line to 
obtain cdparanoia output) was:

cdparanoia "[.0]-[.168626]" -d /dev/cd0 
"/host/localhost/rubyripper/temp_cd0/image_1.wav"

My CLI cdparanoia rip was post-processed to correct for the +667 drive offset, 
resulting in a file identical to that generated by rubyripper.  Adding "-O 667" 
to correct for the offset within cdparanoia (instead of correcting during 
post-processing as rubyripper does) and accounting for cdparanoia's issues with 
the --sample-offset option (i.e. wrapping at +588 and accounting for errors 
reading beyond the end of the disc) did not appear to affect the location or 
nature of the error recorded by cdparanoia.

Original issue reported on code.google.com by comradec...@gmail.com on 17 Jun 2012 at 3:13

Attachments:

GoogleCodeExporter commented 8 years ago
Corrected the cdparanoia log (which was empty).

Original comment by comradec...@gmail.com on 17 Jun 2012 at 3:14

Attachments:

GoogleCodeExporter commented 8 years ago
I believe that there are two pieces for a proper resolution to this:

1. Integration of CTDB will provide not only for accurate rip checking, but 
also provides for repairing rips.
2. Another fix would require directly accessing libparanoia and doing ripping 
directly using the C-interface to get detailed information about the number of 
frames with errors.  This would require a dependency on ruby-ffi, however.

Original comment by comradec...@gmail.com on 23 Aug 2012 at 1:52

GoogleCodeExporter commented 8 years ago
The following patch addresses point 2 by using the "-e" flag on cdparanoia and 
then reading the output and counting instances of notable errors (like that 
used in XLD logs).  If rubyripper finds two matching rips, but cdparanoia 
reported errors with the rips, the details from cdparanoia will be included in 
the log, such as in the (also attached) log file.

I've yet to commit the patch to master, pending review of the code and properly 
merging the patch with HEAD (the patch is relative to commit 8c38d19)

Original comment by comradec...@gmail.com on 25 Aug 2012 at 9:57

Attachments:

GoogleCodeExporter commented 8 years ago
Oops.  Fixed empty patch.

Original comment by comradec...@gmail.com on 25 Aug 2012 at 9:59

Attachments:

GoogleCodeExporter commented 8 years ago

Original comment by boukewou...@gmail.com on 9 Jan 2013 at 10:45