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 AudioContextOptions to AudioContext #36

Merged
merged 18 commits into from Nov 10, 2021
Merged

Conversation

Jerboas86
Copy link
Contributor

@Jerboas86 Jerboas86 commented Oct 26, 2021

The user can now specify:

  • the number of output channels
  • the sample rate
  • the output latency
    when building its AudioContext.

If the intented options are not met by the audio hardware capabilities, the AudioContext is built with default_config options, and a warning is printed to the user.

If the user options are missing, the AudioContext is built with primary_config options. If the primary_config options are not met by the audio harware capabilities, the AudioContext is built with default_config options, and a warning is printed to the user.

This PR modify DestinationNode:

  • Adds max_channels_count method
  • Assert ChannelCountConstraints on DestinationNode

This PR adds merger.rs example to demonstrate new capabilities.

Clippy passes on context.rs and io.rs

related to #35
related to #28

Copy link
Owner

@orottier orottier left a comment

Choose a reason for hiding this comment

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

Interesting work, thanks!

id: AudioNodeId,
}

impl AudioContextRegistration {
pub fn id(&self) -> &AudioNodeId {
Copy link
Owner

Choose a reason for hiding this comment

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

There's no point in marking these functions const as the registration creation is not const. Marking functions const is a liability for the future, so I'd rather not

src/io.rs Outdated
config.sample_rate = CpalSampleRate(opts.sample_rate.unwrap_or(config.sample_rate.0));
config.channels = opts.channels.unwrap_or(config.channels);

match opts.latency_hint {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you move this block into a helper function, makes it a bit more readable and then we can re-use if for the InputBuilder

@@ -134,7 +219,8 @@ pub(crate) fn build_output(
(stream, config, sender)
}

pub(crate) fn build_input() -> (Stream, StreamConfig, Receiver<AudioBuffer>) {
/// Builds the input
pub fn build_input() -> (Stream, StreamConfig, Receiver<AudioBuffer>) {
Copy link
Owner

Choose a reason for hiding this comment

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

You should port the latency hint to the input builder too (can be done in another PR if it's too much work). They should be symmetrical in the end

src/context.rs Outdated
self.inner.render_channel.send(message).unwrap();
self.inner
.render_channel
.send_timeout(message, Duration::from_millis(10))
Copy link
Owner

Choose a reason for hiding this comment

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

Why opt for timeout? If the requested latency > 10millis, this will fail for no good reason

}
}

/// `OfflineAudioContext` doesn't start rendering automatically
/// You need to call this function to start the audio rendering
pub fn start_rendering(&mut self) -> AudioBuffer {
// make buffer_size always a multiple of BUFFER_SIZE, so we can still render piecewise with
Copy link
Owner

Choose a reason for hiding this comment

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

I think actually this is no longer needed, now we can handle varying buffer lengths.
We can remove this code in another PR

src/context.rs Outdated
pub fn length(&self) -> usize {
/// get the length of rendering audio buffer
#[must_use]
pub const fn length(&self) -> usize {
Copy link
Owner

Choose a reason for hiding this comment

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

no const please

Copy link
Contributor Author

@Jerboas86 Jerboas86 left a comment

Choose a reason for hiding this comment

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

You should port the latency hint to the input builder too (can be done in another PR if it's too much work)

I didn't port the refactoring to the input stream. i leave it as "good first issue" or for another PR, provide that you agree on the refactoring :)

///
/// * `options` - options contains latency hint information
///
/// # Warning
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned here the correctness of the API is not enforce by the type system. I could make it more strict but at the cost of more complexity. What do you think ?

@orottier orottier merged commit 91c67be into orottier:main Nov 10, 2021
@Jerboas86
Copy link
Contributor Author

I didn't port the refactoring to the input stream. i leave it as "good first issue" or for another PR

is it Ok with you, if i open a good first issue about this topic?

@orottier
Copy link
Owner

Go ahead!

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.

None yet

2 participants