From e3bf28a414773a03394abf6ff568f2e797229f4f Mon Sep 17 00:00:00 2001 From: Ednunp <29843396+Ednunp@users.noreply.github.com> Date: Sun, 26 Jul 2026 22:38:40 +0100 Subject: [PATCH] P2 leftovers: every inline dialog now audited + one shared sample clamp (review complete bar peer extraction) Inline-built dialogs were invisible to the accessibility audit - which is exactly how mnemonic-less buttons kept slipping through (the single-instance dialog Andre caught, and now TWO more found by this very change: ManualPeerPrompt's OK/Cancel and ProfileSaveAsPrompt's Cancel had no mnemonics - both fixed). Each inline dialog's construction is split from its ShowDialog into a Build seam, and the audit now covers 15 dialog surfaces (was 8): manual peer prompt, quick profile switch, change password, password manager, profile name prompt, and the service Additional-options window join the eight Form dialogs. Behaviour unchanged. The encoder-boundary clamp is now ONE shared rule (Core SampleClamp) instead of three private copies in MixingEngine / AsioCaptureBackend / PushModeWasapiBackend - and the ASIO copy's up-to-four interlocked increments per frame became one batched add per buffer on the RT thread. Pinned by a new self-test (over-range clamps to exactly +/-1 and counts; +/-1 exactly passes untouched). Gate 61/61. Co-Authored-By: Claude Fable 5 --- src/RemSound.App/ManualPeerPrompt.cs | 20 +++++-- src/RemSound.App/ProfilePasswordDialog.cs | 28 +++++++--- .../ProfilePasswordManagerDialog.cs | 20 ++++++- src/RemSound.App/ProfileSaveAsPrompt.cs | 55 ++++++++++++------- src/RemSound.App/QuickProfileSwitchDialog.cs | 28 +++++++--- src/RemSound.App/SelfTest.cs | 26 +++++++++ src/RemSound.App/ServiceProfileDialog.cs | 30 ++++++---- src/RemSound.Sender/AsioCaptureBackend.cs | 10 ++-- src/RemSound.Sender/MixingEngine.cs | 10 +--- src/RemSound.Sender/PushModeWasapiBackend.cs | 10 +--- 10 files changed, 160 insertions(+), 77 deletions(-) diff --git a/src/RemSound.App/ManualPeerPrompt.cs b/src/RemSound.App/ManualPeerPrompt.cs index c40d3e6..d34043f 100644 --- a/src/RemSound.App/ManualPeerPrompt.cs +++ b/src/RemSound.App/ManualPeerPrompt.cs @@ -4,7 +4,19 @@ internal static class ManualPeerPrompt { public static string? Show(IWin32Window owner) { - using var dialog = new Form + var (dialog, textBox) = Build(); + using (dialog) + { + return dialog.ShowDialog(owner) == DialogResult.OK ? textBox.Text.Trim() : null; + } + } + + /// Construction split from ShowDialog so the accessibility audit can inspect the real + /// dialog (inline-built dialogs used to be invisible to the audit — which is exactly how missing + /// mnemonics slipped through). + internal static (Form Dialog, TextBox Input) Build() + { + var dialog = new Form { Text = "Add manual peer", StartPosition = FormStartPosition.CenterParent, @@ -24,8 +36,8 @@ internal static class ManualPeerPrompt Width = 380, AccessibleName = "Peer IP address or hostname", }; - var okButton = new Button { Text = "OK", AutoSize = true, DialogResult = DialogResult.OK }; - var cancelButton = new Button { Text = "Cancel", AutoSize = true, DialogResult = DialogResult.Cancel }; + var okButton = new Button { Text = "&OK", AutoSize = true, DialogResult = DialogResult.OK }; + var cancelButton = new Button { Text = "&Cancel", AutoSize = true, DialogResult = DialogResult.Cancel }; textBox.KeyDown += (_, args) => { if (args.KeyCode == Keys.Enter) @@ -46,6 +58,6 @@ internal static class ManualPeerPrompt dialog.Controls.Add(panel); dialog.AcceptButton = okButton; dialog.CancelButton = cancelButton; - return dialog.ShowDialog(owner) == DialogResult.OK ? textBox.Text.Trim() : null; + return (dialog, textBox); } } diff --git a/src/RemSound.App/ProfilePasswordDialog.cs b/src/RemSound.App/ProfilePasswordDialog.cs index b5e7bef..1b70b9e 100644 --- a/src/RemSound.App/ProfilePasswordDialog.cs +++ b/src/RemSound.App/ProfilePasswordDialog.cs @@ -15,7 +15,24 @@ internal static class ProfilePasswordDialog { public static string? Show(string profileTitle, string currentPassword, bool requireNonEmpty = false) { - using var dialog = new Form + var (dialog, textBox) = Build(profileTitle, currentPassword, requireNonEmpty); + using (dialog) + { + // Run with a foreground 1×1 owner so the prompt jumps to the front even when RemSound is + // sitting minimised in the tray — e.g. a quick profile switch to a passwordless-but- + // streaming profile trips the password gate mid-switch, and the user must be able to read + // and answer it there and then. Centres on screen, forces focus, then closes. + return ForegroundDialog.Show(owner => dialog.ShowDialog(owner)) == DialogResult.OK + ? textBox.Text.Trim() + : null; + } + } + + /// Construction split from ShowDialog so the accessibility audit can inspect the real + /// dialog (inline-built dialogs used to be invisible to the audit). + internal static (Form Dialog, TextBox Input) Build(string profileTitle, string currentPassword, bool requireNonEmpty = false) + { + var dialog = new Form { Text = "Change profile password", StartPosition = FormStartPosition.CenterParent, @@ -100,13 +117,6 @@ internal static class ProfilePasswordDialog dialog.Controls.Add(panel); dialog.AcceptButton = okButton; dialog.CancelButton = cancelButton; - - // Run with a foreground 1×1 owner so the prompt jumps to the front even when RemSound is - // sitting minimised in the tray — e.g. a quick profile switch to a passwordless-but- - // streaming profile trips the password gate mid-switch, and the user must be able to read - // and answer it there and then. Centres on screen, forces focus, then closes. - return ForegroundDialog.Show(owner => dialog.ShowDialog(owner)) == DialogResult.OK - ? textBox.Text.Trim() - : null; + return (dialog, textBox); } } diff --git a/src/RemSound.App/ProfilePasswordManagerDialog.cs b/src/RemSound.App/ProfilePasswordManagerDialog.cs index 27135bd..190138d 100644 --- a/src/RemSound.App/ProfilePasswordManagerDialog.cs +++ b/src/RemSound.App/ProfilePasswordManagerDialog.cs @@ -14,10 +14,22 @@ namespace RemSound.App; internal static class ProfilePasswordManagerDialog { public static bool Show(IWin32Window owner, ProfileStore store) + { + var (dialog, rows) = Build(store); + using (dialog) + { + if (dialog.ShowDialog(owner) != DialogResult.OK) return false; + return SaveChanges(store, rows); + } + } + + /// Construction split from ShowDialog so the accessibility audit can inspect the real + /// dialog (inline-built dialogs used to be invisible to the audit). + internal static (Form Dialog, List<(string Title, string Original, TextBox Box)> Rows) Build(ProfileStore store) { var titles = store.ListProfileTitles(); - using var dialog = new Form + var dialog = new Form { Text = "Profile passwords", StartPosition = FormStartPosition.CenterParent, @@ -77,9 +89,11 @@ internal static class ProfilePasswordManagerDialog dialog.Controls.Add(intro); dialog.AcceptButton = okButton; dialog.CancelButton = cancelButton; + return (dialog, rows); + } - if (dialog.ShowDialog(owner) != DialogResult.OK) return false; - + private static bool SaveChanges(ProfileStore store, List<(string Title, string Original, TextBox Box)> rows) + { var changedAny = false; foreach (var (title, original, box) in rows) { diff --git a/src/RemSound.App/ProfileSaveAsPrompt.cs b/src/RemSound.App/ProfileSaveAsPrompt.cs index 7d380b6..23f83d1 100644 --- a/src/RemSound.App/ProfileSaveAsPrompt.cs +++ b/src/RemSound.App/ProfileSaveAsPrompt.cs @@ -23,7 +23,38 @@ internal static class ProfileSaveAsPrompt string dialogTitle = "Save profile as", string promptLabel = "Profile name:") { - using var dialog = new Form + var (dialog, textBox) = Build(defaultName, dialogTitle, promptLabel); + using (dialog) + { + while (true) + { + if (dialog.ShowDialog(owner) != DialogResult.OK) return null; + var name = textBox.Text.Trim(); + if (string.IsNullOrWhiteSpace(name)) + { + MessageBox.Show(owner, "Please enter a profile name.", "RemSound", MessageBoxButtons.OK, MessageBoxIcon.Warning); + continue; + } + if (store is not null && store.Exists(name)) + { + var overwrite = MessageBox.Show(owner, + $"A profile named \"{name}\" already exists. Overwrite?", + "Confirm overwrite", MessageBoxButtons.YesNo, MessageBoxIcon.Question, MessageBoxDefaultButton.Button2); + if (overwrite != DialogResult.Yes) continue; + } + return name; + } + } + } + + /// Construction split from ShowDialog so the accessibility audit can inspect the real + /// dialog (inline-built dialogs used to be invisible to the audit). + internal static (Form Dialog, TextBox Input) Build( + string? defaultName = null, + string dialogTitle = "Save profile as", + string promptLabel = "Profile name:") + { + var dialog = new Form { Text = dialogTitle, StartPosition = FormStartPosition.CenterParent, @@ -40,7 +71,7 @@ internal static class ProfileSaveAsPrompt AccessibleName = promptLabel.TrimEnd(':', ' '), }; var okButton = new Button { Text = "&OK", AutoSize = true, DialogResult = DialogResult.OK }; - var cancelButton = new Button { Text = "Cancel", AutoSize = true, DialogResult = DialogResult.Cancel }; + var cancelButton = new Button { Text = "&Cancel", AutoSize = true, DialogResult = DialogResult.Cancel }; textBox.KeyDown += (_, args) => { if (args.KeyCode == Keys.Enter) @@ -72,24 +103,6 @@ internal static class ProfileSaveAsPrompt dialog.Controls.Add(panel); dialog.AcceptButton = okButton; dialog.CancelButton = cancelButton; - - while (true) - { - if (dialog.ShowDialog(owner) != DialogResult.OK) return null; - var name = textBox.Text.Trim(); - if (string.IsNullOrWhiteSpace(name)) - { - MessageBox.Show(owner, "Please enter a profile name.", "RemSound", MessageBoxButtons.OK, MessageBoxIcon.Warning); - continue; - } - if (store is not null && store.Exists(name)) - { - var overwrite = MessageBox.Show(owner, - $"A profile named \"{name}\" already exists. Overwrite?", - "Confirm overwrite", MessageBoxButtons.YesNo, MessageBoxIcon.Question, MessageBoxDefaultButton.Button2); - if (overwrite != DialogResult.Yes) continue; - } - return name; - } + return (dialog, textBox); } } diff --git a/src/RemSound.App/QuickProfileSwitchDialog.cs b/src/RemSound.App/QuickProfileSwitchDialog.cs index d2fb8b0..60f698f 100644 --- a/src/RemSound.App/QuickProfileSwitchDialog.cs +++ b/src/RemSound.App/QuickProfileSwitchDialog.cs @@ -19,8 +19,25 @@ internal sealed class QuickProfileSwitchDialog public static string? Show(IReadOnlyList profiles) { if (profiles.Count == 0) return null; + var (dialog, list, chosenPath) = Build(profiles); + using (dialog) + { + dialog.Shown += (_, _) => + { + BringToForeground(dialog); + list.Focus(); + }; + return dialog.ShowDialog() == DialogResult.OK ? chosenPath() : null; + } + } - using var dialog = new Form + /// Audit seam: the real dialog, built with a placeholder profile, never shown. Inline-built + /// dialogs used to be invisible to the accessibility audit — how missing mnemonics slipped through. + internal static Form BuildForAudit() => Build(new[] { new ProfileEntry("Audit profile", "audit", true) }).Dialog; + + private static (Form Dialog, ListBox List, Func ChosenPath) Build(IReadOnlyList profiles) + { + var dialog = new Form { Text = "Quick profile switch", AccessibleName = "Quick profile switch", @@ -123,14 +140,7 @@ internal sealed class QuickProfileSwitchDialog } }; - dialog.Shown += (_, _) => - { - BringToForeground(dialog); - list.Focus(); - }; - - var result = dialog.ShowDialog(); - return result == DialogResult.OK ? chosenPath : null; + return (dialog, list, () => chosenPath); } private static void BringToForeground(Form form) diff --git a/src/RemSound.App/SelfTest.cs b/src/RemSound.App/SelfTest.cs index dc358b2..3eca38e 100644 --- a/src/RemSound.App/SelfTest.cs +++ b/src/RemSound.App/SelfTest.cs @@ -69,6 +69,7 @@ internal static class SelfTest RunStep(results, "Cue variant resolution (dedupe, order, chosen-default fallback)", CueVariantResolution); RunStep(results, "Updater swap + rollback (a failed update restores exactly)", UpdaterSwapRollback); RunStep(results, "ASIO per-driver tick memory (snapshot/restore round-trip)", AsioTickMemory); + RunStep(results, "Encoder-boundary sample clamp (shared rule, exact count)", SampleClampRule); RunStep(results, "App settings save and reload", SettingsRoundTrip); RunStep(results, "Per-peer shaping DSP", PeerShapingDsp); RunStep(results, "Multi-output fan-out (both lanes)", FanOutToBothOutputs); @@ -964,6 +965,22 @@ internal static class SelfTest return "follower flagged + sentinel shared with the app; service resolves it to the live default render endpoint"; } + /// The encoder-boundary clamp is now ONE shared rule (SampleClamp) used by all three + /// capture paths — mix engine, ASIO backend, push-mode backend. A clamp bug here means encoder + /// overflow when loud sources sum past unity, while packets still flow. Pins: over-range clamps to + /// exactly ±1, in-range (including exactly ±1) is untouched and uncounted, count is exact. + private static string? SampleClampRule() + { + var buf = new[] { 0.5f, 1f, 1.5f, -1f, -2.5f, 0f, -0.999f }; + var clipped = SampleClamp.ClampBuffer(buf); + Check(clipped == 2, $"exactly the two over-range samples must count as clipped (got {clipped})"); + Check(buf[2] == 1f && buf[4] == -1f, "over-range samples must clamp to exactly ±1"); + Check(buf[0] == 0.5f && buf[1] == 1f && buf[3] == -1f && buf[5] == 0f && buf[6] == -0.999f, + "in-range samples — including exactly ±1 — must pass through untouched"); + Check(SampleClamp.ClampBuffer(buf) == 0, "a clean buffer must count zero"); + return "over-range → ±1 and counted; ±1 exactly and below untouched; count exact"; + } + /// Per-driver ASIO tick memory (Ed's EVO→ReaRoute→EVO silence): switching drivers clears /// the pair ticks by design (pair N is a different physical channel on a different card), and the /// memory restores each driver's OWN ticks on return. Pins the snapshot/restore round-trip, that a @@ -1866,6 +1883,15 @@ internal static class SelfTest ("Keyboard shortcut import", () => new KeyboardShortcutImportDialog(Array.Empty())), ("Update install notice", () => new UpdateInstallNoticeDialog( new UpdateInfo("v9.9", new Version(9, 9, 0), "https://example.invalid/x.zip", "notes", "https://example.invalid/rel"))), + // The inline-built dialogs, via their Build seams — previously invisible to this audit, + // which is exactly how mnemonic-less buttons slipped through (single-instance, manual peer). + ("Manual peer prompt", () => ManualPeerPrompt.Build().Dialog), + ("Quick profile switch", QuickProfileSwitchDialog.BuildForAudit), + ("Change profile password", () => ProfilePasswordDialog.Build("Audit", "pw").Dialog), + ("Profile passwords manager", () => ProfilePasswordManagerDialog.Build(new ProfileStore( + Path.Combine(Path.GetTempPath(), "remsound-selftest-pwmgr-" + Guid.NewGuid().ToString("N")))).Dialog), + ("Profile name prompt", () => ProfileSaveAsPrompt.Build("Audit").Dialog), + ("Service additional options", () => ServiceProfileDialog.BuildAdditionalOptions(false).Dialog), ("Profile selection", () => new ProfileSelectionDialog(new ProfileStore( Path.Combine(Path.GetTempPath(), "remsound-selftest-picker-" + Guid.NewGuid().ToString("N"))))), }; diff --git a/src/RemSound.App/ServiceProfileDialog.cs b/src/RemSound.App/ServiceProfileDialog.cs index 16bdb1f..42ed22e 100644 --- a/src/RemSound.App/ServiceProfileDialog.cs +++ b/src/RemSound.App/ServiceProfileDialog.cs @@ -334,7 +334,23 @@ internal sealed class ServiceProfileDialog : Form private void ShowAdditionalOptions() { - using var dlg = new Form + var (dlg, logging) = BuildAdditionalOptions(ServiceLoggingEnabled); + using (dlg) + { + if (ForegroundDialog.Show(owner => dlg.ShowDialog(owner)) == DialogResult.OK) + { + ServiceLoggingEnabled = logging.Checked; + } + } + } + + /// Construction split from ShowDialog so the accessibility audit can inspect this inner + /// dialog too. No connect/disconnect cue checkboxes here (removed 2026-07-19, review sweep): the + /// headless service NEVER plays cues — nothing in the service host touches CuePlayer, and a + /// logged-out session couldn't render them anyway. The cue fields stay on Profile for the app. + internal static (Form Dialog, AccessibleCheckBox Logging) BuildAdditionalOptions(bool loggingEnabled) + { + var dlg = new Form { Text = "Additional service options", FormBorderStyle = FormBorderStyle.FixedDialog, @@ -345,22 +361,14 @@ internal sealed class ServiceProfileDialog : Form ClientSize = new Size(460, 110), AccessibleName = "Additional service options", }; - // No connect/disconnect cue checkboxes here (removed 2026-07-19, review sweep): the dialog used - // to offer them, but the headless service NEVER plays cues — nothing in the service host touches - // CuePlayer, and a logged-out session couldn't render them anyway. Offering a switch that does - // nothing is worse than not offering it. The cue fields stay on Profile for the app's own use. - var logging = new AccessibleCheckBox { Text = "Enable service &logging (Alt+L)", AccessibleName = "Enable service logging", AutoSize = true, Checked = ServiceLoggingEnabled }; + var logging = new AccessibleCheckBox { Text = "Enable service &logging (Alt+L)", AccessibleName = "Enable service logging", AutoSize = true, Checked = loggingEnabled }; var ok = new Button { Text = "&OK", AutoSize = true, DialogResult = DialogResult.OK }; var layout = new TableLayoutPanel { Dock = DockStyle.Fill, ColumnCount = 1, Padding = new Padding(12), AutoSize = true }; foreach (var c in new Control[] { logging, ok }) { var w = new FlowLayoutPanel { AutoSize = true, Dock = DockStyle.Fill }; w.Controls.Add(c); layout.Controls.Add(w); } dlg.Controls.Add(layout); dlg.AcceptButton = ok; - - if (ForegroundDialog.Show(owner => dlg.ShowDialog(owner)) == DialogResult.OK) - { - ServiceLoggingEnabled = logging.Checked; - } + return (dlg, logging); } private static Profile CloneProfile(Profile p) => diff --git a/src/RemSound.Sender/AsioCaptureBackend.cs b/src/RemSound.Sender/AsioCaptureBackend.cs index 9cb3b2e..f1d5f59 100644 --- a/src/RemSound.Sender/AsioCaptureBackend.cs +++ b/src/RemSound.Sender/AsioCaptureBackend.cs @@ -350,15 +350,15 @@ internal sealed class AsioCaptureBackend : ICaptureBackend if (lCh < recordChannelCount) l += interleaved[srcBase + lCh]; if (rCh < recordChannelCount) r += interleaved[srcBase + rCh]; } - // Soft-limit-ish clamp at the encoder boundary; matches MixingEngine. - if (l > 1f) { l = 1f; Interlocked.Increment(ref clippedSampleCount); } - else if (l < -1f) { l = -1f; Interlocked.Increment(ref clippedSampleCount); } - if (r > 1f) { r = 1f; Interlocked.Increment(ref clippedSampleCount); } - else if (r < -1f) { r = -1f; Interlocked.Increment(ref clippedSampleCount); } mixScratch[dstBase] = l; mixScratch[dstBase + 1] = r; } + // Encoder-boundary clamp — the shared rule (SampleClamp), with ONE batched counter add per + // buffer instead of the old up-to-four interlocked increments per frame on this RT thread. + var clipped = SampleClamp.ClampBuffer(mixScratch.AsSpan(0, stereoFloats)); + if (clipped > 0) Interlocked.Add(ref clippedSampleCount, clipped); + onMixedSamples(new ReadOnlyMemory(mixScratch, 0, stereoFloats)); // Capture-thread CPU instrumentation (item 2 of RemSoundefficiency.md). Records // the time the WHOLE callback spent — including the synchronous downstream diff --git a/src/RemSound.Sender/MixingEngine.cs b/src/RemSound.Sender/MixingEngine.cs index f586e59..c9044e9 100644 --- a/src/RemSound.Sender/MixingEngine.cs +++ b/src/RemSound.Sender/MixingEngine.cs @@ -382,14 +382,8 @@ internal sealed class MixingEngine : ICaptureBackend } // Hard-clamp mixed sum to [-1, 1] to prevent encoder clipping when multiple loud - // sources sum past unity. Counts clipped samples for diagnostics. - long clipped = 0; - for (var i = 0; i < read; i++) - { - var v = mixScratch[i]; - if (v > 1f) { mixScratch[i] = 1f; clipped++; } - else if (v < -1f) { mixScratch[i] = -1f; clipped++; } - } + // sources sum past unity. Shared rule (SampleClamp); one batched counter add. + var clipped = SampleClamp.ClampBuffer(mixScratch.AsSpan(0, read)); if (clipped > 0) Interlocked.Add(ref clippedSampleCount, clipped); Interlocked.Increment(ref mixTickCount); diff --git a/src/RemSound.Sender/PushModeWasapiBackend.cs b/src/RemSound.Sender/PushModeWasapiBackend.cs index e74432d..63f4e29 100644 --- a/src/RemSound.Sender/PushModeWasapiBackend.cs +++ b/src/RemSound.Sender/PushModeWasapiBackend.cs @@ -369,14 +369,10 @@ internal sealed class PushModeWasapiBackend : ICaptureBackend stereo = stereoScratch; } - // 4. Soft clamp at the encoder boundary (matches MixingEngine / AsioCaptureBackend). + // 4. Encoder-boundary clamp — the shared rule (SampleClamp), one batched counter add. var stereoFloatCount = workingFrames * MixChannels; - for (var i = 0; i < stereoFloatCount; i++) - { - var v = stereo[i]; - if (v > 1f) { stereo[i] = 1f; Interlocked.Increment(ref clippedSampleCount); } - else if (v < -1f) { stereo[i] = -1f; Interlocked.Increment(ref clippedSampleCount); } - } + var clipped = SampleClamp.ClampBuffer(stereo.AsSpan(0, stereoFloatCount)); + if (clipped > 0) Interlocked.Add(ref clippedSampleCount, clipped); // 5. Hand off to the encoder/UDP-send pipeline. Synchronous on the capture thread. onMixedSamples(new ReadOnlyMemory(stereo, 0, stereoFloatCount));