Skip to content
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

Open
Keithcat1 opened this issue Mar 2, 2022 · 7 comments

Comments

@Keithcat1
Copy link

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:

    def get_data(Channel self, unsigned char[:] data):
        BASS_ChannelGetData(self.channel, &data[0] if data is not None and data.shape[0] > 0 else NULL, <QWORD>data.shape[0]) # if the memoryview has a length of 0 or is None, pass NULL

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.

@Timtam
Copy link
Owner

Timtam commented Mar 2, 2022

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.

@Keithcat1
Copy link
Author

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])

@Timtam
Copy link
Owner

Timtam commented Mar 2, 2022

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.

@Keithcat1
Copy link
Author

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.
Also see the readinto() method which is implemented by bytes-like file objects like io.BytesIO and any file object returned by open(file, "rb")

@Timtam
Copy link
Owner

Timtam commented Mar 3, 2022

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:

length = strm.get_length()
data = strm.get_data(length)

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.

@Keithcat1
Copy link
Author

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.

@Timtam
Copy link
Owner

Timtam commented Mar 4, 2022

I'd be interested in seeing a performance comparison here as soon as we got both implementations ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants