mgeier commented 7 years ago

Don't merge this!

This is just an experimental proof-of-concept, which hopefully leads to a better API of the pa_ringbuffer module.

https://github.com/mgeier/python-pa-ringbuffer/pull/4 has to be installed for this to work!

Any suggestions for improvements?

mgeier commented 7 years ago

@tgarc Do you have any comments on this? If not, I'll go through with that ...

tgarc commented 7 years ago

@mgeier Sorry about not getting back to you soon. I updated this to work with my current master and it works fine. My apprehension is how would I add this as a dependency? I could use the repository as a dependency, but I've found that setuptools/pip support for that is not very good. For example, you have to explicitly pass --process-dependency-links when building/installing. There may be a way around that but I think that's probably not the only issue.

Of course if you're thinking about adding this as a pypi package then that might be quite useful.

mgeier commented 7 years ago

Thanks for having a look at this!

Yes, I'm definitely planning to put this on PyPI in one form or another. I just wanted to check first which form makes most sense. And I'm counting on your help! I think this one is too complicated: https://github.com/mgeier/python-pa-ringbuffer/pull/2 (but it still allows a "stand-alone" build, for whatever that's worth). I like the one I used here more (https://github.com/mgeier/python-pa-ringbuffer/pull/4). But is there yet another way? Can it be made even simpler?

For re-use in a package (as opposed to a single module), it should also be possible to copy (or link, or submodule?) the file pa_ringbuffer.py to the package directory and then use a relative import. I haven't really tried that, though.

What do you think about the current API? Do the function names cdef() and init() make sense?

FYI: After this is settled, I'm planning to tackle https://github.com/mgeier/python-pa-ringbuffer/issues/3, and then the usage will probably look something like that:

create_ringbuffer, RingBufferReader, RingBufferWriter = _rb_init(_ffi, _lib)

What do you think about that?

After that, I'm ready for uploading it to PyPI.

tgarc commented 7 years ago

And I'm counting on your help!

Sure. I'm indebted to you sir :bow:

