RSDK-12150 Add AudioIn Component #493
Conversation
…40ba84b7edd5e06bcb7499e42f2d6
oliviamiller
requested review from
SebastianMunozP,
lia-viam and
njooma
and removed request for
a team
lia-viam
left a comment
lia-viam
left a comment
There was a problem hiding this comment.
Thanks for the PR, added some suggestions we can iterate on in addition to figuring out the test failures.
src/viam/sdk/components/audio_in.hpp
Outdated
| /// @struct audio_info | ||
| /// @brief Information about a piece of audio data | ||
| struct audio_info { | ||
| std::string codec; |
There was a problem hiding this comment.
are these possible to enumerate in advance or do we allow arbitrary codecs?
There was a problem hiding this comment.
We decided not to include an enum in the protos so we can allow for any codec but we could include some string constants for commonly used codecs
src/viam/sdk/components/audio_in.cpp
Outdated
| AudioIn::AudioIn(std::string name) : Component(std::move(name)) {} | ||
|
|
||
| bool operator==(const AudioIn::properties& lhs, const AudioIn::properties& rhs) { | ||
| return lhs.supported_codecs == rhs.supported_codecs && |
There was a problem hiding this comment.
cool didn't know about this, changed
There was a problem hiding this comment.
oops we need to #include <tuple> for this
| audio_chunk chunk; | ||
|
|
||
| // Convert int16_t samples to uint8_t bytes | ||
| chunk.audio_data.resize(samples.size() * sizeof(int16_t)); |
There was a problem hiding this comment.
I think let's do this with std::copy and/or boost::span
| for (int i = 0; i < samples_in_chunk; ++i) { | ||
| int sample_idx = chunk_idx * chunk_size + i; | ||
| double t = static_cast<double>(sample_idx) / sample_rate; | ||
| double sample_value = amplitude * std::sin(2.0 * M_PI * frequency_ * t); |
There was a problem hiding this comment.
Let's put as much of this stuff as possible into named helpers since it's a bit mysterious and I believe used a couple times
There was a problem hiding this comment.
its only used here but I added helper functions for clarity
| uint32_t subchunk1_size = 16; // PCM | ||
| outfile.write(reinterpret_cast<const char*>(&subchunk1_size), 4); | ||
| uint16_t audio_format = 1; // PCM | ||
| outfile.write(reinterpret_cast<const char*>(&audio_format), 2); |
There was a problem hiding this comment.
This is kind of opaque as written, I think let's do a helper function template (or generic lambda) that expresses this in terms of sizeof, which is what I assume is going on
There was a problem hiding this comment.
Created a template and helper function for writing wav files
| @@ -0,0 +1,50 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
created common audio package for things that will be reused between audioin and audioout
I think the test failures are due to #490 not being merged yet, all tests are passing for me when I have the protos locally |
src/viam/sdk/common/audio.hpp
Outdated
|
|
||
| /// @struct properties | ||
| /// @brief Properties of an audio component (input or output) | ||
| struct properties { |
There was a problem hiding this comment.
| struct properties { | |
| struct audio_properties { |
since properties is a very general name to be putting in the sdk namespace
|
just merged #490 and merged it into this branch, let's see how the tests do now |
|
|
||
| std::string host("xarm-main.aqb785vhl4.viam.cloud"); | ||
| DialOptions dial_opts; | ||
| dial_opts.set_entity(std::string("88dcef8e-db7f-47dc-9b0f-eb08fdc5a97d")); |
There was a problem hiding this comment.
don't forget to revert these before merging! also in the future it's best to git stash these changes rather than committing them while working on the PR
There was a problem hiding this comment.
oops yeah did not mean to commit this, reverted
3 participants
Add the AudioIn component type. Protos in this PR: #490
manually tested using the audio module example included here.