Enhance GIL management and error handling in resampling#31
Open
shauneccles wants to merge 11 commits intotuxu:masterfrom
Open
Enhance GIL management and error handling in resampling#31shauneccles wants to merge 11 commits intotuxu:masterfrom
shauneccles wants to merge 11 commits intotuxu:masterfrom
Conversation
acquire GIL for execution of callback
attempt to fix exception handling on windows, not tested
Release GIL while resampling
Python bump up
Increase expected size of output sample buffer, to correctly handle input termination
Author
|
For reference this is the results of the asyncio and threading test on my local machine. Looking at #32 which has same tests on same machine at (about) the same time the headlines from this PR is:
|
fakufaku
reviewed
Dec 7, 2025
Collaborator
fakufaku
left a comment
fakufaku
left a comment
There was a problem hiding this comment.
This looks pretty good to me. Please just sync with master.
Would it make sense to combine this with your other PR that contains the tests?
| matrix: | ||
| os: [ubuntu-latest, macos-latest, windows-latest] | ||
| python-version: [3.8, 3.9, "3.10", "3.11", "3.12"] | ||
| python-version: [3.9, "3.10", "3.11", "3.12", "3.13", "3.14"] |
Collaborator
There was a problem hiding this comment.
This has been merged already. Could you please sync with master?
| #endif | ||
|
|
||
| // This value was empirically and somewhat arbitrarily chosen; increase it for further safety. | ||
| #define END_OF_INPUT_EXTRA_OUTPUT_FRAMES 10000 |
Collaborator
There was a problem hiding this comment.
Could you please add some more details about what this constant controls?
| // be more than the expected number of output samples during mid-stream | ||
| // steady-state processing. (Also, when the stream is started, the number | ||
| // of output samples generated will generally be zero or otherwise less | ||
| // than the number of samples in mid-stream processing.) |
Collaborator
There was a problem hiding this comment.
Is there a maximum we could expect?
| output.resize(out_shape); | ||
| } else if ((size_t)output_frames_gen >= new_size) { | ||
| // This means our fudge factor is too small. | ||
| throw std::runtime_error("Generated more output samples than expected!"); |
Collaborator
There was a problem hiding this comment.
Rather than failing, is it possible to truncate the output?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improve GIL management during resampling operations to prevent blocking and enhance performance. Update error handling to manage exceptions more effectively, particularly in callback scenarios. Adjust the expected output size to accommodate additional frames introduced by #22 and remove support for Python 3.8. Update license specification format.
The latter changes are from existing community PRs that have already been merged.