Implement input options #136

ork821 · 2022-04-05T20:31:07Z

Partial implement of #51

orottier

Hi @ork821, thanks for contributing! Could you have a look at my comments?
Also the CI checks fail, could you run cargo fmt and commit those changes too?
Are you planning to add latency_hint too in this PR?

orottier · 2022-04-06T06:28:33Z

src/media/mic.rs

+///
+/// Field is optional and will be set to hardcoded value.
+#[derive(Clone, Debug, Default)]
+pub struct MicrophoneInitOptions {


Maybe the name MicrophoneOptions would be more consistent with AudioContextOptions

src/media/mic.rs

orottier · 2022-04-06T06:33:41Z

src/io.rs

+        Some(sample_rate)=> supported_configs_range
+            .next()
+            .expect("no supported config?!")
+            .with_sample_rate(CpalSampleRate(sample_rate)),


Requesting a specific sample rate should be done after determining the fallback config.
The fallback config is put in the variable default_config and should only contain values that were actually supplied by cpal, so they should never fail.
Could you try to set the requested sample rate only on the config settings?

ork821 · 2022-04-06T18:50:35Z

Hi, @orottier! I appreciate you for commenting my first contribution and my first Rust open source experience!
I just have pushed fixed code according to your comments.
As for adding latency_hint, I think it would be better to merge PR only with sample rate option.
At first sight, I can't exactly say how to implement this. So you can merge this PR and after that start researching about implementing latency_hint

orottier · 2022-04-07T06:28:04Z

src/io.rs

@@ -425,6 +427,10 @@ pub fn build_input() -> (Stream, StreamConfig, Receiver<AudioBuffer>) {

    let mut config: StreamConfig = supported_config.into();
    config.buffer_size = cpal::BufferSize::Fixed(input_buffer_size);
+    if options.sample_rate.is_some() {


This can be written better as:

if let Some(sample_rate) = options.sample_rate { // do stuff with sample_rate }

This avoids the panic path of the unwrap call. See also https://doc.rust-lang.org/rust-by-example/flow_control/if_let.html

orottier · 2022-04-07T06:32:40Z

This is going in the right direction :)

I left one nitpick on your code, please have a look.

Also, the build is still failing, you can check it locally with cargo clippy --all-targets
It says: error: this argument is passed by value, but not consumed in the function body which I think is a false positive and can be ignored with #[allow(clippy::needless_pass_by_value)] similar to https://github.com/orottier/web-audio-api-rs/blob/main/src/context/online.rs#L103

After that we can merge and leave the latency for another time

orottier · 2022-04-07T06:36:13Z

If you'd like I can get you started with some other issues, maybe #44 or #101

ork821 · 2022-04-07T18:48:29Z

This is going in the right direction :)

I left one nitpick on your code, please have a look.

I fixed this. Thank you for your help. This is very important for my skills.

Also, the build is still failing, you can check it locally with cargo clippy --all-targets It says: error: this argument is passed by value, but not consumed in the function body which I think is a false positive and can be ignored with #[allow(clippy::needless_pass_by_value)] similar to https://github.com/orottier/web-audio-api-rs/blob/main/src/context/online.rs#L103

Also fixed this. cargo build, cargo test and cargo clippy --all-targets runs without warnings.

As for other issues, maybe I would try #101. Or will find another issue to fix :)

orottier · 2022-04-08T06:49:35Z

Great. You forgot about cargo fmt though, but I fixed it for you + an unrelated fix because rust 1.60 was released yesterday.

Add a note that we currently ignore the value. Since the constructor options are now identical for the AudioContext, we can use the AudioBufferOptions for the mic too.

orottier · 2022-04-08T07:16:32Z

@ork821 Since we are nearing a v1 release we need to take care of API stability.
Hence I added the latency_hint option for the mic too (but it is left unimplemented). We can fix this within the v1 version
Please have a look at my latest commit :)

ork821 · 2022-04-08T10:28:42Z

@ork821 Since we are nearing a v1 release we need to take care of API stability. Hence I added the latency_hint option for the mic too (but it is left unimplemented). We can fix this within the v1 version Please have a look at my latest commit :)

Hi, @orottier. Thank you for fixing my small cargo fmt fault.

