Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need help debugging/tweaking docker build #307

Closed
andrewlow opened this issue Oct 13, 2018 · 9 comments
Closed

Need help debugging/tweaking docker build #307

andrewlow opened this issue Oct 13, 2018 · 9 comments
Labels
Duplicate Duplicate of another issue Needed: more info A reply from issue author is required Support Questions that needs answering with no code changes needed or that only require a one time change Upstream Bug The issue is a result of an upstream bug

Comments

@andrewlow
Copy link

I can't get the docker build of the current github code here to work. I am able to get another build to work fine on the same hardware.

There is a version of whipper in dockerhub: https://hub.docker.com/r/dartagan/whipper/
This points at https://github.com/DArtagan/docker-whipper where you can see the Dockerfile

I'm testing using the image pulled from dockerhub.

$ docker pull dartagan/whipper
$ docker run --device /dev/cdrom -v /tmp/rips:/rips dartagan/whipper whipper offset find -o 667
Checking device /dev/cdrom
Trying read offset 667 ...
Offset of device is likely 667, confirming ...
                                             
Read offset of device is: 667.
Adding read offset to configuration file.

As you can see above, this works.

Looking at the whipper.conf file

$ cat ~/.config/whipper/whipper.conf 
[drive:HL-DT-ST%3ABDDVDRW%20CH12LS28%3A1.00]
vendor = HL-DT-ST
model = BDDVDRW CH12LS28
release = 1.00
defeats_cache = True
read_offset = 667

You can see my drive model is identified (I added the read_offset by hand at one point) - point here is that the drive model is in the Accurip DB http://www.accuraterip.com/driveoffsets.htm and is listed there as +667

I built the current whipper code into a docker container using the instructions in the README.md

$ docker build -t whipper/whipper .

I added it as an alias

alias whipper="docker run -ti --rm --device=/dev/cdrom \
    -e WHIPPER_DEBUG=debug -e WHIPPER_LOGFILE=whipper.log \
    -v ~/.config/whipper:/home/worker/.config/whipper \
    -v ${PWD}/output:/output \
    whipper/whipper"

then run it to replicate what I've done with the dockerhub build

$  whipper offset find -o 667
Checking device /dev/cdrom
Trying read offset 667 ...
WARNING:whipper.program.cdparanoia:file size 0 did not match expected size 37916636
WARNING:whipper.program.cdparanoia:non-integral amount of frames difference
WARNING: cannot rip with offset 667...    
No matching offset found.
Consider trying again with a different disc.

As you can see above, the current code does not work for me.

Same CD was used. Same machine, OS, hardware etc. Two different docker containers.

The dockerhub image pulled (dartagan/whipper) fails to properly rip the LAST track when I try to rip a CD. Thus it's not fully useful either. However, frustrating that the docker build from this github doesn't work for me.

Looking for suggestions on steps to dig in and figure this out.

@JoeLametta
Copy link
Collaborator

JoeLametta commented Oct 16, 2018

Hi, thanks for reporting this issue!

DArtagan/docker-whipper's Dockerfile uses an old whipper version (v0.5.1) and Xiph's cdparanoia.
The bug you're experiencing isn't related to the Dockerfile shipped in the whipper repository: it's caused by a whipper dependency. For more details see: libcdio/libcdio-paranoia#14 and #234. Xiph's cdparanoia doesn't seem to be affected by that bug but was replaced few months ago because it had another one (quite serious and it's unmaintained).

What happens if you add the detected offset to whipper's config file manually and rip this way?

Cheers,
Joe

@JoeLametta JoeLametta added Duplicate Duplicate of another issue Support Questions that needs answering with no code changes needed or that only require a one time change labels Oct 16, 2018
@andrewlow
Copy link
Author

$ cd /tmp
$ mkdir output
$ alias whipper="docker run -ti --rm --device=/dev/cdrom \
    -e WHIPPER_DEBUG=debug -e WHIPPER_LOGFILE=whipper.log \
    -v ~/.config/whipper:/home/worker/.config/whipper \
    -v ${PWD}/output:/output \
    whipper/whipper"

Above is my setup.

$ whipper cd rip
Using configured read offset 667
Checking device /dev/cdrom
Reading TOC...
CDDB disc id: ed115110
MusicBrainz disc id Yy_QkFETrIPsS8iGAhUwxKhBsdE-
MusicBrainz lookup URL https://musicbrainz.org/cdtoc/attach?toc=1+16+332686+150+19493+45799+63415+80052+104052+122052+149360+175538+191426+204826+222453+246729+266599+287446+317495&tracks=16&id=Yy_QkFETrIPsS8iGAhUwxKhBsdE-
Disc duration: 01:13:53.813, 16 audio tracks

Matching releases:

Artist  : The Crystal Method
Title   : Community Service
Duration: 01:14:14.000
URL     : https://musicbrainz.org/release/44009f4f-58f5-434a-9249-0d53a9cee46d
Release : 44009f4f-58f5-434a-9249-0d53a9cee46d
Type    : Compilation
Barcode : 617465112522
Cat no  : UL 1125-2

creating output directory ./compilation/The Crystal Method - Community Service
Ripping track 1 of 16: 01. Ils - No Soul (PMT remix).flac
Ripping track 1 of 16 (try 2): 01. Ils - No Soul (PMT remix).flac
Ripping track 1 of 16 (try 3): 01. Ils - No Soul (PMT remix).flac
Ripping track 1 of 16 (try 4): 01. Ils - No Soul (PMT remix).flac
Ripping track 1 of 16 (try 5): 01. Ils - No Soul (PMT remix).flac
track can't be ripped. Rip attempts number is equal to 'MAX_TRIES'

@andrewlow
Copy link
Author

whipper.log
debug log that was generated from previous comment run

@JoeLametta
Copy link
Collaborator

Until the bug gets fixed upstream you can probably manage to rip CDs with whipper in three ways:

  • using a (another) disc drive with a known offset value equal to less than 588
  • using 0 as offset (and correct it later)
  • using Xiph's cdparanoia instead of libcdio-paranoia...

@andrewlow
Copy link
Author

Is the libcdio-paranoia defect this one? libcdio/libcdio-paranoia#14

How does one use 0 as an offset and correct it later?

@JoeLametta
Copy link
Collaborator

Is the libcdio-paranoia defect this one? rocky/libcdio-paranoia#14

Yes.

How does one use 0 as an offset and correct it later?

You can use mktoc's offset correction (in your case the offset value for mktoc would be -667). Or CUETools's offset correction feature (with offset 667 - this software needs Mono/Wine).

@JoeLametta JoeLametta added Upstream Bug The issue is a result of an upstream bug Needed: more info A reply from issue author is required and removed *paranoia labels Nov 11, 2018
@JoeLametta
Copy link
Collaborator

No reply in 10 days: closed.

@andrewlow
Copy link
Author

Apologies for letting this one rot. Agree it is an upstream bug.

I tried naively patching the docker container with the Xiph's cdparanoia -- but got the same behaviour. I haven't tried messing with offset correction.

Closed is the correct action for this one. Thanks for all the work on this project.

@JoeLametta
Copy link
Collaborator

I tried naively patching the docker container with the Xiph's cdparanoia -- but got the same behaviour.

That's unexpected...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate Duplicate of another issue Needed: more info A reply from issue author is required Support Questions that needs answering with no code changes needed or that only require a one time change Upstream Bug The issue is a result of an upstream bug
Projects
None yet
Development

No branches or pull requests

2 participants