-
Notifications
You must be signed in to change notification settings - Fork 64
RSDK-12148 RSDK-12149 AudioIn and AudioOut wrappers #1021
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
base: main
Are you sure you want to change the base?
Conversation
…40ba84b7edd5e06bcb7499e42f2d6
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.
Looks good!
I assume we don't want people using AudioInput anymore. Can you add deprecation warnings to the AudioInput classes?
src/viam/audio_codecs.py
Outdated
| @@ -0,0 +1,26 @@ | |||
|
|
|||
| class AudioCodec: | |||
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.
Move this to media/audio.py
Also, this might be better served as an enum rather than a class
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 want to allow for flexibility beyond these predefined codecs so changed to inherit from both enum and str
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! Just a couple small comments/questions but otherwise looks good to me!
examples/server/v1/components.py
Outdated
|
|
||
| for sample in range(samples_per_chunk): | ||
| time_offset = (i * chunk_duration_ms / 1000) + (sample / self.sample_rate) | ||
| amplitude = int(32767 * 0.2 * math.sin(2 * math.pi * 440 * time_offset)) |
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.
(q) I have absolutely zero idea why any of these numbers were selected though I assume it's all reasoned choices. Would someone who wanted to look at this example to understand how to use an AudioIn be reasonably expected to understand what these numbers are for and if/when they might want to change them? If not, can we add some comments explaining?
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.
good point, I made most of them variables/added a comment to clarify
examples/server/v1/components.py
Outdated
| if extra is None: | ||
| extra = {} |
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.
(q, minor) this isn't actually doing anything, is it worth keeping here? Same with get_properties below.
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.
removed
tests/mocks/components.py
Outdated
| return command | ||
|
|
||
| async def get_geometries(self, *, extra: Optional[Dict[str, Any]] = None, timeout: Optional[float] = None, **kwargs): | ||
| return [] |
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.
(minor, nit) could we have this return self.geometries and set self.geometries in the init, akin to what we're doing with MockAudioIn? I'm always a little wary of these test functions returning the zero value of a type because it's hard to distinguish between "everything is working correctly and we got the expected value" and "things are failing but it's silent because we're getting a default zero value which happens to be what we were expecting".
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.
changed to match the audioin tests
Wrappers for this proto update PR https://github.com/viamrobotics/viam-python-sdk/pull/1019/files , adding AudioIn and AudioOut new component types.
Manually tested using server and client example.