Add OutputStream and InputStream classes

[mgeier]

I didn't like the 3 choices for input_device and output_device in the Stream constructor. The possible values are True (for default device), False-ish (for disabling the device) and a device dict to specify a certain non-default device (or other non-default settings). I think that's quite un-symmetrical and hard for users to remember.

My suggestion is to have a Stream class which always has inputs and outputs, so it is not necessary (and not possible) to disable one of them. For the cases where only one "direction" is needed, I implemented the classes InputStream and OutputStream. I think from a user's perspective this is clearer and easier to use, the implementation of the 3 classes is of course a little more complicated than the 1 class. I'm not very experienced with creating class hierarchies in Python, therefore it's quite likely that my implementation can be significantly improved.

I also replaced the device dicts with device numbers and created additional arguments instead. Now there are more constructor arguments than before, but to simplify those, I added an init object to the module namespace which can be used to select default settings which will be used on subsequent calls to the constructors.

[bastibe]

This is quite a handful. There are some very good ideas in here. I particularly like the modularization of the Stream class. This also helps a lot with making the constructor less daunting.

I am not 100% sure if the init object is the right choice:

defaults = { 'samplerate': 44100 , ... } # instead of the implicit global init object
s1 = Stream( ... , **samplerate)

might be a more idiomatic approach. What do you think?

[mgeier]

I'm totally open for modification/replacement of the init object. I tried quite a few things and this is what seemed most easy to use up to now. There might be much better ways, though ...

I guess you mean s1 = Stream( ... , **defaults), right?

Sure, this is very idiomatic and you can still do this without limitations. The init object can be completely ignored, if you don't like to use it.

