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

fix mufft crash caused by unaligned input buffer #837

Merged
merged 2 commits into from
Jul 13, 2020

Conversation

hexptr
Copy link
Contributor

@hexptr hexptr commented Jul 12, 2020

Fixes #832

@poco0317
Copy link
Member

poco0317 commented Jul 12, 2020

Runs fine on Windows

@nico-abram
Copy link
Member

Thanks for your first pull request on this repository @hexptr !

This seems to be allocating a new buffer every time we execute the callback
Could it be rewritten to instead allocate the buffer here (And free it if it existed, same as the output buffer) and store it in the class?

auto nOut = static_cast<int>(recentPCMSamplesBufferSize / 2 + 1);
fftBuffer = mufft_alloc(sizeof(cfloat) * nOut);

As an aside, since you're touching this. I think we can just get rid of this line https://github.com/etternagame/etterna/pull/837/files#diff-2a93c97fb469c142670452b11fddef84R365 (We're clearing the vector this writes into right after)

Neither of these is a huge deal and should not stop this from getting merged

@hexptr
Copy link
Contributor Author

hexptr commented Jul 13, 2020

Thanks for the suggestion.

I took the liberty to rewrite both input and output buffers as vectors that use a custom allocator which calls into the mufft_alloc and mufft_free functions.

Both buffers along with the FFT plan, are stored in the class now.

I added more includes and declarations into the RageSound.h header to make things work. I don't know how much we're worried about the compile speed regressing because of things like this.

@nico-abram
Copy link
Member

nico-abram commented Jul 13, 2020

This looks good! I like how we also reuse the plan now. Thank you!

The need for the include in the header is unfortunate, but I don't think it's worth it to try to add some additional indirection (I'm thinking something pimpl like), the mufft header does not look particularly big (About 300 lines and mostly comments)

I compared the CI times and I didn't see any noticeable changes (I imagine CI times are unreliable but it's something)

@MinaciousGrace
Copy link
Contributor

build times look fine to me

@nico-abram nico-abram merged commit f0c6b1c into etternagame:develop Jul 13, 2020
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.

4 participants