Enhance GIL management and error handling in resampling - #31
Open
shauneccles wants to merge 6 commits into
Open
shauneccles wants to merge 6 commits into
shauneccles wants to merge 6 commits into
Conversation
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
fakufaku
left a comment
fakufaku
left a comment
Collaborator
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?
Author
There was a problem hiding this comment.
Rebased on master, so this is no longer in the diff.
| #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?
Author
There was a problem hiding this comment.
It's now OUTPUT_HEADROOM_FRAMES (1024), with a comment: the output frames allocated on top of ceil(input_frames * ratio). Since process() now grows the buffer when libsamplerate fills it, the value no longer affects correctness, only how often a regrowth happens.
| // 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?
Author
There was a problem hiding this comment.
For a fixed ratio, yes: the end_of_input flush emits roughly ratio × the input the converter holds back, i.e. its filter half-length. Measured: 144 input frames for sinc_best (tail of 2304 at ratio 16, 9216 at ratio 64), 20 for sinc_fastest, 0 for linear. That's up to ~37k frames at ratio 256, so the 10000 constant fails with sinc_best above ratio ~70.
After a ratio change, though, there's no fixed maximum: libsamplerate ramps from the previous ratio, so process(x, 4.0) followed by process(200_000 frames, 0.25) produces far more than 50000. So I dropped the idea of a maximum; see the next comment.
| 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?
Author
There was a problem hiding this comment.
Truncating would lose audio. When the output buffer fills, libsamplerate hasn't consumed all the input yet, so whatever we cut is gone (master does this silently today). Instead process() now keeps calling src_process() on the remaining input with a larger buffer until libsamplerate stops short of filling it, and resample() uses the same path. test_process_flush_at_high_ratio and test_process_after_ratio_change cover both cases and fail on master.
… use Output size: a src_process() call can generate more than ceil(input_frames * ratio). With end_of_input the converter flushes the input it held back (about ratio x its filter half-length: 144 input frames for sinc_best, so ~37k output frames at ratio 256), and after a ratio change libsamplerate ramps from the previous ratio, which has no fixed bound. Today process() silently drops that output, and a fixed extra allowance (#22) only moves the failure. Resampler.process() now keeps calling src_process() with a larger buffer until libsamplerate stops short of filling it. resample() uses the same path, since src_simple() is src_new() + one src_process() + src_delete() and cannot continue. Callback: hold the GIL for the whole of the_callback_func, since the buffer_info destructor releases a Python buffer, and store any exception (ours or one raised by the Python callback) instead of letting it unwind through libsamplerate's C frames; read() re-raises it. Also run the asyncio tests in CI (pytest-asyncio). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
shauneccles
force-pushed
the
perf/more_gil_goodness
branch
from
September 27, 2026 04:04
dfa6e5c to
0063ef8
Compare
This was referenced Sep 27, 2026
Releasing the GIL lets two Python threads drive one SRC_STATE at once: four threads calling process() on one Resampler got "Internal error: Bad length in prepare_data ()", and __exit__ from another thread during read() segfaulted. Every method that touches the state now takes a per-object in-use flag and raises RuntimeError if it's already set. A mutex could deadlock, since the holder needs the GIL to grow the output buffer while a waiter may hold it. Two existing clone bugs, in the same code: - src_clone() copies the callback data pointer, so a clone called back into the original CallbackResampler and segfaulted once the original was freed. read() now publishes the active resampler in a thread_local, which the callback uses instead. - A clone's libsamplerate state still points into the original's last callback buffer; the copy now holds a reference to it, where it used to read freed memory. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Author
|
One more commit after reviewing the GIL handling again: releasing the GIL let two threads use one resampler at the same time (corrupted state, and a segfault via |
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
3 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.
Release the GIL while resampling, and make
Resampler.process()/resample()return all of their output.Rebased on current master (which already has #30 and #36). Includes the tests from #32, as suggested in review.
GIL (Tino's commit from #14):
src_process,src_simpleandsrc_callback_readrun with the GIL released, so resampling scales across threads andrun_in_executor. The callback takes the GIL for its whole body: thebuffer_infodestructor also releases a Python buffer. Exceptions raised in the callback (validation errors or the Python callback's own) are stored and re-raised byread()instead of unwinding through libsamplerate's C frames.Output size: a
src_process()call can generate more thanceil(n * ratio):end_of_input, the converter flushes the input it held back, about ratio × its filter half-length (144 input frames for sinc_best, 20 for sinc_fastest, 0 for linear), so up to ~37k frames at ratio 256;On master that output is silently dropped (e.g.
process(x, 4.0)thenprocess(200_000 frames, 0.25, end_of_input=True)returns exactly 50000 frames). A fixed extra allowance (#22) only moves the limit.process()now keeps callingsrc_process()with a larger buffer until libsamplerate stops short of filling it;resample()goes through the same path, sincesrc_simple()issrc_new()+ onesrc_process()+src_delete()and can't continue. This supersedes #22.Thread safety: releasing the GIL means two Python threads can drive one resampler at once. Several threads sharing a
Resamplercorrupted its state (Bad length in prepare_data ()), and__exit__from another thread duringread()segfaulted. Each resampler now raisesRuntimeErroron concurrent or re-entrant use; separate objects still run in parallel.CallbackResampler.clone()(existing bugs in code this PR touches):src_clonecopies the callback pointer, so a clone called back into the original and segfaulted once the original was freed; it also read the original's freed input buffer. Both fixed.Tests: threading/asyncio performance tests from #32 (speedups reported as warnings, not failures, since CI runners vary), plus regression tests for the high-ratio flush, the ratio change, and an exception raised in the callback. The first two fail on master. CI now installs pytest-asyncio.