-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove the length parameter for functions like Channel.get_data and Channel.put_data #6
Comments
You're right for any put methods like put_data(), but get_data() is meant to retrieve data, there is no buffer to hand over to this method, thus the length parameter has to stay to indicate the amount of data you want to get from the stream. i'm currently working through all objects, adding todo hints wherever I see fit for later and fixing stuff on the fly, i'll deffinitely rework any put methods anyway, thanks for the hint. |
No, you can define get_data in the following way, it will allow the user to pass in any mutable bytes-like object like a bytearray def get_data(Channel self, unsigned char[:] data not None): # we did not mark data as const, so it will be a writable memoryview. Thus, it's safe to pass it to BASS, at least for the duration of this function call
BASS_ChannelGetData(self.channel, &data[0], data.shape[0]) |
Thats absolutely not pythonic here, more like C-ish. Imagine file reading to work like this in Python. Nope, the get data method is meant to return the data, just like any other file reader method too, you just need to tell it how much its supposed to read. Things get that much more complicated if the users need to learn about creating empty buffers, keep them alive, how to actually extract the data so that they can pass it on to other methods and so on. We could probably make the length parameter optional to let it read as much data as it can (if thats not already the case), but your solution really doesn't follow the Pythonic way of life as far as I can tell, sorry. |
Example code of getting all data from a decode stream would look like this: channel = my_output_device.create_stream_from_file("myfile.wav", flags = constants.STREAM.DECODE)
# get the length of the stream
length = channel.get_length()
# make a bytearray big enough to contain all the data
data_buffer = bytearray(length)
# tell BASS to copy data into the buffer
channel.get_data(data_buffer)
# do something with the data
There is no need to keep the data buffer alive because it's a Python object and BASS_ChannelGetData does not need to hold on to the provided pointer. |
I never used bytearrays before you started this discussion, and i'm programming in Python since 2013 :D. Still doesn't feel pythonic to me. Plus, we wouldn't save any parameter. For me it just looks like you're changing the length (int) against a buffer (bytearray), you'd just have to code more due to initializing the bytearray. For me, this code looks shorter:
Your example is longer and more complicated as far as I can tell. I also see no obvious advantages here. We can have both ways however. You can extend get_data() to instead of accepting the length attribute, accept a bytearray or integer instead. If a bytearray is handed as parameter, you just do what you proposed and if its the length as an int, we'll just use the logic that is already there. We can deffinitely handle typing via overloads in that case I guess. I can do it too, but it'd deffinitely not be of high priority to me right now. |
The advantage here is performance. Since the user is providing the buffer, we don't need to allocate any memory. Also, you only need to initialize the bytearray once, then you can replace the data inside it using BASS_ChannelGetData as much as you like. |
I'd be interested in seeing a performance comparison here as soon as we got both implementations ready. |
If you don't want to pass the entire buffer to BASS, then pass a slice, no extra work necessary for Cython. If you want to pass the equivalent of NULL to BASS then do:
You can efficiently retrieve the length of a 1D memoryview with:
data.shape[0]
which is of type Py_ssize_t, which is 32-bits on 32-bit systems and 64-bit on 64-bit systems I believe.
The text was updated successfully, but these errors were encountered: