Fix low range pitch detection of YIN detector #26
Conversation
| @@ -39,7 +39,6 @@ module.exports = function(config = {}) { | |||
| // Set buffer size to the highest power of two below the provided buffer's length. | |||
| let bufferSize; | |||
| for (bufferSize = 1; bufferSize < float32AudioBuffer.length; bufferSize *= 2); | |||
| bufferSize /= 2; | |||
There was a problem hiding this comment.
This is done below as well with const yinBufferLength = bufferSize / 2;
While I couldn't test too low, this did not seem to cause any issues for me and I was immediately able to detect the low E.
There was a problem hiding this comment.
It seems like this makes the comment on line 39 incorrect, unfortunately, and that comment came from the paper, so I'm hesitant to make this change
|
I looked at the time difference, and this added significant time to YIN tests. As mentioned in PR, I don't think it strictly needs to be doubled since it gets really close to being able to detect that E. It can detect the F, gets within a few Hz. The added time of creating the extra elements of the array + checking them when there's no pitch seems to be hurting. Looking at the algorithm, I don't see anything that makes it strictly necessary to be a power of 2 in size. |
|
Looks like you'll have to update all the tests for a change like this - it somewhat affects the accuracy |
|
It would be nice if this were merged. I'm having this issue as well. |
|
@JacobJT Btw, thanks for the fix. I've tested this and it seems to work pretty well. Previously, nothing at D2 or below was detected at all for me. Now it reliably detects D2. It still has trouble with A1, which is usually thinks is A2, but that's still better than detecting nothing! I see no regressions with detection at higher pitches. |
Fixes #24