I am not sure that usage same struct AudioContextOptions is good desition.
For example, according to https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/AudioContext we can have only latencyHint and sampleRate. But according to https://w3c.github.io/mediacapture-main/#dom-mediatrackconstraintset we can have echoCancellation, channelCount, deviceId (and so on) as input device options. If for now it's ok, we can merge.

orottier · 2022-04-08T15:04:36Z

That's a valid point, however I would prefer to keep it this way, because either way we need breaking changes when we add the features that you mention to AudioInputOptions.
https://doc.rust-lang.org/cargo/reference/semver.html#struct-add-public-field-when-no-private

Let's op for a new constructor in the future
fn new_with_config(audio_opts: AudioContextOptions, mic_opts: MicrophoneOptions) -> Microphone
We can add all mic specific fields to MicrophoneOptions. The new constructor is not a breaking change so can be added within the v1 version. This gives us a bit of time to flesh out the complete featureset around mic input

ork821 · 2022-04-08T15:24:10Z

That's a valid point, however I would prefer to keep it this way, because either way we need breaking changes when we add the features that you mention to AudioInputOptions. https://doc.rust-lang.org/cargo/reference/semver.html#struct-add-public-field-when-no-private

Let's op for a new constructor in the future fn new_with_config(audio_opts: AudioContextOptions, mic_opts: MicrophoneOptions) -> Microphone We can add all mic specific fields to MicrophoneOptions. The new constructor is not a breaking change so can be added within the v1 version. This gives us a bit of time to flesh out the complete featureset around mic input

Yes, you are actually right about breaking changes. It would be better to implement mentioned method later if we need it. I think you can merge if everything is okay.

Implement input options

8ad00c2

orottier requested changes Apr 6, 2022

View reviewed changes

ork821 closed this Apr 6, 2022

ork821 reopened this Apr 6, 2022

Fix naming and move setting sample rate

4409ae3

orottier reviewed Apr 7, 2022

View reviewed changes

ork821 added 2 commits April 7, 2022 21:17

Fix clippy warnings and fix code to avoid panic

59d3649

Change lines order

a690cbf

orottier added 2 commits April 8, 2022 08:46

cargo fmt

ae13313

Fix needless mut binding, new clippy version has detected this

758c502

orottier added 2 commits April 8, 2022 08:58

Merge branch 'main' into HEAD

2ae06d8

For API stability, already add latency_hint for Microphone

aaaadfa

Add a note that we currently ignore the value. Since the constructor options are now identical for the AudioContext, we can use the AudioBufferOptions for the mic too.

orottier approved these changes Apr 8, 2022

View reviewed changes

orottier merged commit c55b7c1 into orottier:main Apr 8, 2022

ork821 deleted the feature/add_input_device_options branch April 8, 2022 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement input options #136

Implement input options #136

ork821 commented Apr 5, 2022

orottier left a comment •

edited

Loading

orottier Apr 6, 2022

orottier Apr 6, 2022

ork821 commented Apr 6, 2022 •

edited

Loading

orottier Apr 7, 2022

orottier commented Apr 7, 2022

orottier commented Apr 7, 2022

ork821 commented Apr 7, 2022

orottier commented Apr 8, 2022

orottier commented Apr 8, 2022

ork821 commented Apr 8, 2022

orottier commented Apr 8, 2022

ork821 commented Apr 8, 2022

Implement input options #136

Implement input options #136

Conversation

ork821 commented Apr 5, 2022

orottier left a comment • edited Loading

Choose a reason for hiding this comment

orottier Apr 6, 2022

Choose a reason for hiding this comment

orottier Apr 6, 2022

Choose a reason for hiding this comment

ork821 commented Apr 6, 2022 • edited Loading

orottier Apr 7, 2022

Choose a reason for hiding this comment

orottier commented Apr 7, 2022

orottier commented Apr 7, 2022

ork821 commented Apr 7, 2022

orottier commented Apr 8, 2022

orottier commented Apr 8, 2022

ork821 commented Apr 8, 2022

orottier commented Apr 8, 2022

ork821 commented Apr 8, 2022

orottier left a comment •

edited

Loading

ork821 commented Apr 6, 2022 •

edited

Loading