-
Notifications
You must be signed in to change notification settings - Fork 108
Media Devices #493
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
Media Devices #493
Conversation
theomonnom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!!
| except StopAsyncIteration: | ||
| exhausted = True | ||
| break | ||
| # AudioStream may yield either AudioFrame or AudioFrameEvent; unwrap if needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The correct typing should be:
self, stream: AsyncIterator[AudioFrame | AudioFrameEvent], buf: np.ndarray
| stream_kwargs["mapping"] = mapping | ||
| elif mapping is not None: | ||
| logging.getLogger(__name__).warning( | ||
| "sounddevice.InputStream does not support 'mapping' in this version; " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to support that? or only latest?
| input_channel_index, | ||
| ) | ||
|
|
||
| input_stream = sd.InputStream(**stream_kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Ideally we keep the typed args (no dict)
| def open_output( | ||
| self, | ||
| *, | ||
| apm_for_reverse: Optional[AudioProcessingModule] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be exposed to the user. Let's make this an internal detail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean should not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sry, it shouldn't
c02ce27 to
7e0df4f
Compare
| from typing import Any, AsyncIterator, Optional | ||
|
|
||
| import numpy as np | ||
| import sounddevice as sd # type: ignore[import-not-found, import-untyped] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make sure to import sounddevice lazily
| self._track_streams[track.sid] = (stream, stream_iterator) | ||
| self._mixer.add_stream(stream_iterator) | ||
|
|
||
| async def remove_track(self, track: Track) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this method be sync instead of async? like add_track
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it awaits stream.aclose() in that function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m thinking more in terms of API design rather than the underlying reasons. For consistency, I’d make both methods async
MediaDevicesthat provides centralized access to local audio input/outputs.