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

Reactive paranoia warnings #641

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

saint-erk
Copy link

@saint-erk saint-erk commented Jan 5, 2025

Currently (on develop), ripping using an offset > 587 always warns you about the upstream cd-paranoia bug. It tells you the warning that will be logged when cd-paranoia fails in that way, and then you see the warning when it fails. Here's that output when using cd-paranoia versions 10.2+2.0.1, which still has the offset bug:

WARNING:whipper.program.cdparanoia:because of a cd-paranoia upstream bug whipper may fail to work correctly when using offset values > 587 (current value: 667) and print warnings like this: 'file size 0 did not match expected size'. For more details please check the following issues: #234 and libcdio/libcdio-paranoia#14
WARNING:whipper.program.cdparanoia:file size 0 did not match expected size 64080284
WARNING:whipper.program.cdparanoia:non-integral amount of frames difference

This branch changes the offset warning to appear after the rip fails with size zero with a large offset, and it suppresses the other warnings about 0 not being equal and the non-integer number of frames. Here's that new output, again with 10.2+2.0.1:

WARNING:whipper.program.cdparanoia:file is empty, possibly because of a cd-paranoia upstream bug when using offset values > 587 (current value: 667). For more details please check the following issues: #234 and libcdio/libcdio-paranoia#14

Therefore, when I upgrade cd-paranoia to a version that fixes the offset bug, I can rip my CD without getting the warning about large offsets at all. This resolves issue #635 without trying to figure out whether a fixed version of cd-paranoia is installed. Here's proof with the updated cd-paranoia (10.2+2.0.2), same drive, same CD, but now there's no offset warning between the Q channel CRC check and the successful rip:

...
Track 7 finished, found 30 Q sub-channels with CRC errors
INFO:whipper.command.cd:ripping track 1 of 7: 01. Rush - Bastille Day.flac
INFO:whipper.program.cdparanoia:checksums match, 5d123cc6
...

Additional fix in this branch: One of the AccuRip tests pulls an entry from the live database and asserts the parsed values match the test-defined values, but the entry has been updated with new confidence numbers since the test was written. I've addressed this by changing the assertion about the confidence count to be "greater than or equal" instead of "equal" so that it won't break with future changes to the AccuRip that are outside this project's direct control.

Considered but not done: I considered moving the warning for the bug on track count 99 in the same way, but I don't have such a CD on hand to test.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💖 Thanks for opening your first pull request here! 💖

@saint-erk saint-erk force-pushed the reactive-paranoia-warnings branch from 9e81466 to 2d90289 Compare January 5, 2025 05:03
Upstream cd-paranoia fixed the offset issue, but we can't check
whether the installed version contains the fix. Instead of warning
everyone before the rip starts, warn only the people who get
an empty file with the large offset.

Don't show size mismatch warnings for zero-size files

Signed-off-by: Avery <[email protected]>
@saint-erk saint-erk force-pushed the reactive-paranoia-warnings branch from 2d90289 to 12c6688 Compare January 5, 2025 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant