mmorise / World

A high-quality speech analysis, manipulation and synthesis system
http://www.kisc.meiji.ac.jp/~mmorise/world/english
Other
1.17k stars 251 forks source link

Crash in Dio #78

Closed 3628800 closed 5 years ago

3628800 commented 5 years ago

Hi, there is a crash calling Dio (manifests especially for short signals) when the low-cut filter frequency is large compared to the calculated DFT size.

Specifically, the crash occurs in GetSpectrumForEstimation when fft_size < (cutoff_in_sample * 2 + 1), and DesignLowCutFilter(cutoff_in_sample * 2 + 1, fft_size, y) indexes y (created with length fft_size) upto the first parameter.

mmorise commented 5 years ago

Thank you for your suggestion.

I think that the following modification can solve the problem. We cannot use the low-cut filter in this case, but I don't have another idea to simply solve it. Perhaps, this error rarely occurs, so its simple approach would be reasonable. If you have another idea, please propose it as a pull request. I will consider your idea.

if (cutoff_in_sample * 2 + 1 <= fft_size) DesignLowCutFilter(cutoff_in_sample * 2 + 1, fft_size, y);

3628800 commented 5 years ago

Thanks for the fast response.

A modification to your modification: I think the test needs to be cutoff_in_sample * 3 + 1 <= fft_size based on the maximum loop index in DesignLowCutFilter of (N-1) + (N-1)/2 (in the last loop).

Must the filtering operation be skipped in this case, or does it make any sense to filter the signal up to min(cutoff_in_sample * 2 + 1, 2/3. * (.5 + fft_size))?

mmorise commented 5 years ago

Thank you very much for your comment. Sorry, I'm too busy to modify the program until this weekend. I will modify the program next week.

3628800 commented 5 years ago

Thanks for your time!

mmorise commented 5 years ago

I checked the source code. Based on the result, I would like to propose another idea. In line 592 of dio.cpp, fft_size is determined by the following equation. int fft_size = GetSuitableFFTSize(y_length + (4 * static_cast<int>(1.0 + actual_fs / boundary_f0_list[0] / 2.0))); This equation does not include the length of the low-cut filter. So, the following modification would be effective. int fft_size = GetSuitableFFTSize(y_length + matlab_round(actual_fs / 50.0) * 2 + 1 + (4 * static_cast<int>(1.0 + actual_fs / boundary_f0_list[0] / 2.0))); This equation guarantees that the fft_size is always longer than the length of low-cut filter.

Since the value 50.0 is constant, the modification should also include the additional definition in the constantnumbers.h. const double kLowCut = 50.0; int fft_size = GetSuitableFFTSize(y_length + matlab_round(actual_fs / world::kLowCut) * 2 + 1 + (4 * static_cast<int>(1.0 + actual_fs / boundary_f0_list[0] / 2.0)));

If you have no comment, I will modify the source codes (dio.cpp and constantnumbers.h).

3628800 commented 5 years ago

That seems fine to me. (Although for my particular case this results in an 8x increase in the FFT size, so it might be worth testing on some of your "normal use-case" inputs to make sure the library won't suffer a dramatic slowdown...!)

mmorise commented 5 years ago

I update the source codes in dio.cpp and constantnumbers.h.

In cases where the length of input signal is long, the processing speed would be little affected.

3628800 commented 5 years ago

Thank you.