fix: review findings on the polish + active-speaker batch
Some checks failed
CI / build-and-test (push) Failing after 29s
Some checks failed
CI / build-and-test (push) Failing after 29s
Two real concerns from the code review on ab07297..b266623: 1. ActiveSpeaker removal poisoned the rename-window heuristic. ParticipantTracker.HandleRemoved appends to _recentlyRemoved keyed by MachineName alone; the next Participant Add on the same machine consulted that list with no kind discrimination, so an active-speaker disappearance immediately followed by a participant joining (very common: Teams renames its outputs as participants enter/leave) would cause the new participant to inherit the auto-mix's deterministic v5 GUID. New HandleAutoMixRemoved deliberately skips _recentlyRemoved — the auto-mix row's identity is already stable via the deterministic Id, so re-add restores it without the rename window. 2. IsoPipeline.State writes were not synchronized. Supervisor loop sets State on its own thread; UI thread reads from GetStats. Without volatility, the JIT could cache the field in a register and the UI would stay stuck on Receiving even after Error. Backing field is now an int read/written via Volatile.Read/Volatile.Write, matching the pattern already used for _liveReceiver / _liveSender / _liveProcessor. Tests: 79/79 (was 78) — added ParticipantTrackerTests.ActiveSpeakerRemove_DoesNotPoisonRenameWindowForLaterParticipant which would have caught (1).
This commit is contained in:
parent
b2666236ec
commit
9cb1cc7b3d
3 changed files with 55 additions and 2 deletions
|
|
@ -38,7 +38,7 @@ public sealed class ParticipantTracker
|
|||
HandleRemoved(r.Source);
|
||||
break;
|
||||
case DiscoveryEvent.Removed r when r.Source.Kind == NdiSourceKind.ActiveSpeaker:
|
||||
HandleRemoved(r.Source);
|
||||
HandleAutoMixRemoved(r.Source);
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
|
@ -125,6 +125,23 @@ public sealed class ParticipantTracker
|
|||
_recentlyRemoved.Add(new RecentlyRemoved(existing.Id, source.MachineName, now));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Removes the synthetic auto-mix row's CurrentSource. Crucially does NOT add to
|
||||
/// <see cref="_recentlyRemoved"/> — the auto-mix row's identity is already stable
|
||||
/// via the deterministic v5 GUID, so re-add restores it without needing the
|
||||
/// rename-window heuristic, and we must not let an active-speaker disappearance
|
||||
/// poison the rename matcher for a Participant joining the same machine within
|
||||
/// the window.
|
||||
/// </summary>
|
||||
private void HandleAutoMixRemoved(NdiSource source)
|
||||
{
|
||||
var stableId = DeterministicGuid("auto-mix:" + source.MachineName);
|
||||
var existing = _participants.FirstOrDefault(p => p.Id == stableId);
|
||||
if (existing is null) return;
|
||||
existing.CurrentSource = null;
|
||||
existing.LastSeen = _now();
|
||||
}
|
||||
|
||||
private void PruneRecentlyRemoved(DateTimeOffset now)
|
||||
{
|
||||
_recentlyRemoved.RemoveAll(rr => now - rr.RemovedAt > _renameWindow);
|
||||
|
|
|
|||
|
|
@ -45,7 +45,15 @@ public sealed class IsoPipeline : IAsyncDisposable
|
|||
private readonly object _frameTimesGate = new();
|
||||
|
||||
public Guid ParticipantId { get; }
|
||||
public IsoState State { get; private set; } = IsoState.Idle;
|
||||
|
||||
// Backing field for State, accessed via Volatile.Read/Write so the supervisor
|
||||
// loop's writes are observed promptly by the UI thread's stats poll.
|
||||
private int _state = (int)IsoState.Idle;
|
||||
public IsoState State
|
||||
{
|
||||
get => (IsoState)Volatile.Read(ref _state);
|
||||
private set => Volatile.Write(ref _state, (int)value);
|
||||
}
|
||||
public int ConsecutiveFailures => _consecutiveFailures;
|
||||
|
||||
/// <summary>
|
||||
|
|
|
|||
|
|
@ -135,4 +135,32 @@ public class ParticipantTrackerTests
|
|||
tracker.Participants.Should().HaveCount(1);
|
||||
tracker.Participants[0].Id.Should().Be(firstId);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ActiveSpeakerRemove_DoesNotPoisonRenameWindowForLaterParticipant()
|
||||
{
|
||||
// Regression: the rename-window heuristic matches by MachineName alone, so a
|
||||
// disappearing ActiveSpeaker source on a machine could cause the next
|
||||
// Participant joining that same machine to inherit the auto-mix's GUID, AND
|
||||
// the auto-mix row would be renamed to the participant's display name.
|
||||
// HandleAutoMixRemoved deliberately skips _recentlyRemoved.
|
||||
var tracker = new ParticipantTracker(TimeSpan.FromSeconds(5), () => T0);
|
||||
var autoMix = new NdiSource("WOOGLIN (MS Teams - Active Speaker)", "WOOGLIN", NdiSourceKind.ActiveSpeaker, null);
|
||||
var jane = new NdiSource("WOOGLIN (MS Teams - Jane)", "WOOGLIN", NdiSourceKind.Participant, "Jane");
|
||||
|
||||
tracker.Apply(new DiscoveryEvent.Added(autoMix));
|
||||
var autoMixId = tracker.Participants[0].Id;
|
||||
tracker.Apply(new DiscoveryEvent.Removed(autoMix));
|
||||
tracker.Apply(new DiscoveryEvent.Added(jane));
|
||||
|
||||
// Two distinct rows: the auto-mix (offline, no source) and Jane (a brand-new participant).
|
||||
tracker.Participants.Should().HaveCount(2);
|
||||
var jp = tracker.Participants.Single(p => p.DisplayName == "Jane");
|
||||
jp.Id.Should().NotBe(autoMixId,
|
||||
because: "Jane is a fresh Participant, not a renamed auto-mix");
|
||||
var asRow = tracker.Participants.Single(p => p.DisplayName == "Active Speaker");
|
||||
asRow.Id.Should().Be(autoMixId);
|
||||
asRow.CurrentSource.Should().BeNull(
|
||||
because: "the auto-mix source went away and hasn't been re-added");
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue