Skip to content

Conversation

@UnknownSuperficialNight
Copy link
Contributor

This PR introduces an optional floor value for the AGC, enabling users to set a minimum output level that prevents the gain from dropping below a specified threshold.

For example, setting a floor value of 1.0 ensures that the audio output is always at or above the original source level, preventing excessive attenuation.

It also updates the example AGC to use the new defaults, I noticed forgot to do that in #759

Copy link
Member

@yara-blue yara-blue left a comment

Choose a reason for hiding this comment

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

Code nitpick, please have a look and let me know what you think.

@UnknownSuperficialNight
Copy link
Contributor Author

Code nitpick, please have a look and let me know what you think.

Should we also remove most of the docs for this and add links to the struct and the examples?

Since i don't think we need this anymore

/// ```rust
/// // Apply Automatic Gain Control to the source (AGC is on by default)
/// use rodio::source::{Source, SineWave};
/// use rodio::Sink;
/// let source = SineWave::new(444.0); // An example.
/// let (sink, output) = Sink::new(); // An example.
///
/// let agc_source = source.automatic_gain_control(1.0, 4.0, 0.0, 5.0);
///
/// // Add the AGC-controlled source to the sink
/// sink.append(agc_source);
///
/// ```

@roderickvd
Copy link
Member

roderickvd commented Oct 12, 2025

Nice to see your active maintenance on this one! As you're updating anyway, may I put in a couple of thoughts as well?

  1. Change the attack_time and release_time in the public-facing API with Duration - more idiomatic.
  2. It seems the attack_coeff and release_coeff calculation could reuse duration_to_coefficient from src/source/limit.rs - it should probably be moved to src/math.rs with pub(crate) visibility.

And three questions:

  1. How correct is the logic of min_attack_coeff? The name implies it contains a coefficient, but instead it contains a duration as float in seconds.
  2. What are the intended use cases for the recommended values? Maybe they're good for speech, I don't know, but I've never had success with them on music with large dynamic range and rather fast transients like classical.
  3. AutomaticGainControl is the only user of the experimental feature. Could we by now consider it non-experimental and remove the feature flag?

@yara-blue
Copy link
Member

yara-blue commented Oct 12, 2025

Should we also remove most of the docs for this and add links to the struct and the examples?

I would keep the docs on the source member and link from the struct actually. Since users will generally use AGC like this:

let stream = stream
    .possibly_disconnected_channels_to_mono()
    .constant_samplerate(SAMPLE_RATE)
    .limit(LimitSettings::live_performance())
    .process_buffer::<BUFFER_SIZE, _>(move |buffer| {
       <secret things> // not really look at zed/crates/audio/src/audio.rs if your curious
    })
    .denoise()
    .context("Could not set up denoiser")?
    .automatic_gain_control(0.90, 1.0, 0.0, 5.0)
    .periodic_access(Duration::from_millis(100), move |agc_source| {
        agc_source
            .set_enabled(LIVE_SETTINGS.auto_microphone_volume.load(Ordering::Relaxed));
        let denoise = agc_source.inner_mut();
        denoise.set_enabled(LIVE_SETTINGS.denoise.load(Ordering::Relaxed));
    });

@yara-blue
Copy link
Member

5. AutomaticGainControl is the only user of the experimental feature. Could we by now consider it non-experimental and remove the feature flag?

I really really do not want to expand rodio's API/patterns if we can. As I am actively working on rewriting/re-architecturing the audio pipeline. The larger our API the harder it well get to re-do it. Right now we only really support periodic access as parameter change method. If we stabilize the use of atomics/handles as is experimental here it will become far harder to make the changes I need to make.

Performance improvements:
- Cache atomic loads at start of `process_sample` to reduce redundant operations
- Change floor from `Option<f32>` to `f32`, unwrap once in setter instead of every sample
- Pass cached values as parameters to helper methods to avoid redundant atomic loads

Algorithm fixes:
- Fix `update_peak_level` to use instant attack `(0.0)` instead of `attack_coeff`
- Remove unused `min_attack_coeff` logic that was always 0.0 by default and caused errors when not

API improvements:
- Move `duration_to_coefficient` to `math.rs` with `pub(crate)` visibility for reuse
- Update all examples and benchmarks to use `AutomaticGainControlSettings::default()`
- Increase default `absolute_max_gain` from `5.0` to `7.0`

Documentation:
- Fix incorrect comments in update_peak_level, set_floor, and inner methods
- Clarify instant attack behavior in peak detection
@UnknownSuperficialNight
Copy link
Contributor Author

UnknownSuperficialNight commented Oct 12, 2025

Nice to see your active maintenance on this one

:)

Change the attack_time and release_time in the public-facing API with Duration - more idiomatic.

It seems the attack_coeff and release_coeff calculation could reuse duration_to_coefficient from src/source/limit.rs - it should probably be moved to src/math.rs with pub(crate) visibility.

I agree… Done.

How correct is the logic of min_attack_coeff? The name implies it contains a coefficient, but instead it contains a duration as float in seconds.

Not very correct I looked more into it, we don't need a min_attack_coeff we can just use 0.0 as it looks like I missed it during development but min_attack_coeff was always 0.0 by default or very close too.

We already have:

let attack_speed = if desired_gain > self.current_gain {
    attack_coeff
} else {
    release_coeff
};

To do this so peak doing the same thing for if sample_value > self.peak_level { is irrelevant so ill instead replace it with 0f32 to be as fast as possible. We still want release_coeff to be considered during the peak calculations to stop the peak from going down too rapidly.

What are the intended use cases for the recommended values? Maybe they're good for speech, I don't know, but I've never had success with them on music with large dynamic range and rather fast transients like classical.

Maybe we should work together on the defaults because for me, it's working fine, see attached video:

asdasdasd.mp4

But your issue with music with large dynamic range and rather fast transients like classical might have been due to update_peak_level accounting for attack_coeff when it shouldn't

AutomaticGainControl is the only user of the experimental feature. Could we by now consider it non-experimental and remove the feature flag?

@dvdsk responded to this, I don't feel that I have sufficient knowledge on the current state to provide valuable input on that.

Added some optimisations for AGC:

Increased default absolute_max_gain to 7.0 as this is the default i personally use (In my program) and there is no reason not too (if there is an issue with this let me know)

1. Cached Atomic Loads

  • What: Load target_level(), absolute_max_gain(), attack_coeff(), release_coeff() once at the start of process_sample()
  • Why: Each call performs an atomic load (with experimental feature), so calling them multiple times per sample was wasteful
  • Impact: Reduced atomic loads per sample to just 4 total

2. Optimized Floor Storage

  • What: Changed floor: Option<f32> to floor: f32
  • Why: Unwrap happens once in set_floor() instead of unwrap_or(0f32) on every single sample
  • Impact: Eliminates one branch + Option check per sample

3. Modified Helper Methods with Parameters

  • What: Helper methods now accept cached values as parameters:
    • update_peak_level(sample_value, release_coeff)
    • calculate_peak_gain(target_level, absolute_max_gain)
  • Why: Previously, update_peak_level() would call self.release_coeff() internally, causing another atomic load. Now it uses the cached parameter.
  • Impact: Prevents redundant atomic loads within helper methods

@yara-blue
Copy link
Member

Good to merge if you ask me! 🎉

@UnknownSuperficialNight UnknownSuperficialNight changed the title feat: Add optional floor value to AGC feat: Add optional floor value to AGC and performance optimizations Oct 12, 2025
@yara-blue yara-blue merged commit be453f2 into RustAudio:master Oct 12, 2025
9 checks passed
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.

3 participants