Repository navigation
Conversation
There was a problem hiding this comment.
I would normally try to stay on released versions, but if you like to depend on a development version, that's also fine for me.
There was a problem hiding this comment.
v19.7.0 is from 2021 and there's been no tag since (!). There are a few years of upstream fixes (e.g. CoreAudio alignment)
| if not self.closed: | ||
| self.close() |
There was a problem hiding this comment.
I didn't check this, but I have the feeling that close() should be idempotent, shouldn't it?
But then, PortAudio behaves strangely in some cases ... for example, in the past I had to call stop() before close(), see https://github.com/spatialaudio/python-sounddevice/blob/283f40d6ebdedda2c0d609c41a6a14476c1dc87e/src/sounddevice.py#L2966-L2969
There was a problem hiding this comment.
Yeah close() currently tolerates repeat calls (passes NULL to Pa_CloseStream and ignores any error), the closed check is just a shortcut so could be dropped. I could add a self.abort to this, too, to make it maybe safer as well...
There was a problem hiding this comment.
Note that there were problems with abort() in older PortAudio versions (see spatialaudio/python-sounddevice#75 (comment)), but that's probably not relevant anymore.
| # Each action ends up in the result queue exactly once, so limiting | ||
| # the number of pending actions guarantees that it never overflows: | ||
| if len(self._actions) >= len(self._result_q): | ||
| raise RuntimeError('Too many pending actions') |
There was a problem hiding this comment.
I don't understand this check.
The action queue and the result queue already have the same size, and the result queue is drained before trying to write into the action queue, so it should already be guaranteed to never overflow?
At least that was the idea, maybe I was missing something?
There was a problem hiding this comment.
It looks like the callback empties action_q into its linked list every block, so action_q having room doesn't bound the number of current actions. With qsize=16 for example you can 1) enqueue 16, 2) let the callback pick them up, then 3) enqueue 16 more, and have 32 active. When they finish, only 16 fit in result_q, so remove_action() prints "result queue is full" and the rest are never reported back (wait() hangs). This check caps unfinished actions (queued, running, or not yet drained) at the result queue size. So I think qsize already effectively limits the number of concurrent actions in master. Exceeding it now raises an error instead of silently losing actions (master behavior), so I documented this in the Mixer docstring.
Does that make sense? Did you have a different idea for how this should work? The fix (increasing qsize) to make it so result_q (and action_q) bigger seems like it should be safe enough.
AFAICT, you are the only (regular) contributor, so if you like to have any of this, feel free to add it! |
Fix a few minor bugs I came across:
memsetcall for input-only streams__del__for mixers that ensure they are closed. Otherwise a garbage-collected mixer could keep calling a callback with a freed state.RuntimeErrorif you try to enqueue an action and it would otherwise be silently ignored because the queue is full.Bumped the PortAudio dep while I was in there. I found these because I asked Claude Opus 5.5 to investigate queue / PortAudio use since I've occasionally in CIs seen failures related to a queue being full. Opus made some suggestions, I had it implement them, and I reviewed and vouch for them. They all seem like reasonable / useful hardenings to me.
(This repo has no agentic guidance and we probably don't need it yet, but happy to talk more about it if desired. In MNE-Python we've had some nice improvements in terms of efficiency and bugs by having experts use it IMO, so I'll use it a bit here, too, to make little fixes/improvements unless there are objections! I could for example have it add some minimal unit tests, which could help us find some bugs as well.)