bastibe / PySoundCard

PySoundCard is an audio library based on PortAudio, CFFI and NumPy
BSD 3-Clause "New" or "Revised" License
87 stars 9 forks source link

Improve callback signature #21

Closed mgeier closed 10 years ago

mgeier commented 10 years ago

This would reduce the amount of copying data and, more importantly, allocating memory (which should be avoided).

This doesn't make a lot of difference in the example programs in tests/, because views can be returned in the callback. But in the more general case (especially if block_length=0), memory has to be reserved and an additional copy has to be made. PortAudio already reserves memory for the buffers, so this should be available directly to the user.

The downside is, that the user now has to use indexing to fill the output array, e.g.:

output[:] = ...

If desired, I can also add this change to the examples and the documentation.

bastibe commented 10 years ago

What happens if the user returns anything other than an integer from the callback?

I know we didn't check for this in the past either, but it would probably be sensible to check for invalid return values there. I don't want to crash the interpreter!

mgeier commented 10 years ago

I just tried it:

CFFI actually does type checks (and doesn't crash the interpreter). If I return the wrong thing, it says:

From callback <function Stream.__init__.<locals>.callback_stub at 0x7f47de849d90>:
Trying to convert the result back to C:
TypeError: an integer is required

If I return something out of the allowed range of 0, 1 or 2, there is no error and the stream stops (I'm not sure if it's equivalent to "complete" or "abort", but I guess this doesn't really matter).

I think that's quite a reasonable behavior by default, would you like an additional check?

bastibe commented 10 years ago

I am somewhat torn about this, but I think the users would appreciate an error message in terms of what they did wrong as opposed to how it was implemented. TypeErrors are quite uncommon for return values. It would probably be better if the error message said something like _Must return continue_flag, complete_flag or abort_flag_.

Maybe we can come up with a more elegant way to deal with this later. Feel free to put this into a separate issue instead of changing this PR, though.

mgeier commented 10 years ago

I added a commit changing the docs/examples to reflect the change in the callback signature.

As for checking the return value, I think we should check as little as reasonable, as we discussed in PySoundFile. Also, the callback wrapper is the only place where performance really matters, so I think we should keep the logic in there at a minimum.

Anyway, as you suggested, we should leave it as is for now and we can change that later if we feel the need for it.

bastibe commented 10 years ago

Alright, this is great, then.

I would love to have tests right now to verify that this does not break anything (even though it looks innocuous enough). But if you tested it, feel free to merge.