Incorrectly fetches track length with certain offsets

ferk6a commented 6 years ago

I've seen a number of people running into the same issue when using whopper (https://github.com/JoeLametta/whipper/issues/234), and one user going as far as just removing errors checks in the source code to force cd-paranoia to continue working (https://github.com/JoeLametta/whipper/issues/234#issuecomment-393637392).

My hardware is HL-DT-ST GH22NS50, and in the AccurateRip database, it is expected that its read offset is +667. However, when using that number with cd-paranoia, it stops saying that the time/sector goes beyond end of my specified track:

$ cd-paranoia --stderr-progress --sample-offset=667 --force-cdrom-device /dev/sr0 1[00:00:00.00]-1[00:03:13.44] /tmp/tmpoEuyVd.track01.offset667.whipper.wav
Sending all callback output to stderr for wrapper script
cdparanoia III release 10.2 libcdio 2.0.0 x86_64-pc-linux-gnu
(C) 2001 Monty <monty@xiph.org> and Xiphophorus
(C) 2004, 2005, 2008 Rocky Bernstein <rocky@gnu.org>
(C) 2014 Robert Kausch <robert.kausch@freac.org>

Report bugs to bug-libcdio@gnu.org

Time/sector offset goes beyond end of specified track.

However, Xiph's cdparanoia works ok:

$ cdparanoia --stderr-progress --sample-offset=667 --force-cdrom-device /dev/sr0 1[00:00:00.00]-1[00:03:13.44] /tmp/tmpoEuyVd.track01.offset667.whipper.wav
Sending all callbacks to stderr for wrapper script
cdparanoia III release 10.2 (September 11, 2008)

Ripping from sector       1 (track  1 [0:00.00])
          to sector   14520 (track  1 [3:13.44])

outputting to /tmp/tmpoEuyVd.track01.offset667.whipper.wav

##: 0 [read] @ 25872
 (== PROGRESS == [>                             | ...... 00 ] == :^D . ==)   ##: 0 [read] @ 57624
 (== PROGRESS == [>                             | ...... 00 ] == :^D o ==)   ##: 0 [read] @ 89376
 (== PROGRESS == [>                             | ...... 00 ] == :^D 0 ==)   ##: 0 [read] @ 121128
 (== PROGRESS == [>                             | ...... 00 ] == :^D O ==)   ##: 0 [read] @ 152880
 (== PROGRESS == [>                             | ...... 00 ] == :^D 0 ==)   ##: 0 [read] @ 184632
##: 0 [read] @ 216384
 (== PROGRESS == [>                             | ...... 00 ] == :^D o ==)   ##: 0 [read] @ 248136
 (== PROGRESS == [>                             | ...... 00 ] == :^D . ==)   ##: 0 [read] @ 279888
 (== PROGRESS == [>                             | ...... 00 ] == :^D   ==)   ##: 0 [read] @ 311640
 (== PROGRESS == [>                             | ...... 00 ] == :^D . ==)   ##: 0 [read] @ 343392
##: 0 [read] @ 375144
##: 0 [read] @ 406896
...

And following the advice to leave out the time argument to cd-paranoia to guess, I end up with a very weird time signature indeed (but the sector number does match and the track is correctly ripped):

$ cd-paranoia --stderr-progress --sample-offset=667 --force-cdrom-device /dev/sr0 1 /tmp/tmpoEuyVd.track01.offset667.whipper.wav
Sending all callback output to stderr for wrapper script
cdparanoia III release 10.2 libcdio 2.0.0 x86_64-pc-linux-gnu
(C) 2001 Monty <monty@xiph.org> and Xiphophorus
(C) 2004, 2005, 2008 Rocky Bernstein <rocky@gnu.org>
(C) 2014 Robert Kausch <robert.kausch@freac.org>

Report bugs to bug-libcdio@gnu.org

Ripping from sector       1 (track  1 [0:00.00])
          to sector   14520 (track  2 [0:00.-1])

outputting to /tmp/tmpoEuyVd.track01.offset667.whipper.wav

