[larsoner]

Fix a few minor bugs I came across:

1. Protect memset call for input-only streams
2. Add a __del__ for mixers that ensure they are closed. Otherwise a garbage-collected mixer could keep calling a callback with a freed state.
3. Have the cancel action keep the action alive in the meantime, otherwise the callback could potentially read freed memory
4. Probably the most useful for me personally -- raise a RuntimeError if 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.)

[mgeier]

Thanks for this PR!

[mgeier]

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.

[larsoner]

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)

[mgeier]

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

[larsoner]

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...

[mgeier]

Note that there were problems with abort() in older PortAudio versions (see spatialaudio/python-sounddevice#75 (comment)), but that's probably not relevant anymore.

[larsoner]

From that thread it looks like the hang was with 2010/2014-era PortAudio builds and had already been fixed upstream by 2017, so I think we might as well keep the abort()

[mgeier]

This repo has no agentic guidance and we probably don't need it yet, but happy to talk more about it if desired.

AFAICT, you are the only (regular) contributor, so if you like to have any of this, feel free to add it!

[mgeier]

Thanks for the update!

Now that I'm thinking a bit more about this ... there's really no need to push the done_actions one by one into the queue. Why not send the whole list in a single write operation? It's just a linked list, so the Python side could then simply walk the list and discard its elements one by one.

[larsoner]

Okay should be done now @mgeier !

[mgeier]

Thanks! I think your implementation is perfectly fine, but I'm quite pedantic, so I have more suggestions (not necessarily requests, though):

1. The style of handling the linked list is different than in the existing code, which uses ...addr to store the address to a pointer instead of the pointer itself. This typically avoids an if clause (which isn't much of a performance hit, but I consider it more a matter of style, see also https://github.com/mkirchner/linked-list-good-taste). So the idea would be to use last_done_action_addr instead of last_done_action.

2. There is a trade-off between storing an additional pointer in state vs. walking the list to get to its end (I guess the list would be empty in the typical case, so there is not much walking to do). Again, this is not so much a question of performance but of style. The existing code doesn't store last_action_addr in the state but instead walks the list when necessary. Instead of a new field in state, we would have a new local variable last_done_action_addr that's initially set to &state->done_actions (and in remove_action() we walk the list until we find NULL and store its address as the new value).

If you do both (and maybe even only with point 1?), remove_action() wouldn't need to get the state at all, but instead it would get last_done_action_addr.

I would do both, but I think it would also be possible to do only one of them.

All this is of course a matter of style and taste, the only real argument is consistency with the existing code.

What do you think?

[larsoner]

Makes sense to me! There's now a local done_addr in callback, and remove_action now takes and returns that address instead of getting state, so last_done_action is now gone from struct state.

[mgeier]

Thanks!