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

Add FFT buffer size of 32768 #59

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

KungFuFurby
Copy link
Contributor

@KungFuFurby KungFuFurby commented Nov 30, 2024

Context for this pull request: The powers of 2 shown here go up to 16384, and I wish to add a few more power of 2's worth of buffer sizes to the mix.

The Terrific Audio Driver by Undisbeliever calls samples_fft_to_spectrum when analyzing a BRR sample (this is created for the SPC700, which has a 64KB limit) by calculating a spectrum in one go (thus, if the samples are large enough they tend to request a buffer size large enough). This works fine up to 16384 samples, but a BRR sample can theoretically be as large as 116496 (though in practice it is usually smaller than this). The result is a panic.

Context for the code in question:
https://github.com/undisbeliever/terrific-audio-driver/blob/241c53fa3706bc89f3190ab7627145c29122c9ce/crates/tad-gui/src/sample_analyser.rs#L926

The code was since amended to limit the analysis size to 16384 samples for the time being. I determined that microfft only goes up to 32768...

@KungFuFurby KungFuFurby changed the title Add FFT buffer sizes of 32768, 65536 and 131072 Add FFT buffer sizes of 32768 Nov 30, 2024
@KungFuFurby KungFuFurby changed the title Add FFT buffer sizes of 32768 Add FFT buffer size of 32768 Nov 30, 2024
@undisbeliever
Copy link

For reference this is the sample analyser KungFuFurby is talking about. It's used to help determine the frequency (tuning) of the BRR sample when it played at 32kHz.

sample-analyser

To quickly fix the panic I am currently truncating the sample when analysing it.

I'm still not sure how to properly analyse a large sample (or even if spectrum-analyzer is fit for the task). Truncating the sample works well enough for now.

Thank you for creating and publishing the spectrum-analyzer crate. It allowed me to quickly build and test the sample analyser dialog over the course of 3 days.

@phip1611
Copy link
Owner

phip1611 commented Dec 16, 2024

Hey, thanks for the interest in my project!

I'm still not sure how to properly analyse a large sample (or even if spectrum-analyzer is fit for the task). Truncating the sample works well enough for now.

I think that this is exactly the way to do it, also in other libraries. Do in in multiple chunks.

Thank you for creating and publishing the spectrum-analyzer crate. It allowed me to quickly build and test the sample analyser dialog over the course of 3 days.

Awesome!!

@phip1611 phip1611 force-pushed the main branch 4 times, most recently from c3f2f27 to 73904b7 Compare December 16, 2024 07:45
@phip1611
Copy link
Owner

phip1611 commented Dec 16, 2024

Hey! Can you please

  • rebase your PR onto the latest main
  • run cargo fmt
  • update this PR accordingly?

Thanks! I'm happy to merge once this is fixed

@KungFuFurby @undisbeliever

@KungFuFurby KungFuFurby force-pushed the fft-add-larger-buffer-sizes branch from 3df9879 to ad2fc81 Compare December 16, 2024 20:00
@KungFuFurby
Copy link
Contributor Author

Thanks for telling me about this, and very good call, given that my first commit was... a bit rushed, to put things simply, and didn't do enough of the research. So I discarded the first commit completely, since those sizes don't actually exist yet (and I seem to have realized that the lower layer stuff has a lot of lines to handle, so I can clearly tell how things are going exponentially-speaking). I also, in the process of doing conflict resolution, changed the microfft dependency to use a tilde requirement so that it can keep up to date as needed, and did a minor wording change to the changelog. And the formatting was modified courtesy of cargo fmt. Yes, it's also force pushed.

@phip1611
Copy link
Owner

Great. Well done! ;)

I'm going to fix the remaining CI error by myself

@phip1611 phip1611 merged commit f8e44ac into phip1611:main Dec 17, 2024
5 of 7 checks passed
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

Successfully merging this pull request may close these issues.

3 participants