##: 0 [read] @ 21168
 (== PROGRESS == [>                             | ...... 00 ] == :^D . ==)   ##: 0 [read] @ 50568
 (== PROGRESS == [>                             | ...... 00 ] == :^D o ==)   ##: 0 [read] @ 79968
 (== PROGRESS == [>                             | ...... 00 ] == :^D 0 ==)   ##: 0 [read] @ 109368
##: 0 [read] @ 138768
 (== PROGRESS == [>                             | ...... 00 ] == :^D O ==)   ##: 0 [read] @ 168168
 (== PROGRESS == [>                             | ...... 00 ] == :^D 0 ==)   ##: 0 [read] @ 197568
 (== PROGRESS == [>                             | ...... 00 ] == :^D o ==)   ##: 0 [read] @ 226968
 (== PROGRESS == [>                             | ...... 00 ] == :^D . ==)   ##: 0 [read] @ 256368
...

rocky commented 8 months ago

Looks great. It would be great to get confirmation from as many of the below as possible.

[ ] @fenugrec
[ ] @branchmispredictor
[ ] @dangpzanco
[ ] @francoisjacques
[ ] @JoeLametta
[ ] @aereaux
[ ] @45054 (is that bounty still claimable by @buddyabaddon ?)
[ ] @seisure

Thanks.

buddyabaddon commented 8 months ago

Thanks for helping with another validation @mdosch.

Looks like things are working for drives with the following offsets:

+667
+594

If anyone has a drive with an offset less than 1 sector (<+588) or greater than 2 sectors (>+1176), I'd love to hear your results.

Also (they seem to be much rarer) if anyone has a drive with a negative offset, that would be interesting to see as well.

aereaux commented 8 months ago

I'm not sure what I needed to do to trigger bad behavior previously but it seems to work through all the listed tests on a disc in a drive with offset +702. I did try going through the offset find process but it seemed to get stuck on trying a negative offset. Not sure if that's because of my drive or something else.

buddyabaddon commented 8 months ago

Thanks for testing @aereaux. I'm assuming you are using libcdio-paranoia via Whipper here...

As you have a drive with >1 sector sample offset, you just need to attempt to rip a disc with your +702 read_offset set in whipper.conf (or specified via the cmdline via --offset +702) to trigger bad behavior.

I suspect that your offset find attempt got stuck on a negative offset because your drive doesn't support reading into the lead-in. Pre-existing libcdio-paranoia logic (and even after my change for that matter), doesn't apply the same empty padding approach as reading into the lead-out when --force-overread is not specified.

To find your drive sample offset, I believe Whipper simply runs through multiple extraction attempts with different offsets and compares the results with the AccurateRip database. If there's a match, it attempts a few more tracks with the current offset and if they are all accurate, it considers your offset discovered.

whipper offset find --help shows the default set of offsets it attempts... did your try hang on offset -472 by chance? I see it in the list before your +702 offset.

Can you try via whipper find offset --offsets 702? You can set the environment variable WHIPPER_DEBUG=DEBUG to get extra debug information too.

In summary, libcdio-paranoia shouldn't attempt to actually read into the disc lead-in or lead-out unless --force-overread is specified. Right now that is not the case for lead-in when a negative offset is specified. This should be treated as a separate bug and I did not attempt to address this.

aereaux commented 8 months ago

Yeah, it was -472 where it hanged and doing whipper offset find with an explicitly +702 offset worked fine. I think I had this problem originally too, so I don't think it's related to this change.

In short my tests seemed all good for this. Thanks for doing the work on it!

fenugrec commented 7 months ago

Awesome, seems to work perfectly - tested on drive HL-DT-ST DVDRAM GSA-H62N .

whipper offset find correctly reports offset of 667 (we'll have to do something about silencing whipper's "offset > 587" warning now !
whipper cd rip success with confidence 10

@buddyabaddon very well done.

rocky commented 7 months ago

Ok - thanks everyone for the information! I didn't want to belabor this, but I did want to get some sense of what is up.

Again @buddyabaddon - thanks. Let's merge and if there is more you can iterate.

libcdio / libcdio-paranoia

Incorrectly fetches track length with certain offsets #14