search

mk-fg / python-pulse-control

Python high-level interface and ctypes-based bindings for PulseAudio (libpulse)
https://pypi.org/project/pulsectl/
MIT License
170 stars 36 forks source link

Asyncio Integration #46

Closed mhthies closed 3 years ago

mhthies commented 3 years ago

This Pull Request adds a complete asynchronous interface for Python 3's asyncio event loop framework to the library. The PulseAsync interface class is a 1:1 copy of the current Pulse class, except that every method is defined async, i.e. returns an awaitable coroutine instead of the plain result. Additionally, I replaced event_callback_set() etc. with a method subscribe_events() which returns an asynchronous generator over the subscribed events.

For the control flow, I chose none of your options from https://github.com/mk-fg/python-pulse-control/issues/11#issuecomment-259560564, but instead I created a new PulseAudio main_loop_api implementation, using the asyncio event loop to call the registered PA defer/io/timeout event callbacks asynchronously. This requires quite involved ctypes function pointer handling, but it just works from a user's perspective: The Pulse Audio asynchronous event handling is completely integrated into the Python asyncio event loop, so we can use pulsectl like any other asyncio library, without needing multiple threads or control flow inversion to get other asynchronous tasks running concurrently with libpulse. This implementation of the libpulse main_loop_api is provided in pa_asyncio_mainloop.py and is completely independent from the rest of the library – it is only used by the PulseAsync class for providing this mainloop implementation to the PulseAudio context.

The PulseAsync class shares most of its code with the Pulse class, especially all the method/action definitions and the returned PulseObject classes with all their magic argument handling. I only needed to rework the state change handling in _pulse_state_cb(), connect(), etc. and the heart of all PulseAudio methods, the _pulse_op_cb() context manager, which waits for the PulseAudio actions to provide a result via a callback method (which is now implemented via asyncio Futures). Apart from that, I only added the async keyword here and there.

All the asyncio features should be compatible with Python 3.6 and newer, I didn't test on Python 3.6 yet, but I plan to do so. (EDIT: tested with Python 3.6. Works.) To avoid breaking the library for older Python versions, the async interface is only included in the __init__.py module if sys.version_info >= (3, 6).

I copied the unittest suite in dummy_instance.py as well, with similar adaptions (some async and await here and there`) for using the async interface correctly. On my machine (Python 3.9, PulseAudio 14.2) the tests are all green. (EDIT: Same holds on Debian Buster with Python 3.6 and PulseAudio 12.2.)

Due to this simple adaption of the pulsectl module, I decided to try to go the way of adding this async interface (with only minimal changes compared to the existing blocking interface) to this library, instead of writing a new async PulseAudio library from scratch, as you proposed in https://github.com/mk-fg/python-pulse-control/issues/32#issuecomment-523782757. In any case, I would have had to use or rewrite the ctypes wrappers for libpulse (as given in _pulsectl.py) in a very similar way and provide some abstraction of the resulting PulseAudio objects like PulseObject. Thus, I think it is more appropriate to include the async interface in the pulsectl. If you don't agree on this, I'll probably publish the code in a new library on top of pulsectl.

Fixes #32

mk-fg commented 3 years ago

chose none of your options from #11 (comment)

Same thread also had this comment: https://github.com/mk-fg/python-pulse-control/issues/11#issuecomment-260447500

Don't you think it'd be better to have a separate asyncio-only module, maintained and installed separately? I mean, not like you'd ever need two of these together in the same app (it's either asyncio or a blocking code).

And even sharing ctypes between the two seem to be more trouble than it's worth, as if tomorrow I'll decide to change stuff there for blocking wrappers, it'd impose extra maintenance burden of having go and test/fix all asyncio code for these changes as well, while having these separate will work perfectly fine.

Maybe just throw away all the blocking code, simplifying asyncio bits greatly in the process (as now they don't need to co-exist with that old stuff), and publish as a separate project, which asyncio app devs can easily install instead of dragging in all this old garbage? Will be happy to link it in the description here for anyone looking to use the thing from asyncio.

mk-fg commented 3 years ago

If you don't agree on this, I'll probably publish the code in a new library on top of pulsectl.

I think that's indeed a much better way to go about it. Imo ctypes code and constructors from there would be easier to maintain for each of the two (sync/async) backends separately, feel free to just copy the whole thing, iirc license there should be permissive enough.

mhthies commented 3 years ago

Imo ctypes code and constructors from there would be easier to maintain for each of the two (sync/async) backends separately, feel free to just copy the whole thing, iirc license there should be permissive enough.

Well, the ctypes code in _pulsectl.py is actually asynchronous: It's a plain wrapper of the callback-based asynchronous interface of libpulse. Thus, it can be shared between the blocking and async interface and I don't see a point in rewriting those ~700 loc. Same for the modeling of the PulseObject classes and their attributes, which are parsed from the c structs.

Personally, I don't like copy&pasting large chunks of code – while giving the flexibility of developing the code base in different ways, it also yields the maintenance burden of having to apply bug fixes and improvements from the original project manually. So, I'm left with the possibilities of forking this library and publishing a new version under a different name with both interfaces (blocking and async – no need to drop the blocking code) or create a new async library which depends on pulsectl to reuse its code – which is probably what I will do now.

Don't you think it'd be better to have a separate asyncio-only module, maintained and installed separately? I mean, not like you'd ever need two of these together in the same app (it's either asyncio or a blocking code).

Yes, that's right, but still, I think the ctype wrappers can easily be shared by both implementations and should only be maintained in a single place. So the straight-forward approach was to propose the async interface to be added to this library – although I was aware of your previous comments about an async interface. However, since you seem to be really against the addition of this interface, I think I'll go the way of an additional library described above.

mk-fg commented 3 years ago

I don't really mind copy-pasting, but also think that "maintenance burden of having to apply bug fixes and improvements from the original project manually" doesn't apply much in this case. IIRC these are just header files with minimal logic, so should be no need to update anything there after they match libpulse ABI and have everything that higher-level code wants (and ideally nothing more). And if that ever changes, you'll probably want to update all higher-level wrappers as well anyway, as it's unlikely to be just a reshuffling of fields in a struct on a whim.

Pretty certain that maintaining essentially two separate projects sharing glorified .h file in the same repo would be a lot more trouble than it's worth (conflicts, coordination, issues for separate parts, style diffs, etc etc). IMO it's worth splitting these just to not have to write two separate README files for two different APIs :)

I think I'll go the way of an additional library described above.

Maybe drop a message once you settle on the pypi name and repo for it if you'll remember, so that I can link it here properly instead of that goofy "here are some hacks and links for asyncio" section.

mhthies commented 3 years ago

For anyone stumbling across this PR: I created a new Python library pulsectl-asyncio from the contents of this PR. Just as proposed in this PR, it provides an asyncio implementation of the libpulse main_loop_api and an async-ified high-level Python frontend, but depends on pulsectl for the PythonObject classes and the ctypes wrappers of libpulse. It's available on PyPI and published on GitHub.

@mk-fg: Thanks for already adding it to the README file! :)

mk-fg commented 3 years ago

If python code is fast enough for this, you can probably also extend it beyond just controlling pulseaudio, but to actually feed or fetch audio samples to/from it, to play/record streams or maybe even process them in python. It's kinda what get_peak_sample() already does, and doesn't make much sense with blocking interfaces. (SoundWall fork of this repo also has some sutff along those lines - https://github.com/SoundWall/python-pulse-control/blob/ac371fb/pulsectl/pulsectl.py#L979-L1053 )