-
Notifications
You must be signed in to change notification settings - Fork 90
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
False positive on HTOA #82
Comments
Have you listened to the hidden track? Is it completely silent? Some quiet noise? The peak value definitely is very low. |
Yeah, this one doesn't look like a bug to me: sure HTOA's peak is very low level (about -67 dBFS) but that section isn't digitally silent. This is whipper's expected behavior (as stated in the README, emphasis mine):
If you have any remaining questions about this issue, do not hesitate to ask for clarification. Thanks, |
Is it considered a bug of EAC to not rip it then? Could this HTOA track exist because of scratches? Here is the actual .flac so you can decide if it is considered the correct output. |
No, it's a design decision: please read about it here.
It could happen but, looking at the spectrogram and the waveform, this one seems not to be the case. Here is the waveform: And the spectrogram: |
@BoBeR182 |
This will effect getting a 100% rip, as different rippers will have different track lists now, especially if the HTOA is not a real track as in this case. |
HTOA is on track zero. So it won't mess with the actual track numbers. We can tweak the silence ratio. Unrelated, but somehow related, any clue how EAC handles CDs with more than 99 songs? (99 tracks, max 99 indexes per track, normal index = 01) |
If I'm interpreting this correctly to mean that you will skip htoa that is "almost" silent, please don't. I'm already uneasy that it uses floating point comparrison and doesn't just check exact integer equality to zero. |
I’ve to say I totally agree with @kevmitch. I thought we we’re looking for exactly zero, but apparently none. Maybe I took that idea from one suggestion somewhere or one version somewhere else. I must say that @thomasvs last words on the topic (maybe in a blog post, can check if you guys want) were that they was more work to be done on that issue than one may think/the pull request was doing. But I don’t know what it meant by that. |
I too think that's not a good idea. --- reference/whipper/morituri/command/cd.py
+++ edited/whipper/morituri/command/cd.py
@@ -41,7 +41,7 @@
logger = logging.getLogger(__name__)
-SILENT = 1e-10
+SILENT = -32768
MAX_TRIES = 5
DEFAULT_TRACK_TEMPLATE = u'%r/%A - %d/%t. %a - %n'
@@ -461,8 +461,8 @@
if number == 0:
# HTOA goes on index 0 of track 1
# ignore silence in PREGAP
- if trackResult.peak <= SILENT:
- logger.debug('HTOA peak %r is below SILENT threshold, disregarding', trackResult.peak)
+ if trackResult.peak == SILENT:
+ logger.debug('HTOA peak %r is equal to the SILENT threshold, disregarding', trackResult.peak)
self.itable.setFile(1, 0, None,
self.ittoc.getTrackStart(1), number)
logger.debug('Unlinking %r', trackResult.filename)
--- reference/whipper/morituri/program/sox.py
+++ edited/whipper/morituri/program/sox.py
@@ -16,11 +16,11 @@
if not os.path.exists(track_path):
logger.warning("SoX peak detection failed: file not found")
return None
- sox = Popen([SOX, track_path, "-n", "stat"], stderr=PIPE)
+ sox = Popen([SOX, track_path, "-n", "stats", "-b", "16"], stderr=PIPE)
out, err = sox.communicate()
if sox.returncode:
logger.warning("SoX peak detection failed: " + str(sox.returncode))
return None
# relevant captured line looks like:
- # Maximum amplitude: 0.123456
- return float(err.splitlines()[3].split()[2])
+ # Max level 23170
+ return int(err.splitlines()[2].split()[2]) Of course, as sox's peak level is currently outputted as floating point (percentage FS), other code paths needs to be reworked to handle the new return value format correctly. |
Wouldn't silence be 0? Why -32768? |
That's because CDDA audio samples are encoded as signed 16-bit two's complement integers, with values ranging from −32768 to +32767 (from -2^15 to 2^15 - 1). Here's an excerpt from SoX's stats effect:
|
Yeah, I know, and that's why I'm asking. Silence should be 0, since it's neither negative nor positive. -32768 would be plain negative. If you generate a silence track, all samples will be 0. |
You're right: sorry (I've got confused and that's what happen when you keep coding at late night...) |
@JoeLametta Why did you close this? The proposed patch wasn’t really discussed or rejected, right? |
I've closed this issue because what's reported as false positive is just the result of a design decision. I don't think we really want to ignore "almost" silent HTOAs... If you were referring to the patch which uses integer comparison for silence, I won't drop it (although it's incomplete) but it's a better idea to discuss it in a separate (new) issue. ;) |
https://musicbrainz.org/release/e053ebec-04e5-45ec-8eb2-6fa6af67867b
Whipper detects a HTOA on this CD, EAC does not.
The text was updated successfully, but these errors were encountered: