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

wrong results with different buffer sizes in the sinewave example #72

Closed
jeychenne opened this issue Mar 28, 2019 · 4 comments
Closed

Comments

@jeychenne
Copy link

I tried to play with the sinewave example, changing the buffer size and sampling rate. Perhaps I'm missing something obvious, but it looks like depending on the buffer size, the results are wrong. Here is the test I ran, using the current master version of FFTS:

    std::cerr << "Expected pitch: 250 Hz" << std::endl;

    auto d = sinewave(2048, 250, 16000);
    auto p = pitch::yin<double>(d, 16000);
    std::cerr << "pitch with 2048 samples @ 16000 Hz: " << p << " Hz" << std::endl;

    d = sinewave(256, 250, 16000);
    p = pitch::yin<double>(d, 16000);
    std::cerr << "pitch with 256 samples @ 16000 Hz: " << p << " Hz" << std::endl;
    
    d = sinewave(4096, 250, 16000);
    p = pitch::yin<double>(d, 16000);
    std::cerr << "pitch with 4096 samples @ 16000 Hz: " << p << " Hz" << std::endl;

    d = sinewave(2048, 250, 22050);
    p = pitch::yin<double>(d, 22050);
    std::cerr << "pitch with 2048 samples @ 22050 Hz: " << p << " Hz" << std::endl;

    d = sinewave(4096, 250, 22050);
    p = pitch::yin<double>(d, 22050);
    std::cerr << "pitch with 4096 samples @ 22050 Hz: " << p << " Hz" << std::endl;

    d = sinewave(8092, 250, 22050);
    p = pitch::yin<double>(d, 22050);
    std::cerr << "pitch with 8092 samples @ 22050 Hz: " << p << " Hz" << std::endl;

    d = sinewave(8092, 250, 48000);
    p = pitch::yin<double>(d, 48000);
    std::cerr << "pitch with 8092 samples @ 4800 Hz: " << p << " Hz" << std::endl;

    d = sinewave(4046, 250, 22050);
    p = pitch::yin<double>(d, 22050);
    std::cerr << "pitch with 4046 samples @ 22050 Hz: " << p << " Hz" << std::endl;

    d = sinewave(4046, 250, 48000);
    p = pitch::yin<double>(d, 48000);
    std::cerr << "pitch with 4046 samples @ 4800 Hz: " << p << " Hz" << std::endl;

And here is the output on my machine (debian x64):

Expected pitch: 250 Hz
pitch with 2048 samples @ 16000 Hz: -1 Hz
pitch with 256 samples @ 16000 Hz: 1781.45 Hz
pitch with 4096 samples @ 16000 Hz: -1 Hz
pitch with 2048 samples @ 22050 Hz: 286.617 Hz
pitch with 4096 samples @ 22050 Hz: 1100.9 Hz
pitch with 8092 samples @ 22050 Hz: 250.226 Hz
pitch with 8092 samples @ 4800 Hz: 250.356 Hz
pitch with 4046 samples @ 22050 Hz: 250.337 Hz
pitch with 4046 samples @ 4800 Hz: 250.626 Hz

Can you confirm this problem, and do you have an explanation for it? Thank you!

@sevagh
Copy link
Owner

sevagh commented Mar 28, 2019

Is this using the sinewave from https://github.com/sevagh/pitch-detection/blob/master/test/util.cpp#L14 ?

I can confirm I see the same behavior. However, I just downloaded a clip of guitar B3 (~247Hz), and created some resampled versions at 22050 and 16000, and the results were mostly OK (except with a buffer size of 256, which is probably too small).

I'm most surprised at 2048@16000 = -1, and 4096@16000 = -1 - I suspect the sine wave generator is doing the wrong thing here.

@jeychenne
Copy link
Author

jeychenne commented Mar 28, 2019

Thanks for your quick reply. I did use test_util::sinewave. And yes, a buffer size of 256 at 16kHz doesn't really make sense since it only contains a couple of periods...

FYI I tried to replace the yin algorithm your mpm and I get the same nonsense, so you're probably right that the sinewave generator has a problem.

Anyway, thanks for making these algorithms available 👍 I'll keep exploring...

@sevagh
Copy link
Owner

sevagh commented Mar 28, 2019

If you make a better sinewave generator, I'd be happy to merge it. Good luck.

@sevagh
Copy link
Owner

sevagh commented Dec 27, 2023

Hi @jeychenne

It's been a long time but I finally found an explanation for this. FFTS has a strange behavior in the complex-to-complex FFT: anthonix/ffts#65

You can see some of my posts in that issue from ~2020 (which is after you posted this issue), but I never put two and two together.

I've been diving deep into fixing some of the corner cases and have settled on:

  • Using the complex-to-complex transform for non-power-of-two sizes
  • Using the real-to-complex transform for power-of-two sizes

I've incorporated your original reported failing test cases into some new test cases and they all pass now:

Note: Google Test filter = *PowerOfTwo*
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from AllAlgorithmsSineWave
[ RUN      ] AllAlgorithmsSineWave.PowerOfTwo
Expected pitch: 250 Hz
pitch with 2048 samples @ 16000 Hz: 250.015 Hz
pitch with 256 samples @ 16000 Hz: -1 Hz
pitch with 4096 samples @ 16000 Hz: 250.015 Hz
pitch with 2048 samples @ 22050 Hz: 253.76 Hz
pitch with 4096 samples @ 22050 Hz: 248.747 Hz
pitch with 8092 samples @ 22050 Hz: 250.74 Hz
pitch with 8092 samples @ 4800 Hz: 249.243 Hz
pitch with 4046 samples @ 22050 Hz: 250.742 Hz
pitch with 4046 samples @ 4800 Hz: 252.297 Hz
[       OK ] AllAlgorithmsSineWave.PowerOfTwo (10 ms)
[----------] 1 test from AllAlgorithmsSineWave (10 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (10 ms total)

256 is understandably poor (it's too short for that pitch, probably), but the others have been improved.

I also generate tones with numpy/librosa and write them to text files for my old unit tests.

This is being worked on in this PR: #87

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

No branches or pull requests

2 participants