From 28b1dc478f6008bd7395e3638b673246f13ac7c3 Mon Sep 17 00:00:00 2001 From: atlas Date: Thu, 19 Feb 2026 05:51:36 -0500 Subject: [PATCH] first pass of review comments --- Cargo.toml | 14 ++++++++++++++ README.md | 2 ++ assets/logo.svg | 1 + .../firewheel-core/src/dsp/distance_attenuation.rs | 1 + crates/firewheel-core/src/dsp/interleave.rs | 3 +++ crates/firewheel-core/src/dsp/mix.rs | 1 + crates/firewheel-core/src/dsp/volume.rs | 1 + crates/firewheel-core/src/log.rs | 1 + crates/firewheel-core/src/node.rs | 1 + crates/firewheel-cpal/src/lib.rs | 2 ++ .../firewheel-graph/src/graph/compiler/schedule.rs | 2 ++ crates/firewheel-nodes/src/convolution.rs | 1 + .../firewheel-nodes/src/fast_filters/bandpass.rs | 1 + .../firewheel-nodes/src/fast_filters/highpass.rs | 1 + crates/firewheel-nodes/src/fast_filters/lowpass.rs | 1 + crates/firewheel-nodes/src/freeverb/mod.rs | 1 + crates/firewheel-nodes/src/svf.rs | 10 ++++++++++ 17 files changed, 44 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 9dd0d598..d6118662 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -137,6 +137,8 @@ bevy_reflect = [ ] # Enables the wasm-bindgen feature for the CPAL backend wasm-bindgen = ["firewheel-cpal/wasm-bindgen"] +# NOTE: these glam deps will also be maintenance burden, in an upstream future we should +# only use the bevy glam version and not make it optional. # Enables `glam::Vec2` and `glam::Vec3` parameter derives for glam 0.29. glam-29 = ["firewheel-core/glam-29"] # Enables `glam::Vec2` and `glam::Vec3` parameter derives for glam 0.30. @@ -198,6 +200,7 @@ members = [ ] [workspace.dependencies] +# if we upstream firewheel to bevy, these should become deps on bevy_log tracing = "0.1" tracing-subscriber = "0.3" log = "0.4" @@ -216,6 +219,17 @@ firewheel-macros = { path = "crates/firewheel-macros", version = "0.10.0" } bevy_platform = { version = "0.18", default-features = false, features = [ "alloc", ] } +# NOTE: depending on bevy_ecs means upstreaming bevy_seedling requires upstreaming firewheel itself, +# because otherwise publishing a release becomes impossible: seedling depends on firewheel with bevy +# feature enabled, which effectively creates a dep cycle. Assuming we want to upstream firewheel to +# bevy, the problem now becomes how to deal with the crate structure, since there's 8 crates. +# The alternative is to make bevy_ecs depend on firewheel and move the Component impls to bevy_ecs, +# but that is a non-starter. +# My proposal for the path forward is to have a bevy_firewheel crate in bevy/crates, which houses a copy +# of the firewheel top level crate and all firewheel subcrates (with names unchanged), as well as the +# original top level firewheel crate. bevy_firewheel would bring in all the subcrates with bevy integration +# enabled, and the original top level firewheel crate will be the way to use firewheel without bevy. +# Versioning will become lockstep with bevy versions for all firewheel crates. bevy_ecs = { version = "0.18", default-features = false } bevy_reflect = { version = "0.18", default-features = false } num-traits = { version = "0.2", default-features = false } diff --git a/README.md b/README.md index a9f6792a..af79538a 100644 --- a/README.md +++ b/README.md @@ -7,6 +7,8 @@ A mid-level open source audio graph engine for games and other applications, written in Rust. This crate can be used as-is or as a base for other higher-level audio engines. (Think of it like [wgpu](https://wgpu.rs/) but for audio). +NOTE: I disagree that this is like wgpu for audio. This is a step higher than wgpu, since it provides DSP infrastructure. This is more akin +to bevy_render, and cpal is more like wgpu in this analogy. ALSA/JACK/WASAPI/ASIO/CoreAudio, etc are equivalent to Vulkan/Metal/DirectX/OpenGL/WebGPU etc. ## Key Features diff --git a/assets/logo.svg b/assets/logo.svg index c790c8b7..7d3cc95a 100644 --- a/assets/logo.svg +++ b/assets/logo.svg @@ -1,5 +1,6 @@ + >( &mut channels[0].as_mut()[start_frame_in_channels..start_frame_in_channels + samples]; ch.copy_from_slice(interleaved); + // TODO: should this silence check use epsilon if calculate_silence_mask { if ch.iter().find(|&&s| s != 0.0).is_none() { @@ -56,6 +57,7 @@ pub fn deinterleave>( } if calculate_silence_mask { + // TODO: should this silence check use epsilon for (ch_i, ch) in channels.iter_mut().enumerate() { if ch.as_mut()[0..samples] .iter() @@ -81,6 +83,7 @@ pub fn deinterleave>( { *out_s = in_chunk[ch_i]; } + // TODO: should this silence check use epsilon if calculate_silence_mask && ch_i < 64 { if ch.iter().find(|&&s| s != 0.0).is_none() { diff --git a/crates/firewheel-core/src/dsp/mix.rs b/crates/firewheel-core/src/dsp/mix.rs index bc4d43a4..774f8c9b 100644 --- a/crates/firewheel-core/src/dsp/mix.rs +++ b/crates/firewheel-core/src/dsp/mix.rs @@ -218,6 +218,7 @@ impl MixDSP { } } + // TODO: method duplication pub fn mix_first_into_second_stereo( &mut self, first_l: &[f32], diff --git a/crates/firewheel-core/src/dsp/volume.rs b/crates/firewheel-core/src/dsp/volume.rs index ea64c979..22dd1c1d 100644 --- a/crates/firewheel-core/src/dsp/volume.rs +++ b/crates/firewheel-core/src/dsp/volume.rs @@ -353,6 +353,7 @@ impl Default for DbMeterNormalizer { pub fn is_buffer_silent(buffer: &[f32], amp_epsilon: f32) -> bool { let mut silent = true; for &s in buffer.iter() { + // TODO: check if this autovectorizes if s.abs() > amp_epsilon { silent = false; break; diff --git a/crates/firewheel-core/src/log.rs b/crates/firewheel-core/src/log.rs index 77608c76..4e39c8c9 100644 --- a/crates/firewheel-core/src/log.rs +++ b/crates/firewheel-core/src/log.rs @@ -137,6 +137,7 @@ impl RealtimeLogger { pub fn try_debug(&mut self, message: &str) -> Result<(), RealtimeLogError> { #[cfg(debug_assertions)] { + // TODO: code duplication if message.len() > self.max_msg_length { self.shared_state .message_too_long_occured diff --git a/crates/firewheel-core/src/node.rs b/crates/firewheel-core/src/node.rs index f144293c..bdd6a729 100644 --- a/crates/firewheel-core/src/node.rs +++ b/crates/firewheel-core/src/node.rs @@ -505,6 +505,7 @@ pub struct ProcBuffers<'a, 'b> { /// Each channel slice will have a length of [`ProcInfo::frames`]. /// /// These buffers may contain junk data. + // TODO: This is UB; having references to uninitialized data is not allowed. Use MaybeUninit or raw pointers. pub outputs: &'a mut [&'b mut [f32]], } diff --git a/crates/firewheel-cpal/src/lib.rs b/crates/firewheel-cpal/src/lib.rs index 46e4c40d..5ed8c214 100644 --- a/crates/firewheel-cpal/src/lib.rs +++ b/crates/firewheel-cpal/src/lib.rs @@ -835,6 +835,7 @@ struct DataCallback { processor: Option>, sample_rate: u32, sample_rate_recip: f64, + // Why are these commented? //_first_internal_clock_instant: Option, //_prev_stream_instant: Option, predicted_delta_time: Duration, @@ -917,6 +918,7 @@ impl DataCallback { // TODO: PLEASE FIX ME: // + // TODO: investigate CPAL // It appears that for some reason, both Windows and Linux will sometimes return a timestamp which // has a value less than the previous timestamp. I am unsure if this is a bug with the APIs, a bug // with CPAL, or I'm just misunderstaning how the timestamps are supposed to be used. Either way, diff --git a/crates/firewheel-graph/src/graph/compiler/schedule.rs b/crates/firewheel-graph/src/graph/compiler/schedule.rs index a87c945f..1209a554 100644 --- a/crates/firewheel-graph/src/graph/compiler/schedule.rs +++ b/crates/firewheel-graph/src/graph/compiler/schedule.rs @@ -660,7 +660,9 @@ fn sum_inputs( .set_silent(all_buffers_silent, frames as u16); } +// TODO: this must be marked as unsafe, and invariants justified at every callsite. #[inline] +// TODO: this should use an UnsafeCell type for interior mutability. This allow is not acceptable #[allow(clippy::mut_from_ref)] fn buffer_slice_mut<'a>( buffers: &'a [f32], diff --git a/crates/firewheel-nodes/src/convolution.rs b/crates/firewheel-nodes/src/convolution.rs index c499b476..6d837a19 100644 --- a/crates/firewheel-nodes/src/convolution.rs +++ b/crates/firewheel-nodes/src/convolution.rs @@ -342,6 +342,7 @@ impl AudioNodeProcessor for ConvolutionProcessor AudioNodeProcessor for Processor { // this can be used to only recalculate them every few frames. // // TODO: use core::hint::cold_path() once that stabilizes + // TODO: instead, we can extract a function and use #[cold] + #[inline(always)] today // // TODO: Alternatively, this could be optimized using a lookup table if self.coeff_update_mask.do_update(i) { diff --git a/crates/firewheel-nodes/src/fast_filters/highpass.rs b/crates/firewheel-nodes/src/fast_filters/highpass.rs index c96b1ef5..5f0ddbfa 100644 --- a/crates/firewheel-nodes/src/fast_filters/highpass.rs +++ b/crates/firewheel-nodes/src/fast_filters/highpass.rs @@ -195,6 +195,7 @@ impl AudioNodeProcessor for Processor { // this can be used to only recalculate them every few frames. // // TODO: use core::hint::cold_path() once that stabilizes + // TODO: instead, we can extract a function and use #[cold] + #[inline(always)] today // // TODO: Alternatively, this could be optimized using a lookup table if self.coeff_update_mask.do_update(i) { diff --git a/crates/firewheel-nodes/src/fast_filters/lowpass.rs b/crates/firewheel-nodes/src/fast_filters/lowpass.rs index 59263382..05014765 100644 --- a/crates/firewheel-nodes/src/fast_filters/lowpass.rs +++ b/crates/firewheel-nodes/src/fast_filters/lowpass.rs @@ -194,6 +194,7 @@ impl AudioNodeProcessor for Processor { // this can be used to only recalculate them every few frames. // // TODO: use core::hint::cold_path() once that stabilizes + // TODO: instead, we can extract a function and use #[cold] + #[inline(always)] today // // TODO: Alternatively, this could be optimized using a lookup table if self.coeff_update_mask.do_update(i) { diff --git a/crates/firewheel-nodes/src/freeverb/mod.rs b/crates/firewheel-nodes/src/freeverb/mod.rs index d2ae970f..99790bc8 100644 --- a/crates/firewheel-nodes/src/freeverb/mod.rs +++ b/crates/firewheel-nodes/src/freeverb/mod.rs @@ -254,6 +254,7 @@ impl AudioNodeProcessor for FreeverbProcessor { // the threshold for "completely silent" // threshold chosen by ear + // TODO: double check this let threshold = 0.0001; if matches!( buffers.check_for_silence_on_outputs(threshold), diff --git a/crates/firewheel-nodes/src/svf.rs b/crates/firewheel-nodes/src/svf.rs index 78519144..eb884183 100644 --- a/crates/firewheel-nodes/src/svf.rs +++ b/crates/firewheel-nodes/src/svf.rs @@ -678,6 +678,7 @@ impl Processor { // this can be used to only recalculate them every few frames. // // TODO: use core::hint::cold_path() once that stabilizes + // TODO: instead, we can extract a function and use #[cold] + #[inline(always)] today // // TODO: Alternatively, this could be optimized using a lookup table if self.coeff_update_mask.do_update(i) { @@ -727,6 +728,7 @@ impl Processor { // this can be used to only recalculate them every few frames. // // TODO: use core::hint::cold_path() once that stabilizes + // TODO: instead, we can extract a function and use #[cold] + #[inline(always)] today // // TODO: Alternatively, this could be optimized using a lookup table if self.coeff_update_mask.do_update(i) { @@ -776,6 +778,7 @@ impl Processor { // this can be used to only recalculate them every few frames. // // TODO: use core::hint::cold_path() once that stabilizes + // TODO: instead, we can extract a function and use #[cold] + #[inline(always)] today // // TODO: Alternatively, this could be optimized using a lookup table if self.coeff_update_mask.do_update(i) { @@ -825,6 +828,7 @@ impl Processor { // this can be used to only recalculate them every few frames. // // TODO: use core::hint::cold_path() once that stabilizes + // TODO: instead, we can extract a function and use #[cold] + #[inline(always)] today // // TODO: Alternatively, this could be optimized using a lookup table if self.coeff_update_mask.do_update(i) { @@ -869,6 +873,7 @@ impl Processor { // this can be used to only recalculate them every few frames. // // TODO: use core::hint::cold_path() once that stabilizes + // TODO: instead, we can extract a function and use #[cold] + #[inline(always)] today // // TODO: Alternatively, this could be optimized using a lookup table if self.coeff_update_mask.do_update(i) { @@ -920,6 +925,7 @@ impl Processor { // this can be used to only recalculate them every few frames. // // TODO: use core::hint::cold_path() once that stabilizes + // TODO: instead, we can extract a function and use #[cold] + #[inline(always)] today // // TODO: Alternatively, this could be optimized using a lookup table if self.coeff_update_mask.do_update(i) { @@ -966,6 +972,7 @@ impl Processor { // this can be used to only recalculate them every few frames. // // TODO: use core::hint::cold_path() once that stabilizes + // TODO: instead, we can extract a function and use #[cold] + #[inline(always)] today // // TODO: Alternatively, this could be optimized using a lookup table if self.coeff_update_mask.do_update(i) { @@ -1012,6 +1019,7 @@ impl Processor { // this can be used to only recalculate them every few frames. // // TODO: use core::hint::cold_path() once that stabilizes + // TODO: instead, we can extract a function and use #[cold] + #[inline(always)] today // // TODO: Alternatively, this could be optimized using a lookup table if self.coeff_update_mask.do_update(i) { @@ -1057,6 +1065,7 @@ impl Processor { // this can be used to only recalculate them every few frames. // // TODO: use core::hint::cold_path() once that stabilizes + // TODO: instead, we can extract a function and use #[cold] + #[inline(always)] today // // TODO: Alternatively, this could be optimized using a lookup table if self.coeff_update_mask.do_update(i) { @@ -1101,6 +1110,7 @@ impl Processor { // this can be used to only recalculate them every few frames. // // TODO: use core::hint::cold_path() once that stabilizes + // TODO: instead, we can extract a function and use #[cold] + #[inline(always)] today // // TODO: Alternatively, this could be optimized using a lookup table if self.coeff_update_mask.do_update(i) {