fix: close audio-gap on unchanged ReevaluateAll; reset compressor on enable
Two self-review follow-ups from the F1/F2 commits surfaced by an audio-correctness pass over `244367c..HEAD`. Both are low-risk, high-signal fixes — the kind that prevent users complaining about "weird little blips" when changing profiles or unmuting the compressor. ## Audio-gap on `PwCommand::ReevaluateAll` `enqueue_route` used to unconditionally drop `managed_route_links[node_id]` before the next 50 ms drain tick rebuilt. With `object.linger="false"` on the link-factory props, dropping the `Link` proxy destroys the actual graph link immediately. Result: every profile reload / route set / route unset / bypass toggle caused a 21–42 ms audio dropout on every already-correctly-routed stream — even when nothing about the stream's routing had actually changed. `managed_route_links` now carries the target sink name alongside the `Link` proxies (new `ManagedRoute` struct: `target_sink_name` + `links`). `enqueue_route` only drops when the target name differs from the stored one; the unchanged case leaves the live links intact, and `apply_pending_routes`' destroy/create loop sees its `want_set` already satisfied and exits as a no-op. Live verification: pw-cat /tmp/sine streaming through processed, issue `route set firefox bypass` (rule that doesn't touch pw-cat). Before this fix the link IDs would flip; after, link IDs 83 + 122 stayed identical across `reevaluating all known streams streams=1` in the daemon log. Listener-visible gap goes from one quantum to zero. The path that *does* change target (real bypass toggle, real-sink hot-swap, a rule edit that flipped the stream's decision) still drops + rebuilds — the gap there is unavoidable without a core-sync barrier or a "transition through both old and new links" choreography. That's acceptable: the user explicitly asked for the route change in those cases. ## Compressor envelope reset across `enabled` transition F6 made `compressor.enabled = false` actually skip processing, but didn't touch the envelope or RMS state — which kept ticking forward during enabled periods, sat stale during disabled periods, and then bled out via release on the first re-enable. With long release times this meant up to ~100 ms of artificial gain reduction after switching from a `transparent` profile back to a compressing one, for no acoustic reason. `Compressor::set_config` now detects the `disabled → enabled` transition and resets `envelope_db`, `rms_state`, and `last_gr_db` so the compressor starts from a clean state — same behaviour as a freshly-constructed `Compressor::new(...)`. Same-enabled transitions (parameter tweaks while enabled, or no-op `set_config` while disabled) leave the envelope alone, so live tweaks still don't pop. Regression test `compressor::tests::enable_transition_resets_stale_envelope` winds the envelope hot, toggles disable+enable via two `set_config` calls, then asserts the next quiet sample produces zero GR. Without the reset that assertion would fail by ~5+ dB. ## Verified 190 tests pass (+1 for the envelope reset; +0 for the link fix — exercised by live-smoke since it's about side-effect timing not value); clippy clean at `-D warnings --all-targets`.
This commit is contained in:
atagen
parent
5143c07c99
commit
5c769a1226
2 changed files with 111 additions and 10 deletions
|
|
@ -127,6 +127,18 @@ struct PendingRoute {
|
|||
route: Route,
|
||||
}
|
||||
|
||||
/// The daemon-owned routing links for one stream, paired with the
|
||||
/// target sink name they were built for. Stored in
|
||||
/// `RoutingState::managed_route_links` so the bypass / profile
|
||||
/// re-evaluation paths can ask "is the stream already routed to
|
||||
/// this sink?" without losing the existing live links to find out
|
||||
/// — keeping them alive avoids the audio gap the destroy+rebuild
|
||||
/// cycle otherwise introduces.
|
||||
struct ManagedRoute {
|
||||
target_sink_name: String,
|
||||
links: Vec<Link>,
|
||||
}
|
||||
|
||||
/// Subject id passed to `set_property` for keys that aren't bound to
|
||||
/// a specific node (system-wide settings like `default.audio.sink`).
|
||||
const METADATA_SUBJECT_GLOBAL: u32 = 0;
|
||||
|
|
@ -192,8 +204,14 @@ pub struct RoutingState {
|
|||
pending_routes: HashMap<u32, PendingRoute>,
|
||||
/// Explicit `link-factory` `Link` proxies owned by the daemon,
|
||||
/// keyed by source stream node id. Kept alive so the links
|
||||
/// persist; dropped on stream removal or route change.
|
||||
managed_route_links: HashMap<u32, Vec<Link>>,
|
||||
/// persist; dropped on stream removal or *target-changing*
|
||||
/// route change. The accompanying `target_sink_name` lets
|
||||
/// `enqueue_route` skip the drop when a re-evaluation arrives
|
||||
/// at the same target (the common case for profile.reload
|
||||
/// / route.unset on an unaffected stream) — avoiding the
|
||||
/// 21-42 ms audio gap that an unconditional drop+rebuild
|
||||
/// would cause.
|
||||
managed_route_links: HashMap<u32, ManagedRoute>,
|
||||
/// Window-based limit for `default.audio.sink` re-assertions
|
||||
/// so a hostile WP policy can't pull us into a hot loop.
|
||||
/// `(window_start, attempts_in_window)`. See
|
||||
|
|
@ -1023,11 +1041,25 @@ impl RoutingState {
|
|||
app_label: String,
|
||||
route: Route,
|
||||
) {
|
||||
// Replacing intent: drop any old managed links for this
|
||||
// stream so apply_pending_routes can rebuild against the new
|
||||
// target. Dropping the proxies destroys the links via
|
||||
// `object.linger = "false"`.
|
||||
self.managed_route_links.remove(&node_id);
|
||||
// If we already have live links for this stream pointing
|
||||
// at the *same* sink, leave them alone. `apply_pending_routes`
|
||||
// will run its idempotent destroy/create pass on the next
|
||||
// drain tick and find the want_set already satisfied — a
|
||||
// no-op. This is the common case for profile.reload /
|
||||
// route.set / route.unset / profile.use when the change
|
||||
// doesn't move *this* stream, and skipping the drop avoids
|
||||
// the 21-42 ms audio gap an unconditional rebuild would
|
||||
// cost. When the target *did* change (bypass toggle,
|
||||
// real-sink hot-swap, or rule edit that flipped this
|
||||
// stream's decision), drop the proxies so the old links
|
||||
// tear down before the new ones come up.
|
||||
let already_at_target = self
|
||||
.managed_route_links
|
||||
.get(&node_id)
|
||||
.is_some_and(|m| m.target_sink_name == target_sink_name);
|
||||
if !already_at_target {
|
||||
self.managed_route_links.remove(&node_id);
|
||||
}
|
||||
self.pending_routes.insert(
|
||||
node_id,
|
||||
PendingRoute {
|
||||
|
|
@ -1156,6 +1188,7 @@ impl RoutingState {
|
|||
let mut created: Vec<Link> = self
|
||||
.managed_route_links
|
||||
.remove(&node_id)
|
||||
.map(|m| m.links)
|
||||
.unwrap_or_default();
|
||||
let mut all_ok = true;
|
||||
for (out_port, in_port) in &want {
|
||||
|
|
@ -1179,7 +1212,13 @@ impl RoutingState {
|
|||
}
|
||||
}
|
||||
if !created.is_empty() {
|
||||
self.managed_route_links.insert(node_id, created);
|
||||
self.managed_route_links.insert(
|
||||
node_id,
|
||||
ManagedRoute {
|
||||
target_sink_name: intent.target_sink_name.clone(),
|
||||
links: created,
|
||||
},
|
||||
);
|
||||
}
|
||||
if all_ok {
|
||||
tracing::info!(
|
||||
|
|
|
|||
|
|
@ -117,11 +117,23 @@ impl Compressor {
|
|||
self.last_gr_db
|
||||
}
|
||||
|
||||
/// Update parameters. Recomputes alphas. Envelope state is kept,
|
||||
/// so live tweaks don't pop.
|
||||
/// Update parameters. Recomputes alphas. Envelope state is kept
|
||||
/// across same-enabled transitions so live tweaks don't pop, but
|
||||
/// reset on a `disabled → enabled` transition so a stale
|
||||
/// envelope from before the disable doesn't bleed out at the
|
||||
/// release time-constant when processing resumes (otherwise
|
||||
/// switching from a `transparent` profile back to a compressing
|
||||
/// one would briefly duck on the first ~100 ms of audio for no
|
||||
/// reason).
|
||||
pub fn set_config(&mut self, cfg: CompressorConfig) {
|
||||
let cfg = cfg.sanitized();
|
||||
let was_disabled = !self.cfg.enabled;
|
||||
self.cfg = cfg;
|
||||
if was_disabled && self.cfg.enabled {
|
||||
self.envelope_db = -200.0;
|
||||
self.rms_state = 0.0;
|
||||
self.last_gr_db = 0.0;
|
||||
}
|
||||
self.attack_alpha = time_to_alpha(cfg.attack_ms, self.sample_rate);
|
||||
self.release_alpha = time_to_alpha(cfg.release_ms, self.sample_rate);
|
||||
self.rms_alpha = time_to_alpha(cfg.rms_window_ms, self.sample_rate);
|
||||
|
|
@ -295,6 +307,56 @@ mod tests {
|
|||
assert_eq!(c.gain_reduction_db(), 0.0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn enable_transition_resets_stale_envelope() {
|
||||
// Run a loud signal through an enabled compressor to wind
|
||||
// the envelope up, then disable + re-enable via set_config.
|
||||
// The first sample after re-enable must NOT see the stale
|
||||
// envelope (which would otherwise duck the signal until
|
||||
// release_ms wound it down). Concretely: with a quiet input
|
||||
// after re-enable, the envelope should be at the floor, so
|
||||
// GR is zero — same as a freshly-constructed compressor.
|
||||
let loud_cfg = CompressorConfig {
|
||||
enabled: true,
|
||||
threshold_db: -20.0,
|
||||
ratio: 4.0,
|
||||
attack_ms: 0.1,
|
||||
release_ms: 1000.0, // slow release so stale state would otherwise stick
|
||||
knee_db: 0.0,
|
||||
makeup_db: Some(0.0),
|
||||
..CompressorConfig::default()
|
||||
};
|
||||
let mut c = Compressor::new(loud_cfg, 48_000.0);
|
||||
// Drive hot signal to wind envelope up.
|
||||
for _ in 0..2_000 {
|
||||
c.process_frame(0.5, 0.5);
|
||||
}
|
||||
assert!(
|
||||
c.gain_reduction_db() < -5.0,
|
||||
"precondition: envelope should be wound up; gr={}",
|
||||
c.gain_reduction_db()
|
||||
);
|
||||
|
||||
// Disable, then re-enable — should reset.
|
||||
let mut disabled_cfg = loud_cfg;
|
||||
disabled_cfg.enabled = false;
|
||||
c.set_config(disabled_cfg);
|
||||
c.set_config(loud_cfg);
|
||||
|
||||
// Now drive a quiet signal. With reset envelope, GR should
|
||||
// ride near zero; without reset, the stale envelope would
|
||||
// bleed gain reduction out over ~release_ms.
|
||||
let (l, r) = c.process_frame(0.001, 0.001);
|
||||
assert!(
|
||||
c.gain_reduction_db().abs() < 0.01,
|
||||
"envelope didn't reset across enable transition; gr={}",
|
||||
c.gain_reduction_db()
|
||||
);
|
||||
// Output should be quiet (within makeup-applied scale).
|
||||
assert!(l.abs() < 0.01);
|
||||
assert!(r.abs() < 0.01);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn static_curve_at_threshold_with_soft_knee() {
|
||||
// At exactly threshold, soft knee contributes exactly half the
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue