Skip to content

Remove input frames allocation#5

Merged
OpenSauce merged 5 commits intomainfrom
dont-allocate
May 23, 2025
Merged

Remove input frames allocation#5
OpenSauce merged 5 commits intomainfrom
dont-allocate

Conversation

@OpenSauce
Copy link
Owner

No description provided.

@OpenSauce OpenSauce requested a review from Copilot May 22, 2025 21:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the audio processor to reuse pre-allocated input buffers instead of allocating a new Vec each callback, reducing per-frame allocations.

  • Introduces input_frames field to hold reusable per-channel input buffers
  • Initializes input_frames in the constructor
  • Updates the processing loop to clear and refill input_frames rather than creating a new Vec
Comments suppressed due to low confidence (1)

src/io/processor.rs:20

  • [nitpick] The field name input_frames is ambiguous about its role as per-channel buffers; consider renaming to input_buffers or channel_input_buffers for clarity.
input_frames: Vec<Vec<f32>>,

out_l: Port<AudioOut>,
out_r: Port<AudioOut>,
upsampler: SincFixedIn<f32>,
downsampler: SincFixedIn<f32>,
Copy link

Copilot AI May 22, 2025

Choose a reason for hiding this comment

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

Add a doc comment explaining that this field holds reusable per-channel input buffers to avoid per-frame allocations.

Suggested change
downsampler: SincFixedIn<f32>,
downsampler: SincFixedIn<f32>,
/// Reusable per-channel input buffers to avoid per-frame allocations.

Copilot uses AI. Check for mistakes.
.unwrap();

let mut input_frames = Vec::with_capacity(channels);
input_frames.push(Vec::with_capacity(client.buffer_size() as usize));
Copy link

Copilot AI May 22, 2025

Choose a reason for hiding this comment

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

Only one buffer is pushed below, so input_frames will be empty for other channels and panic on access; initialize a Vec for each channel (e.g., using resize_with(channels, || Vec::with_capacity(buffer_size))).

Suggested change
input_frames.push(Vec::with_capacity(client.buffer_size() as usize));
input_frames.resize_with(channels, || Vec::with_capacity(client.buffer_size() as usize));

Copilot uses AI. Check for mistakes.
// Check for a new preset without blocking
if let Ok(new_chain) = self.rx_chain.try_recv() {
self.chain = new_chain;
println!("Received new chain");
Copy link

Copilot AI May 22, 2025

Choose a reason for hiding this comment

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

Use a structured logging macro (e.g., log::info!) instead of println! to integrate with the application's logging and control verbosity.

Copilot uses AI. Check for mistakes.
@OpenSauce OpenSauce requested a review from Copilot May 23, 2025 21:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Refactors the audio processing pipeline to eliminate per-callback frame allocations, standardize logging, and update dependency version specifications.

  • Processor now reuses an input buffer (input_frames) instead of allocating on each process call.
  • Switched from eprintln! to the log crate for error reporting and improved send error handling.
  • Updated Cargo.toml to use broader version ranges and removed some dependencies.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/sim/chain.rs Changed default amplifier channel index
src/main.rs Moved set_amp_chain call to after start()
src/io/processor.rs Introduced reusable input_frames, replaced vec allocs, and switched to log
src/io/manager.rs Improved error handling in set_amp_chain using unwrap_or_else
Cargo.toml Broadened dependency version constraints and added log
Comments suppressed due to low confidence (6)

src/sim/chain.rs:213

  • The default channel is labeled ‘Clean’ but channel 1 is ‘Rhythm’ (per define_channel). It should probably be set_channel(0) for the clean channel.
chain.set_channel(1);

src/main.rs:52

  • Setting the amplifier chain after start() may cause the audio thread to run one callback with the default chain. Consider moving this call before processor_manager.start() to ensure the new chain is applied immediately.
processor_manager.set_amp_chain(chain);

src/io/processor.rs:5

  • [nitpick] Importing the entire log module is unnecessary; you can call the logging macros (log::error!, etc.) without a use log; statement.
use log;

src/io/processor.rs:13

  • [nitpick] The field doc comment was shortened and is now less descriptive. Consider restoring a more explanatory comment such as 'The current mutable amplifier chain used by the audio thread.'
/// Amplifier.

src/io/manager.rs:41

  • [nitpick] Using eprintln! inside unwrap_or_else is inconsistent with the rest of the code’s logging strategy. Consider using log::error! for consistency and better output control.
tx.try_send(Box::new(new_chain)).unwrap_or_else(|e| {

Cargo.toml:7

  • You removed serde_json, serde_derive, and other dependencies without updating code references; this may break serialization or functionality elsewhere. Verify that all removed crates are truly unused.
jack = "0.13"

@OpenSauce OpenSauce merged commit be1c455 into main May 23, 2025
1 check passed
@OpenSauce OpenSauce deleted the dont-allocate branch December 6, 2025 20:26
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.

2 participants