Open jnitsun opened 2 months ago
Hi @jnitsun! Thanks for this PR - I always appreciate it when folks send in new code to review.
I know this is your first pull request, so forgive the amount of detail below - these are all friendly notes that I hope will be helpful.
CONTRIBUTING.md
says that we use clang-format (specifically the LLVM style - although it's not listed explicitly there). If you run clang-format -i --style LLVM <filename(s)>
before committing, these extraneous style changes should go away.AudioStream
class - both of which include documentation (great!) - but you haven't changed the docstring of the AudioStream
class itself, so users won't know how to use the new functionality.AudioStream
API in a way that causes a couple of problems:
AudioStream
has both an input and output, and is designed so that people can process an audio stream through a Pedalboard
. In adding this file-playback functionality, the input_audio_stream
argument is silently ignored while the provided audio file loops indefinitely. This violates separation of concerns and makes the API confusing for users. It's not intuitive for a newcomer to Pedalboard to expect that the AudioStream
class is also capable of reading a file from disk and playing its contents to the output device in a loop; you'd expect the AudioFile
class would deal with reading audio from disk instead.I haven't spent much time thinking about a design for this API, but if I were to apply the design principles used for the rest of Pedalboard, I'd suggest a design that allows users to pass audio buffers (i.e.: np.ndarray
objects) into an AudioStream
for playback:
input_audio_device
argument Optional[str] = None
instead, so that users don't have to pass an input audio device in the case where they want to play back audio directly.
None
default parameter will also require the output_device_name
to become Optional[str] = None
(as positional arguments must come before keyword arguments), and you'll have to add a check in the constructor to throw an explicit error if the output device name is missing.write
to AudioStream
that takes an np.ndarray
and copies it into an internal thread-safe buffer for playback
input_audio_device
was provided, write
should throw an exception explaining that this AudioStream
object is streaming audio from <device_name>
and cannot be written to.AudioStream
is running, it plays back the available audio in the internal buffer (through the provided plugins).def_property_readonly
) to indicate how many samples remain in its internal buffer, so callers can decide when to provide more samples.AudioStream
close
method should be changed to block the close of the stream until the provided buffer is finished playing; and could optionally take a force: bool = False
parameter to stop the stream despite it having some buffered data.Here's an example of how someone might use this API, showing how it would be consistent with the existing Pedalboard API:
with AudioFile("my_file.mp3") as f:
with AudioStream(output_device_name="MacBook Pro Speakers") as stream:
while f.tell() < f.frames:
# Read audio from the file, decode it, and write it to the AudioStream for playback in chunks:
while stream.samples_buffered < 8192:
stream.write(f.read(1024))
# Before this print() is called, stream.close() will implicitly be called,
# which will block until all audio has been played back.
print("Playback done!")
I know that the Pull Request you've made here will technically work (and has tests - thank you for that!) - but merging it as-is would create an API that is likely to confuse users and doesn't align with the design principles of the existing API. The API I've proposed here is probably going to be a lot of work to implement, so I don't expect you to make all of those changes in this PR unless you're feeling particularly ambitious. 😅
Enhanced the AudioStream class by introducing an optional parameter to accept an audio file.
Fixed #294.
Made alterations to the AudioStream class that now takes in an optional audio file parameter. If the parameter is not specific, it works as it did before.