Skip to content

Commit a5ec3f3

Browse files
srperensPer Enstedtclaude
authored
feat(audio-mixer): honor ramp_ms on mute toggles, with cancel-guard (#540)
Mute toggles previously hard-coded MUTE_ANTICLICK_RAMP_MS (10 ms), so external callers (Open Live, MCP) sending PATCH `.../mute` could not request the broadcast-style 500 ms route fade they need for on-air / off-air transitions. The volume path already honored `ramp_ms`; this brings parity to mute. Backend: - properties.rs: mute branch passes `ramp_ms.unwrap_or(MUTE_ANTICLICK_RAMP_MS)` into apply_mute, matching the volume branch. - volume_ramp.rs: rename anticlick_ms -> ramp_ms in apply_mute. Add a per-element generation counter (Arc<Mutex<HashMap<String, u64>>>) so the tokio-scheduled `set_property("mute", true)` aborts on fire if a later apply_mute call has bumped the generation. Without this, a fast unmute mid-fade would still let the stale mute=true land afterwards and silently kill the route. clear() also bumps every generation so a stopped pipeline cannot be re-muted by leftover scheduled toggles. API: - UpdatePropertyRequest.ramp_ms doc now covers `mute` and points at the 500 ms broadcast use case. OpenAPI snapshot regenerated (also picked up the pending 0.4.12 -> 0.4.13-dev version bump). Tests: - long_mute_ramp_takes_full_duration: 200 ms fade-out must keep audible level mid-ramp and only set mute=true after the full duration + grace. - unmute_during_long_mute_fade_cancels_pending_toggle: regression for the cancel-guard — an unmute 40 ms into a 200 ms fade must prevent the stale mute=true from landing 200 ms later. - All existing volume_ramp + pipeline_lifecycle tests still green. Usage example: `PATCH .../elements/<flow>:mixer:to_main_vol_<ch>` with `{ property_name: "mute", value: true, ramp_ms: 500 }` now produces a 500 ms dB-linear fade-out before mute=true lands; the matching `value: false, ramp_ms: 500` produces a 0->pre_mute fade-in. Co-authored-by: Per Enstedt <per.enstedt@svt.se> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 145534a commit a5ec3f3

5 files changed

Lines changed: 124 additions & 20 deletions

File tree

backend/src/gst/pipeline/properties.rs

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,11 @@ impl PipelineManager {
1212
/// Set a property on an element.
1313
///
1414
/// `ramp_ms` is consulted only for routes that support smooth interpolation
15-
/// (currently audio `volume`-element `volume`/`mute`). Other properties are
16-
/// set immediately regardless. `None` selects the per-route default ramp.
15+
/// (currently audio `volume`-element `volume` and `mute`). Other properties
16+
/// are set immediately regardless. `None` selects the per-route default
17+
/// ramp (short anti-zipper for `volume`, short anti-click for `mute`); a
18+
/// caller can request a longer broadcast-style fade by passing an explicit
19+
/// duration (e.g. 500 ms for a route mute on-air/off-air).
1720
pub(super) fn set_property(
1821
&self,
1922
element: &gst::Element,
@@ -50,7 +53,7 @@ impl PipelineManager {
5053
element,
5154
element_id,
5255
*v,
53-
MUTE_ANTICLICK_RAMP_MS,
56+
ramp_ms.unwrap_or(MUTE_ANTICLICK_RAMP_MS),
5457
) =>
5558
{
5659
return Ok(());
@@ -183,8 +186,8 @@ impl PipelineManager {
183186
/// Validates that the property can be changed in the current pipeline state.
184187
///
185188
/// `ramp_ms` is consulted only for routes that support smooth interpolation
186-
/// (currently audio `volume`-element `volume`). For other properties it is
187-
/// silently ignored.
189+
/// (currently audio `volume`-element `volume` and `mute`). For other
190+
/// properties it is silently ignored.
188191
pub fn update_element_property(
189192
&self,
190193
element_id: &str,

backend/src/gst/volume_ramp.rs

Lines changed: 48 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ use gstreamer::prelude::*;
2323
use gstreamer_controller::prelude::*;
2424
use gstreamer_controller::{DirectControlBinding, InterpolationControlSource, InterpolationMode};
2525
use std::collections::HashMap;
26-
use std::sync::Mutex;
26+
use std::sync::{Arc, Mutex};
2727
use std::time::Duration;
2828
use tracing::{debug, warn};
2929

@@ -52,13 +52,20 @@ pub struct VolumeRampManager {
5252
/// Pre-mute volume per element id, captured when entering mute so we can
5353
/// restore it on unmute.
5454
pre_mute: Mutex<HashMap<String, f64>>,
55+
/// Per-element generation counter, bumped on every `apply_mute` call. The
56+
/// delayed `mute=true` toggle scheduled by `apply_mute(true, ramp_ms)`
57+
/// captures the generation at scheduling time and bails out on fire if it
58+
/// has been superseded — so a fast unmute cancels a still-pending mute.
59+
/// Wrapped in `Arc` so the spawned tokio task can own a handle.
60+
mute_gen: Arc<Mutex<HashMap<String, u64>>>,
5561
}
5662

5763
impl VolumeRampManager {
5864
pub fn new() -> Self {
5965
Self {
6066
sources: Mutex::new(HashMap::new()),
6167
pre_mute: Mutex::new(HashMap::new()),
68+
mute_gen: Arc::new(Mutex::new(HashMap::new())),
6269
}
6370
}
6471

@@ -190,7 +197,9 @@ impl VolumeRampManager {
190197

191198
/// Toggle `mute` with anti-click protection. Ramps `volume` toward zero
192199
/// before `mute=true` is applied (masking the discontinuity click), and
193-
/// restores the pre-mute volume on unmute.
200+
/// restores the pre-mute volume on unmute. `ramp_ms` controls both the
201+
/// pre-mute fade-out and the post-unmute fade-in — short values (≤50ms)
202+
/// behave like a click guard, longer values produce broadcast-style fades.
194203
///
195204
/// Falls back to a direct `set_property` if the pipeline doesn't have a
196205
/// running stream-time yet.
@@ -199,8 +208,20 @@ impl VolumeRampManager {
199208
element: &gst::Element,
200209
element_id: &str,
201210
target_mute: bool,
202-
anticlick_ms: u32,
211+
ramp_ms: u32,
203212
) -> bool {
213+
// Bump generation up front so any in-flight scheduled `mute=true`
214+
// toggle (from a previous apply_mute(true, …)) sees a stale value
215+
// when it fires and bails out. Both directions invalidate stale
216+
// pending toggles — a second mute(true) replaces the first, and
217+
// mute(false) cancels a pending mute(true).
218+
let current_gen = {
219+
let mut gens = self.mute_gen.lock().unwrap();
220+
let g = gens.entry(element_id.to_string()).or_insert(0);
221+
*g = g.wrapping_add(1);
222+
*g
223+
};
224+
204225
if target_mute {
205226
// Capture pre-mute volume only if not already muted (avoid
206227
// overwriting on repeat-mute).
@@ -216,7 +237,7 @@ impl VolumeRampManager {
216237
}
217238

218239
// Ramp to silence first.
219-
if !self.apply_volume_ramp(element, element_id, 0.0, anticlick_ms) {
240+
if !self.apply_volume_ramp(element, element_id, 0.0, ramp_ms) {
220241
element.set_property("mute", true);
221242
return true;
222243
}
@@ -226,16 +247,29 @@ impl VolumeRampManager {
226247
// A small extra margin (5ms) ensures the volume control source
227248
// has reached zero before mute kicks in — otherwise the hard
228249
// zeroing of the volume array would still produce a click.
250+
// The captured generation is checked on fire: if a newer
251+
// apply_mute call has bumped it, this scheduled toggle is stale
252+
// and must not run (e.g. unmute arrived mid-fade).
229253
let element_weak = element.downgrade();
230254
let element_id_owned = element_id.to_string();
231-
let delay = Duration::from_millis(anticlick_ms as u64 + 5);
255+
let delay = Duration::from_millis(ramp_ms as u64 + 5);
256+
let mute_gen = self.mute_gen.clone();
232257
tokio::spawn(async move {
233258
tokio::time::sleep(delay).await;
259+
let still_current =
260+
mute_gen.lock().unwrap().get(&element_id_owned).copied() == Some(current_gen);
261+
if !still_current {
262+
debug!(
263+
"volume_ramp[{}]: scheduled mute=true superseded, skipping",
264+
element_id_owned
265+
);
266+
return;
267+
}
234268
if let Some(elem) = element_weak.upgrade() {
235269
elem.set_property("mute", true);
236270
debug!(
237-
"volume_ramp[{}]: mute=true applied after anti-click ramp",
238-
element_id_owned
271+
"volume_ramp[{}]: mute=true applied after {}ms fade-out",
272+
element_id_owned, ramp_ms
239273
);
240274
}
241275
});
@@ -256,7 +290,7 @@ impl VolumeRampManager {
256290
// and a normal apply_volume_ramp would produce a flat ramp
257291
// (instant unmute = click). Forcing start=0 guarantees a real
258292
// 0→target fade-in.
259-
if !self.apply_volume_ramp_from(element, element_id, 0.0, target, anticlick_ms) {
293+
if !self.apply_volume_ramp_from(element, element_id, 0.0, target, ramp_ms) {
260294
element.set_property("volume", target);
261295
}
262296
}
@@ -265,9 +299,15 @@ impl VolumeRampManager {
265299

266300
/// Drop all cached control sources. Bindings are owned by the elements
267301
/// and are released when the pipeline drops; this just releases our side.
302+
/// Also bumps every element's generation so any in-flight scheduled
303+
/// `mute=true` toggle from a now-stopped pipeline becomes a no-op.
268304
pub fn clear(&self) {
269305
self.sources.lock().unwrap().clear();
270306
self.pre_mute.lock().unwrap().clear();
307+
let mut gens = self.mute_gen.lock().unwrap();
308+
for v in gens.values_mut() {
309+
*v = v.wrapping_add(1);
310+
}
271311
}
272312
}
273313

backend/tests/volume_ramp_test.rs

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,65 @@ async fn clear_drops_cache_and_subsequent_ramps_work() {
293293
);
294294
}
295295

296+
/// Long mute fades honor the caller-supplied ramp_ms instead of the legacy
297+
/// hardcoded 10 ms anti-click. A 200 ms mute should still have the volume
298+
/// well above zero at +50 ms (the old behavior would have hit zero in ~10 ms).
299+
#[tokio::test(flavor = "multi_thread")]
300+
async fn long_mute_ramp_takes_full_duration() {
301+
let p = TestPipe::new();
302+
let mgr = VolumeRampManager::new();
303+
304+
assert!(mgr.apply_volume_ramp(&p.volume, "v", 1.0, 50));
305+
settle(80).await;
306+
307+
// Long fade-out before mute.
308+
assert!(mgr.apply_mute(&p.volume, "v", true, 200));
309+
310+
// ~50 ms in: well below 1.0 (fade is dB-linear) but not yet silent.
311+
tokio::time::sleep(Duration::from_millis(50)).await;
312+
let mid = p.vol();
313+
assert!(
314+
!p.mute(),
315+
"mute=true must not land before the fade-out has finished"
316+
);
317+
assert!(
318+
mid > 0.01,
319+
"at +50 ms of a 200 ms fade, volume must still be audible (got {})",
320+
mid
321+
);
322+
323+
// After the full duration plus the 5 ms grace, mute=true is applied.
324+
tokio::time::sleep(Duration::from_millis(180)).await;
325+
assert!(p.mute(), "mute=true should land after the 200 ms fade-out");
326+
}
327+
328+
/// Cancel-guard: a mid-fade unmute must cancel the pending `mute=true`
329+
/// toggle scheduled by the long fade-out, otherwise that toggle would land
330+
/// after the unmute and silently kill the route.
331+
#[tokio::test(flavor = "multi_thread")]
332+
async fn unmute_during_long_mute_fade_cancels_pending_toggle() {
333+
let p = TestPipe::new();
334+
let mgr = VolumeRampManager::new();
335+
336+
assert!(mgr.apply_volume_ramp(&p.volume, "v", 1.0, 50));
337+
settle(80).await;
338+
339+
// Start a long fade-out — schedules mute=true at +200 ms.
340+
assert!(mgr.apply_mute(&p.volume, "v", true, 200));
341+
342+
// Unmute well before the scheduled mute=true would fire.
343+
tokio::time::sleep(Duration::from_millis(40)).await;
344+
assert!(mgr.apply_mute(&p.volume, "v", false, 50));
345+
346+
// Wait past the original 200 ms+grace window. The previously-scheduled
347+
// mute=true must have observed the bumped generation and bailed out.
348+
tokio::time::sleep(Duration::from_millis(250)).await;
349+
assert!(
350+
!p.mute(),
351+
"stale scheduled mute=true must be cancelled by an intervening unmute"
352+
);
353+
}
354+
296355
/// `is_volume_element` correctly distinguishes the volume element from
297356
/// other audio elements that happen to expose a `volume` property.
298357
#[tokio::test(flavor = "multi_thread")]

openapi.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
"license": {
1111
"name": "MIT OR Apache-2.0"
1212
},
13-
"version": "0.4.12"
13+
"version": "0.4.13-dev"
1414
},
1515
"paths": {
1616
"/api/auth/status": {
@@ -9885,7 +9885,7 @@
98859885
"null"
98869886
],
98879887
"format": "int32",
9888-
"description": "Optional ramp duration in milliseconds. Currently only honored for\naudio `volume`-element `volume` updates — when set, the value is\ninterpolated per-sample over the given duration to avoid zipper\nnoise on fader drags or to match an auto-transition duration.\nWhen omitted, a short default ramp is used for `volume`; other\nproperties are set immediately.",
9888+
"description": "Optional ramp duration in milliseconds. Currently honored for audio\n`volume`-element `volume` and `mute` updates — when set, `volume` is\ninterpolated per-sample over the given duration (anti-zipper / fade)\nand `mute=true` is preceded by a fade-out of the same length while\n`mute=false` is followed by a 0→pre_mute fade-in. Useful for\nbroadcast-style on-air / off-air route transitions (e.g. 500 ms).\nWhen omitted, a short default ramp is used for `volume`/`mute`; other\nproperties are set immediately.",
98899889
"minimum": 0
98909890
},
98919891
"value": {

types/src/api.rs

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -83,11 +83,13 @@ pub struct UpdatePropertyRequest {
8383
/// The new value for the property
8484
#[cfg_attr(feature = "validation", garde(skip))]
8585
pub value: PropertyValue,
86-
/// Optional ramp duration in milliseconds. Currently only honored for
87-
/// audio `volume`-element `volume` updates — when set, the value is
88-
/// interpolated per-sample over the given duration to avoid zipper
89-
/// noise on fader drags or to match an auto-transition duration.
90-
/// When omitted, a short default ramp is used for `volume`; other
86+
/// Optional ramp duration in milliseconds. Currently honored for audio
87+
/// `volume`-element `volume` and `mute` updates — when set, `volume` is
88+
/// interpolated per-sample over the given duration (anti-zipper / fade)
89+
/// and `mute=true` is preceded by a fade-out of the same length while
90+
/// `mute=false` is followed by a 0→pre_mute fade-in. Useful for
91+
/// broadcast-style on-air / off-air route transitions (e.g. 500 ms).
92+
/// When omitted, a short default ramp is used for `volume`/`mute`; other
9193
/// properties are set immediately.
9294
#[serde(default, skip_serializing_if = "Option::is_none")]
9395
#[cfg_attr(feature = "validation", garde(range(max = 60000)))]

0 commit comments

Comments
 (0)