-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Audio fingerprint 2.0 #957
Conversation
38a55e0
to
f9d02f3
Compare
src/sources/audio.ts
Outdated
} | ||
} | ||
|
||
async function getAudioFingerprintWithoutTimeout(): Promise<number> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to specify in the name that this function doesn't have a timeout? Seems unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getAudioFingerprint
is already taken, so I have to make this name more specific. I'd be happy to use a better naming. What function names can you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe getAudioFingerprintWithoutTimeout
->getUnstabilisedFingerprint
or getBaseAudioFingerprint
Or rename default export getAudioFingerprint
->getStabilizedAudioFingerprint
But it is not a blocker anyway, you can leave as is if other names don't feel right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 3 functions:
getAudioFingerprint
(stabilized, with timeout)getRawAudioFingerprint
(not stabilized, with timeout)getAudioFingerprintWithoutTimeout
(not stabilized, without timeout)
I don't quite understand how you want to rename all of them.
What do you think about this:
getStableAudioFingerprintWithTimeout
,getAudioFingerprintWithTimeout
,getAudioFingerprint
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe like this
getAudioFingerprint
(stabilized, with timeout) -> as is
getRawAudioFingerprint
(not stabilized, with timeout) -> getUnstableAudioFingerprint
getAudioFingerprintWithoutTimeout
(not stabilized, without timeout) -> getBaseAudioFingerprint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't like the part about timeout
because it looks like more generic function tries to explain the behavior of particular use case
for (let i = 0; i < signal.length; i++) { | ||
const value = signal[i] | ||
// In very rare cases the signal is 0 on a short range for some reason. Ignoring such samples. | ||
if (value === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: might be not safe if all the values are zeros (not sure if it's possible) because we will return NaN as a signal value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is indeed possible and handled by extractFingerprint
(see the comments). getMiddle
is called only if there is at least 1 non-zero sample in the signal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, makes sense, thanks
The new audio fingerprint withstands the fingerprinting protection of Safari 17