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

Add encode-microphone example #291

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

morajabi
Copy link
Contributor

No description provided.

@morajabi
Copy link
Contributor Author

CI failure seems unrelated to this change (?)

@@ -31,6 +31,11 @@ serde_json = "1.0"
bytes = "1.1"
lazy_static = "1.4"
rand = "0.8"
# encode-microphone
cpal = "0.14.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would drop the patch and stick to major and minor like most of the other lines


#[tokio::main]
async fn main() -> Result<()> {
let mut app = Command::new("encode-microphone")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check out clap derive, it's pretty nice.

// Create a MediaEngine object to configure the supported codec
let mut m = MediaEngine::default();

m.register_default_codecs()?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is very opus specific maybe only register Opus

Result::<()>::Ok(())
});

let (sender, frame_receiver) = flume::bounded::<AudioFrame>(3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need to add extra dependency on flume here or can you just use tokio channels?

let (encoded_sender, encoded_receiver) = flume::bounded::<AudioEncodedFrame>(3);

// Encoder thread
thread::spawn(move || {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add some commandline option here with default 48000kHz but it can be overriden in case there is stereo mic with 16kHz sampling rate for example?

let (encoded_sender, encoded_receiver) = flume::bounded::<AudioEncodedFrame>(3);

// Encoder thread
thread::spawn(move || {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need a separate thread for encoder only? can't this run on the same thread as rtp send?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d like that too, but since Encoder didn’t impl Send I guess it wasn’t possible to use it across awaits in an async thread. Is there a solution?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure, (I am not a very good rust async programmer :) )

eprintln!("an error occurred on stream: {}", err);
};

// until it is 960
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make a comment what 960 is, my guess it's 20ms frame with 48kHz sampled data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, I should replace it with a const and a comment.

};

// until it is 960
let mut buffer: Vec<f32> = Vec::new();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with_capacity to preallocate all memory

#[cfg(any(
not(any(target_os = "linux", target_os = "dragonfly", target_os = "freebsd")),
not(feature = "jack")
))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can probably be removed

let host = cpal::default_host();

// Set up the input device and stream with the default input config.
let device = if device == "default" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since you already hardcode device on 316 the else is not relevant here.

@k0nserv
Copy link
Member

k0nserv commented Oct 6, 2022

@morajabi I'm holding of on reviewing this until CI passes FYI

@melekes
Copy link
Contributor

melekes commented Nov 22, 2022

@morajabi friendly ping. Will you have time to address @xnorpx feedback?

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

Successfully merging this pull request may close these issues.

4 participants