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.

Sign up for GitHub

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

Jump to bottom

changed BUFFER_SIZE to usize (cf. #49) #52

Merged
merged 6 commits into from
Nov 17, 2021
Merged

changed BUFFER_SIZE to usize (cf. #49) #52

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0ca046c
changed BUFFER_SIZE to usize (cf. #49)
b-ma Nov 15, 2021
c78f69c
derive `BUFFER_SIZE: usize` from `BUFFER_SIZE_U32: u32`
b-ma Nov 16, 2021
76a1e43
removed clippy errors
b-ma Nov 16, 2021
370a0d5
increased ulps to make test pass
b-ma Nov 16, 2021
d3578ca
cleaner assertion
b-ma Nov 16, 2021
f989c3f
cast BUFFER_SIZE to u32 when needed
b-ma Nov 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/io.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use std::sync::atomic::AtomicU64;
use std::sync::Arc;

use crate::message::ControlMessage;
use crate::{SampleRate, BUFFER_SIZE};
use crate::{SampleRate, BUFFER_SIZE_U32};

use cpal::{
traits::{DeviceTrait, HostTrait, StreamTrait},
Expand Down Expand Up @@ -139,8 +139,7 @@ impl StreamConfigsBuilder {
///
/// * `options` - options contains latency hint information from which buffer size is derived
fn get_buffer_size(&self, options: Option<&AudioContextOptions>) -> u32 {
#[allow(clippy::cast_possible_truncation)]
let buffer_size = BUFFER_SIZE as u32;
let buffer_size = BUFFER_SIZE_U32;
let default_buffer_size = match self.supported.buffer_size() {
SupportedBufferSize::Range { min, .. } => buffer_size.max(*min),
SupportedBufferSize::Unknown => buffer_size,
Expand Down Expand Up @@ -457,8 +456,7 @@ pub fn build_input() -> (Stream, StreamConfig, Receiver<AudioBuffer>) {
let default_config: StreamConfig = supported_config.clone().into();

// determine best buffer size. Spec requires BUFFER_SIZE, but that might not be available
#[allow(clippy::cast_possible_truncation)]
let buffer_size = BUFFER_SIZE as u32;
let buffer_size = BUFFER_SIZE_U32;
let mut input_buffer_size = match supported_config.buffer_size() {
SupportedBufferSize::Range { min, .. } => buffer_size.max(*min),
SupportedBufferSize::Unknown => buffer_size,
Expand Down
8 changes: 7 additions & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,14 @@
//! //std::thread::sleep(std::time::Duration::from_secs(4));
//! ```

/// Render quantum size as u32 (audio graph is rendered in blocks of this size)
///
/// derive BUFFER_SIZE from this const to prevent downcasting from `usize` to `u32`
/// (cf. discussion [https://github.com/orottier/web-audio-api-rs/pull/52#issuecomment-969156219])
pub const BUFFER_SIZE_U32: u32 = 128;

/// Render quantum size (audio graph is rendered in blocks of this size)
pub const BUFFER_SIZE: usize = 128;
pub const BUFFER_SIZE: usize = BUFFER_SIZE_U32 as usize;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestions:

  • replace BUFFER_SIZE with BUFFER_SIZE_USIZE to make explicit that there is several BUFFER_SIZE_* variables, even when usize type is used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My (not really opinionated) rationale here was that as BUFFER_SIZE_U32 is finally only used in low-level io.rs, maybe it's good to reinforce that it is a kind of "special" const compared to BUFFER_SIZE (which should be preferred).

Copy link
Owner

Choose a reason for hiding this comment

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

I also prefer BUFFER_SIZE as a succinct name, conforming nicely to the spec naming.
If the U32 variant is only used in io.rs, let's move the const there so it will not pollute the pub namespace.
You could even use the usize variant in that file, and only cast at the final moment when passsing it to cpal. In that case we don't even need a const

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, used u32::try_from(u: usize) at last moment and clippy doesn't complain


/// Maximum number of channels for audio processing
pub const MAX_CHANNELS: usize = 32;
Expand Down