I like the one I used here more (mgeier/python-pa-ringbuffer#4). But is there yet another way?

I haven't had time to look more into how sharing/linking CFFI instances is done so I can't give any good input on this.

Can it be made even simpler?

4 looks pretty darn simple. But is there any real need to wrap RingBuffer inside another class? Do you think it possible that `init` would be called more than once from the same Python instance?

FYI: After this is settled, I'm planning to tackle mgeier/python-pa-ringbuffer#3,

The use cases you're discussing in https://github.com/mgeier/python-pa-ringbuffer/issues/3 are a little over my head. I don't have the imagination to see why one might want to initiate a ringbuffer from C and pass it to Python. On the other hand, I've never done any python embedding or any more advanced coding involving passing C data structures to Python. Separating ringbuffer into reader/writer classes is also interesting, and I think it's conceptually nice, but without being able to imagine more advanced use cases for pa_ringbuffers it's hard for me to see any practical benefit. From my perspective, RingBuffer already does everything I want. But I'd be interested to hear what kind of use cases you're imagining or playing around with.

PS I'm half asleep while writing this so it's possible that none of it makes any sense 😴

mgeier commented 7 years ago

4 looks pretty darn simple. But is there any real need to wrap RingBuffer inside another class?

Are you referring to having _RingBufferBase in addition to the final RingBuffer class?

The reason for that is quite simply to save one level of indentation.

Would you prefer the full class definition to be moved to a single class inside the init() function?

Do you think it possible that init would be called more than once from the same Python instance?

I'm not sure if this will be a real use case, but who knows?

Initially, I was monkey-patching the CFFI instance into the pa_ringbuffer module instance, but I got rid of this (https://github.com/mgeier/python-pa-ringbuffer/pull/2/commits/55595d8b71e36e8f9da23289ac739adb34b54f5e) because it just seemed too hackish.

Do you think this would actually be a good idea?

Or do you have another idea how to do it?

[...] why one might want to initiate a ringbuffer from C and pass it to Python.

TBH, I don't know if anyone would ever want that. It was just an idea. Regardless, I liked the idea of separating the Python ring buffer class from the actual C ring buffer pointer. This makes passing the pointer from Python to C more explicit, which I think is good. That this also allows passing a ring buffer pointer in the other direction is just a nice side effect.

Also, I don't want to hide the fact that I'm using the PortAudio ring buffer. That's not an implementation detail! That's also the reason why I'm calling it pa_ringbuffer, it's not supposed to be a generic ring buffer for Python.

Separating ringbuffer into reader/writer classes is also interesting, and I think it's conceptually nice, but without being able to imagine more advanced use cases for pa_ringbuffers it's hard for me to see any practical benefit.

If you read and write the same ring buffer from Python, there is most likely something going wrong in your code. You would only ever use half of the methods of a given RingBuffer object, the other half is basically forbidden (but this is not checked!). So it's strange to have those methods in the first place. Splitting into two classes would give you an object where you are allowed to call all methods.

Also, continuing my point from above, two separate classes would make it (hopefully) clearer that this is not a generic ring buffer object for Python, but rather a Python interface for one end of a PortAudio ring buffer.

Does that make sense?

mgeier commented 7 years ago

I think it might be a good idea to return a namedtuple (with the attributes create, Reader and Writer) from the init() function. This way, in addition to what I mentioned above:

from pa_ringbuffer import init as _rb_init()
create_ringbuffer, RingBufferReader, RingBufferWriter = _rb_init(_ffi, _lib)

ptr1 = create_ringbuffer(...)
reader = RingBufferReader(ptr1)
ptr2 = create_ringbuffer(...)
writer = RingBufferWriter(ptr2)

... it would also be possible to use it like that:

from pa_ringbuffer import init as _ringbuffer_init()
rb = _ringbuffer_init(_ffi, _lib)

ptr1 = rb.create(...)
reader = rb.Reader(ptr1)
ptr2 = rb.create(...)
writer = rb.Writer(ptr2)

This way, rb would kinda look like a module.

mgeier commented 7 years ago

I've merged the cdef() and init() stuff (https://github.com/mgeier/python-pa-ringbuffer/pull/4).

Then I implemented splitting the ring buffer class as I described above: https://github.com/mgeier/python-pa-ringbuffer/pull/5 https://github.com/mgeier/python-rtmixer/pull/6

And I must confess that I'm not so convinced anymore. It looks like the theoretical advantage of conceptually separating "reader" and "writer" doesn't look that nice in practice. The usage just gets more complicated ...

So I'm inclined to leave it as it is, as a single RingBuffer class.

@tgarc Any thoughts?

tgarc commented 7 years ago

@mgeier sounds good to me as is. Sorry I can't give more input.

mgeier commented 7 years ago

I've moved the project to https://github.com/spatialaudio/python-pa-ringbuffer and published a first release on PyPI: https://pypi.python.org/pypi/pa-ringbuffer.

An example of the RingBuffer documentation can be found at http://python-rtmixer.readthedocs.io/#rtmixer.RingBuffer.

If you have any further suggestions or comments, please speak up!

Otherwise, feel free to close this pseudo-PR any time. You can of course also merge it, but I'm not sure if it really works ...

tgarc commented 7 years ago

thanks again for this module @mgeier

tgarc / pastream

WIP proof-of-concept use of pa_ringbuffer #1

4 looks pretty darn simple. But is there any real need to wrap RingBuffer inside another class? Do you think it possible that `init` would be called more than once from the same Python instance?

4 looks pretty darn simple. But is there any real need to wrap RingBuffer inside another class?

tgarc / pastream

WIP proof-of-concept use of pa_ringbuffer #1

4 looks pretty darn simple. But is there any real need to wrap RingBuffer inside another class? Do you think it possible that init would be called more than once from the same Python instance?

4 looks pretty darn simple. But is there any real need to wrap RingBuffer inside another class?

4 looks pretty darn simple. But is there any real need to wrap RingBuffer inside another class? Do you think it possible that `init` would be called more than once from the same Python instance?