First pass of review comments

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 }

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
 

crates/firewheel-core/src/dsp/distance_attenuation.rs

@@ -361,6 +361,7 @@ impl DistanceAttenuatorStereoDsp {
                     // 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) {

crates/firewheel-core/src/dsp/interleave.rs

@@ -30,6 +30,7 @@ pub fn deinterleave<V: AsMut<[f32]>>(
             &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<V: AsMut<[f32]>>(
         }
 
         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<V: AsMut<[f32]>>(
             {
                 *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() {

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],

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;

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

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]],
 }
 

crates/firewheel-cpal/src/lib.rs

@@ -835,6 +835,7 @@ struct DataCallback {
     processor: Option<FirewheelProcessor<CpalBackend>>,
     sample_rate: u32,
     sample_rate_recip: f64,
+    // Why are these commented?
     //_first_internal_clock_instant: Option<cpal::StreamInstant>,
     //_prev_stream_instant: Option<cpal::StreamInstant>,
     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,

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],

crates/firewheel-nodes/src/convolution.rs

@@ -342,6 +342,7 @@ impl<const CHANNELS: usize> AudioNodeProcessor for ConvolutionProcessor<CHANNELS
             DeclickFadeCurve::EqualPower3dB,
         );
 
+        // TODO: double check this epsilon choice
         buffers.check_for_silence_on_outputs(f32::EPSILON)
     }
 }

crates/firewheel-nodes/src/fast_filters/bandpass.rs

@@ -206,6 +206,7 @@ impl<const CHANNELS: usize> AudioNodeProcessor for Processor<CHANNELS> {
                 // 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) {

crates/firewheel-nodes/src/fast_filters/highpass.rs

@@ -195,6 +195,7 @@ impl<const CHANNELS: usize> AudioNodeProcessor for Processor<CHANNELS> {
                 // 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) {

crates/firewheel-nodes/src/fast_filters/lowpass.rs

@@ -194,6 +194,7 @@ impl<const CHANNELS: usize> AudioNodeProcessor for Processor<CHANNELS> {
                 // 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) {

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),

crates/firewheel-nodes/src/svf.rs

@@ -678,6 +678,7 @@ impl<const CHANNELS: usize> Processor<CHANNELS> {
             // 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<const CHANNELS: usize> Processor<CHANNELS> {
             // 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<const CHANNELS: usize> Processor<CHANNELS> {
             // 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<const CHANNELS: usize> Processor<CHANNELS> {
             // 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<const CHANNELS: usize> Processor<CHANNELS> {
             // 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<const CHANNELS: usize> Processor<CHANNELS> {
             // 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<const CHANNELS: usize> Processor<CHANNELS> {
             // 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<const CHANNELS: usize> Processor<CHANNELS> {
             // 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<const CHANNELS: usize> Processor<CHANNELS> {
             // 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<const CHANNELS: usize> Processor<CHANNELS> {
             // 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) {