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

Julie Roberts bug #74

Closed
RecursiveForest opened this issue Nov 11, 2016 · 7 comments
Closed

Julie Roberts bug #74

RecursiveForest opened this issue Nov 11, 2016 · 7 comments
Labels
Bug Generic bug: can be used together with more specific labels Upstream Bug The issue is a result of an upstream bug
Milestone

Comments

@RecursiveForest
Copy link
Contributor

RecursiveForest commented Nov 11, 2016

I will edit this post soon to explain what I mean. I think this is critical enough to require fixing for 1.0, especially if the goal is 1.0 = what.cd use.

Edit: https://savannah.gnu.org/bugs/?49831

@RecursiveForest RecursiveForest added this to the 1.0 milestone Nov 11, 2016
@JoeLametta JoeLametta added the Bug Generic bug: can be used together with more specific labels label Nov 13, 2016
@zacklb
Copy link

zacklb commented Dec 10, 2016

I found that cdparanoia's -Y option rips the 5th Roberts track properly. The -Y option turns off a flag called PARANOIA_MODE_VERIFY which lead me to the function i_stage1 (in paranoia.c). When I recompiled cdparanoia with this function commented out, the track rips correctly (at an otherwise full paranoia mode). So the issue has something to do with this stage 1 search to verify samples (I have no idea what this is).

Here's the output for the stage1 and 2 searches, one with the -Y flag and the other at full paranoia mode, on a drive with +6 read samples:
cdparanoia -Y -O 6 5 /dev/null
cdparanoia -O 6 5 /dev/null

I can't be much more help than this unfortunately. Maybe someone could send the Roberts phenomenon along to the Debian cdparanoia maintainers? Here's one: TANIGUCHI Takaki takaki@debian.org

@MerlijnWajer
Copy link
Collaborator

Maybe we can set some breakpoints and possibly step through the (whole?) ripping process, and try to see if we can spot said problem.

@kevmitch
Copy link

So what is this bug actually? cdparanoia was producing an incorrect rip?

@45054
Copy link

45054 commented Dec 11, 2016

Here you can find the cdparanoia bug report: http://lists.xiph.org/pipermail/paranoia-dev/2016-August/000279.html

Another old bug report looks a lot like this one: http://thomas.apestaart.org/morituri/trac/ticket/27

@RecursiveForest
Copy link
Contributor Author

RecursiveForest commented Dec 12, 2016

A user on the PTH forums reported setting MIN_WORDS_SEARCH to 128 from 64 was sufficient to fix the bug. If people could test this patch to confirm that it works for them, that would be swell. We especially want to make sure it rips other albums successfully still.

However, it has occurred to me that this bug is workaroundable by more than just the AccurateRip report: cdparanoia, at least on my drive, reports "!" where the error occurs in the Julie Roberts track. This means that if we correctly parse the cdparanoia output, we should be able to report an error when cdparanoia does and abort the rip.

I have edited the above in light of new information, including the patches the PTH user has contributed: https://savannah.gnu.org/bugs/?49831

@MerlijnWajer
Copy link
Collaborator

Good to see progress!

A few things:

  1. Let's please run this patch past cdparanoia developers. I can poke their mailing list. It could be a rather large change with other implications. (And I wonder a bit if it is the correct fix)
  2. I ... I have code to parse all different cdparanoia errors that I applies to morituri/program/cdparanoia.py - it's part of the patches I want to upstream. (I use it to report on all of this, but also to implement an early "give up?" oracle to bail early when we know as CD won't rip well. (See some code here, not ready to be merged yet - http://sprunge.us/TEUF && http://sprunge.us/BJij)

@JoeLametta
Copy link
Collaborator

🎉🎉

@a10footsquirrel's fix for this issue has just been merged in libcdio-paranoia master branch (see here).

This issue can now be closed but first, please thank user @a10footsquirrel for his effort (publicly contributing a fix for the issue and working to get it accepted upstream).

Focusing on #87

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Generic bug: can be used together with more specific labels Upstream Bug The issue is a result of an upstream bug
Projects
None yet
Development

No branches or pull requests

6 participants