One thing that cannot be done (at least I don't know how) with dictionaries, is setting both input_device and output_device by just setting device.

Another idea behind the init object is that when somebody else writes a script using PySoundCard which uses the defaults, you can easily adapt it to your local settings without changing their code! E.g. let's assume they wrote a script called record_stuff.py using default settings and I want to use it with device ID 6 on my computer, then I could write (assuming I use IPython):

import pysoundcard as pa
pa.init.device = 6
%run record_stuff.py

The same thing should work if you import a third-party module written using PySoundCard.

This way, when writing something to be used on a different platform, you don't have to take care of the device settings because the device can be selected from outside of your code.

If you want to avoid this in your application and if you want full control, you just have to set all the arguments explicitly, then the init object won't be used at all.

And finally, the dict stuff is just much more to type and there is no auto-completion.

[mgeier]

I thought there would be more comments about the class hierarchy and about inheriting stuff. I think this is really the first time I made a non-trivial class hierarchy in Python ... Any suggestions for improvements?

What about using input_device in one case and just device in another case, does this make things more easy or more complicated?

[bastibe]

How about defining Stream so it takes a default argument, like

defaults = StreamDefaults() # or Init
class Stream:
    ...
    def __init__( ... , defaults=defaults):
    ...

If I understand this correctly, this would allow the user to do this:

import pysoundcard as pa
pa.defaults.device = 6
s1 = pa.Stream(...) # should use the above setting

But should also allow this:

import pysoundcard as pa
my_defaults = pa.StreamDefaults(device=7)
s2 = pa.Stream(defaults=my_defaults)

Although this is so similar to

import pysoundcard as pa
defaults = {'input_device': 7, 'output_device':7}
s3 = pa.Stream(**defaults)

that it might not be worth the effort. Also, using a defaults argument for the constructor is kind of duplicating default arguments. I don't like that.

The alternative would be to define Stream like so:

defaults = StreamDefaults() # or Init
class Stream:
    ...
    def __init__( ... , input_device=defaults.input_device, ...):
    ...

and hope that this still works:

import pysoundcard as pa
pa.defaults.device = 6
s1 = pa.Stream(...) # should use the above setting

In my opinion, this is the best solution, as it is wholly unsurprising to the user and implements the desired functionality. I'm not sure if this works though. I don't know if the lexical scope only captures the defaults object, or the actual defaults.input_device member value.

[mgeier]

See also https://github.com/bastibe/PySoundCard/commit/2b4bb3d42addfe6702dceb4630b1d1ac446bf827#commitcomment-8589754

Having a defaults argument would indeed be redundant, because there is nothing you can do by passing defaults, which you couldn't also do by setting the arguments one by one or with **a_dict_holding_the_arguments.

Using the attributes as default wouldn't work because the actual object reference is stored:

>>> class StreamDefaults:
...     input_device = 'class instance'
... 
>>> defaults = StreamDefaults
>>> def myfunc(device=defaults.input_device):
...     print(device)
... 
>>> defaults.input_device = 'changed after function definition'
>>> myfunc()
class instance

This only works if the default object itself is mutated (like it would if you pass the whole default object as default argument):

>>> class StreamDefaults:
...     input_device = ['class instance']
... 
>>> defaults = StreamDefaults
>>> def myfunc(device=defaults.input_device):
...     print(device)
... 
>>> defaults.input_device[:] = ['changed after function definition']
>>> myfunc()
['changed after function definition']

[bastibe]

I've had another, longer, look at the code today.

Currently, the class hierarchy is quite clear:

• Low-level _Stream for shared RW functionality, _Reader for R and _Writer for W.
• High-level Stream, InputStream and OutputStream that are different cominations of the Low-level classes.

This distinction between low-level and high-level could be emphasized by putting them in different files. This would also reduce the class clutter somewhat and make the individual files more readable.

A bit of a downside to the current approach is that there are now quite a number of different classes and it is somewhat confusing how they all fit together. I like the class hierarchy though, and I think it easily beats having everything in one giant monster-class.

This could be layed out differently as well:

• Low-level _Stream just like now.
• High-level InputStream and OutputStream that combine the current (InputStream and _Reader) and (OutputStream and _Writer). Additionally, a high-level Stream that derives from the new InputStream and OutputStream (and, for the sake of readability, _Stream, though that is redundant).

This is functionally equivalent to the current solution, but uses fewer classes. However, the clear distinction between low-level and high-level is somewhat muddied by Stream deriving from two other high-level classes.

What do you think?

[mgeier]

I'm not sure if putting things in separate file is worthwhile. Sure, it would unclutter it a bit, but it would make it more complicated on another level.

I initially wanted to derive Stream from both InputStream and OutputStream. The problem was that both have their own copy of device, which need to be renamed to input_device and output_device in Stream. I didn't find another way to do this than having them as hidden attributes in the base classes and giving them the appropriate names in the derived classes.

Is there another way to solve this problem?

I wanted to avoid having InputStream.input_device etc., because this seemed clumsy and repetitive.

Another problem is that each of the "high-level" classes defines their own callback function in the constructor, and they cannot automatically be combined. We could of course try to create a callback function that switches between the different use cases, but I wanted to keep the logic inside the callback functions to a minimum.

[mgeier]

I moved a bit of initialization code around in the stream classes and I created constructors for _Reader and _Writer (0a10b92f17a8bfdcdc42c5cb20292b1e2422a944 EDIT: 3724447ff7e25c193c18eddb18a8828e033bbb9f). This way I was able to remove a few hidden variables (like self._input_channels) and safe a few lines. The general class hierarchy is unchanged, though.

I'm not sure if that's better ...

[bastibe]

I think it's perfectly reasonable to have an input_device in an InputStream. It is a reflection of the fact that Streams can be simplex or duplex. In a way, I actually think that this is more clear than having a context-dependent device.

[mgeier]

I think it's perfectly reasonable to have an input_device in an InputStream.

I heavily disagree. It's a design mistake if "input" appears twice.

It is a reflection of the fact that Streams can be simplex or duplex.

True. But InputStreams can't.

In a way, I actually think that this is more clear than having a context-dependent device.

That's a legitimate concern. But if we go that way, we have to abandon InputStream and OutputStream. I agree that having both device and input_device, sometimes meaning the same thing, is not optimal. But it's just the logical consequence from splitting the stream classes. Splitting the classes should have been a solution to the unsatisfying way to de-activate devices; if we find another way to solve that, we can probably drop this whole PR.

Any alternatives?

[bastibe]

I have worked with quite deep class hierarchies in the past, and by necessity, parent classes are more general than child classes. Child classes are often used as more specialized, simpler frontends for generic parents (cf: facade pattern).

As such, children often inherit a lot of stuff they don't actually need. I therefore think that InputStream.input_device is perfectly reasonable.

If you really can't stand it, how about using InputStream._input_device internally, and supplying a simple accessor device in InputStream and OutputStream, and two accessors input_device and output_device in Stream?

[mgeier]

I therefore think that InputStream.input_device is perfectly reasonable.

I think it's understandable that people do such things, but from a user experience standpoint, it's definitely not reasonable.

If you really can't stand it,

I really can't.

how about using InputStream._input_device internally, and supplying a simple accessor device in InputStream and OutputStream, and two accessors input_device and output_device in Stream?

I think that's basically what I did initially (fd46c888ab173b6d02ec0277db83944555c9cef0). I just didn't use an accessor property (if that's what you are referring to), I just re-bound the value to a new name in the derived class, because the values don't change anyway during the lifetime of the stream objects.

I could make it a property, though, to enforce its read-only-ness, but I'm not sure if that's necessary.

[bastibe]

I like your initial approach, then. Is there a problem with that?

[mgeier]

I created a new pull request (#43) which incorporates most of the changes discussed here.

[mgeier]

I'm closing this in favor of #43.