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

Check maximum window size dynamically #102

Merged
merged 5 commits into from Feb 2, 2021
Merged

Conversation

jiixyj
Copy link
Owner

@jiixyj jiixyj commented Feb 18, 2020

This should fix #85 .

@sdroege
Copy link

sdroege commented Sep 2, 2020

In my Rust port of this library I only check for overflows in the calculations of the audio_data here. This allows the user to select any value that can be addressed on the target platform, but also allows the user to select more than memory that is available.

Handling the case where not enough memory is available seems futile with modern OS that are overcommitting, and limiting the API to only allow up to a certain window size seems limiting for user code that might know that a bigger window size can be selected.

As such I would say that this change seems like the right thing to do: prevent overflows in the calculations (as they would cause bad things later on) but allow to allocate as much as the user wants as long as it can be represented.

@jiixyj jiixyj merged commit 752d842 into master Feb 2, 2021
@jiixyj
Copy link
Owner Author

jiixyj commented Feb 2, 2021

Thanks for your thoughts on this! I merged this to master just now.

@jiixyj jiixyj deleted the 85-avoid-allocation-overflow branch February 2, 2021 22:14
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.

Maximum window size is 3 milliseconds
2 participants