First pass of review comments#105

Draft

atlv24 wants to merge 1 commit into

BillyDM:main from

atlv24:ad/review

Draft

First pass of review comments #105
atlv24 wants to merge 1 commit into
BillyDM:main from
atlv24:ad/review

Conversation

@atlv24

@atlv24 atlv24 commented Feb 19, 2026

Copy link

Copy Markdown

Contributor

As i can't easily leave comments on already merged code, and I am auditing the entire codebase, the easiest way to leave notes is as comments. I do not intend for this PR to merge; it is just reference. I will be putting up PRs fixing the things ive noted here once the other two PRs i have open merge: #103 #104

@atlv24


 first pass of review comments

28b1dc4

BillyDM

BillyDM reviewed

Feb 19, 2026

View reviewed changes

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

// 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

@BillyDM BillyDM Feb 19, 2026

Copy link

Copy Markdown

Owner

Choose a reason for hiding this comment

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

Oh interesting! I might have to try that.

atlv24

atlv24 commented

Feb 19, 2026

View reviewed changes

Cargo.toml

Comment on lines +228 to +231

# 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.

@atlv24 atlv24 Feb 19, 2026

Copy link

Copy Markdown

Contributor Author

Choose a reason for hiding this comment

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

note that i wrote this on the plane before discussion after posting my writeup, this is still one of the possible plans but is not my #1 anymore

BillyDM

BillyDM reviewed

Feb 19, 2026

View reviewed changes

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

&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

@BillyDM BillyDM Feb 19, 2026

Copy link

Copy Markdown

Owner

Choose a reason for hiding this comment

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

Oh yeah, that would be a handy feature to optimize microphone inputs. I'll add that!

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

}

// TODO: method duplication

@BillyDM BillyDM Feb 19, 2026

Copy link

Copy Markdown

Owner

Choose a reason for hiding this comment

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

The reason for the duplication is to have optimized auto-vectorized loops for the most common use cases. Though I suppose those optimized loops could all be in a single function.

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

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

@BillyDM BillyDM Feb 19, 2026

Copy link

Copy Markdown

Owner

Choose a reason for hiding this comment

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

It appears to auto-vectorize https://rust.godbolt.org/z/xGen4r59a

crates/firewheel-core/src/log.rs

pub fn try_debug(&mut self, message: &str) -> Result<(), RealtimeLogError> {

#[cfg(debug_assertions)]

{

// TODO: code duplication

@BillyDM BillyDM Feb 19, 2026

Copy link

Copy Markdown

Owner

Choose a reason for hiding this comment

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

What is the duplication here?

crates/firewheel-core/src/node.rs

/// 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.

@BillyDM BillyDM Feb 19, 2026

Copy link

Copy Markdown

Owner

Choose a reason for hiding this comment

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

Oh, by "junk data" I just meant that it can contain data from previous process loops (this is a very common thing in the audio processing world, clearing the buffers for every node would be a lot of extra processing overhead). The buffers are all initialized to zero before being sent to the audio thread, so there is no safety concern here. I'll make that clearer in the docs.

Cargo.toml

]

# 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

@BillyDM BillyDM Feb 19, 2026

Copy link

Copy Markdown

Owner

Choose a reason for hiding this comment

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

I agree

Cargo.toml

]

[workspace.dependencies]

# if we upstream firewheel to bevy, these should become deps on bevy_log

@BillyDM BillyDM Feb 19, 2026

Copy link

Copy Markdown

Owner

Choose a reason for hiding this comment

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

I agree

README.md

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

@BillyDM BillyDM Feb 19, 2026

Copy link

Copy Markdown

Owner

Choose a reason for hiding this comment

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

Agreed, I should change that.

crates/firewheel-cpal/src/lib.rs

// TODO: PLEASE FIX ME:

// TODO: investigate CPAL

@BillyDM BillyDM Feb 19, 2026

Copy link

Copy Markdown

Owner

Choose a reason for hiding this comment

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

Yeah, I'm at a bit of a loss on this one. Might have to make an issue in the CPAL repository.

Cargo.toml

# 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.

@BillyDM BillyDM Feb 19, 2026 •

edited

Loading

Copy link

Copy Markdown

Owner

Choose a reason for hiding this comment

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

We could also consider merging the crates to as few as possible. The only one that really needs to be separate is firewheel-cpal (assuming we keep the audio backend abstraction).

firewheel-core was originally meant to be like a "stable node API" that third party node developers would base on. But if we are planning to upstream Firewheel to Bevy anyway, then I suppose it doesn't make sense to have this stability guarantee anymore.

Labels

None yet

2 participants

@atlv24 @BillyDM

Uh oh!

Conversation

@atlv24 atlv24 commented Feb 19, 2026

Uh oh!

@BillyDM BillyDM Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

@atlv24 atlv24 Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

@BillyDM BillyDM Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

@BillyDM BillyDM Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

@BillyDM BillyDM Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

@BillyDM BillyDM Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

@BillyDM BillyDM Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

@BillyDM BillyDM Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

@BillyDM BillyDM Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

@BillyDM BillyDM Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

@BillyDM BillyDM Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

@BillyDM BillyDM Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

@BillyDM BillyDM Feb 19, 2026 •

edited

Loading