Add RX audio stream diagnostics#2889
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds per-RX-audio-stream diagnostics (rate/deficit/late/gaps/freshness/format) sourced from PanadapterStream, resets those diagnostics when the RX audio stream lifecycle changes, and surfaces the results in the Network Diagnostics UI (new Audio tab table + graphs) with an updated dialog style.
Changes:
- Add
PanadapterStream::AudioStreamDiagnosticstracking, snapshot, and reset hooks for VITA audio packets. - Expose audio diagnostics through
RadioModel, and reset diagnostics on RX audio create/adopt/remove events. - Expand/restyle
NetworkDiagnosticsDialogto display audio stream health, per-stream rows, and timing/feed trend charts.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/models/RadioModel.h | Exposes audio stream diagnostics getters/reset on the model. |
| src/models/RadioModel.cpp | Implements diagnostics getter/reset and wires resets into RX audio stream lifecycle. |
| src/gui/NetworkDiagnosticsDialog.h | Extends sampling/state to include RX audio feed/late/gap/format fields and new UI widgets. |
| src/gui/NetworkDiagnosticsDialog.cpp | Implements new RX audio formatting/health logic, table + graph UI, and dialog restyling. |
| src/core/PanadapterStream.h | Defines audio diagnostics/tracker structures and adds snapshot/reset APIs. |
| src/core/PanadapterStream.cpp | Records per-audio-stream timing metrics from incoming packets and provides snapshot/reset behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| constexpr int kAudioSampleRate = 24000; | ||
| constexpr int kOpusFramesPerPacket = 240; |
There was a problem hiding this comment.
Addressed in the amended commit: kAudioSampleRate now references AudioEngine::DEFAULT_SAMPLE_RATE instead of duplicating 24000.
| @@ -162,6 +164,7 @@ bool PanadapterStream::start(RadioConnection* conn) | |||
| m_audioPacketTimerStarted = false; | |||
| m_previousAudioPacketGapMs = 0; | |||
| m_audioPacketJitterEstimateMs = 0.0; | |||
| resetAudioStreamStats(); | |||
There was a problem hiding this comment.
Addressed in the amended commit: removed the duplicate manual audio gap/jitter resets from start, rebindToEphemeralPort, startWan, and stop; resetAudioStreamStats() is now the single reset path.
| const QString audioHealth = formatAudioHealth(sample); | ||
| m_audioStreamHealthLabel->setText(audioHealth); | ||
| if (audioHealth == "Healthy") { | ||
| m_audioStreamHealthLabel->setStyleSheet("QLabel { color: #64d36e; font-weight: 700; }"); | ||
| } else if (audioHealth == "Audio stream is arriving too slowly" |
There was a problem hiding this comment.
Addressed in the amended commit: formatAudioHealth() now returns an AudioHealthStatus with an AudioHealthState enum plus display text, and the label color is selected from the enum instead of comparing user-facing strings.
There was a problem hiding this comment.
Thanks @rfoust — nice diagnostic work, and the per-stream feed-rate / deficit / late-arrival metrics will be genuinely useful when triaging RX audio glitches in the field. The wiring through recordAudioStreamPacketLocked inside the existing stats-mutex region looks correct, and resetting on every (re)start / stop / clearRegisteredStreams is the right call.
A few comments — none are blockers.
Copilot findings — all three look valid:
-
kAudioSampleRate = 24000in PanadapterStream.cpp:24 duplicatesAudioEngine::DEFAULT_SAMPLE_RATE(defined atsrc/core/AudioEngine.h:71, used in ~40 places acrossAudioEngine.cpp). Worth referencing the shared constant so the deficit/feed-rate math stays correct if the engine sample rate ever changes. A simple#include "AudioEngine.h"andconstexpr int kAudioSampleRate = AudioEngine::DEFAULT_SAMPLE_RATE;would do it. -
Duplicate field resets in
start/rebindToEphemeralPort/startWan/stop. Each call site now does:m_audioPacketTimerStarted = false; m_previousAudioPacketGapMs = 0; m_audioPacketJitterEstimateMs = 0.0; resetAudioStreamStats(); // ← also resets all three
resetAudioStreamStats()already covers those three members. Drop the manual lines at the four call sites and letresetAudioStreamStats()be the single source of truth — that's exactly what it's there for. -
Brittle string comparisons in the audio-health styling (NetworkDiagnosticsDialog.cpp:2099). The
if (audioHealth == "Healthy") ... else if (audioHealth == "Audio stream is arriving too slowly" ...)branches will silently fall to the warning-color fallback ifformatAudioHealth()'s wording ever drifts. Acknowledging this mirrors the existingnetworkQuality()pattern in the same function, so it's not out of step with the file. Optional follow-up: haveformatAudioHealthreturn an enum (or a{status, displayText}pair) and switch on the enum. Not blocking.
Scope note (minor): The PR description does call out the restyle, so it's not hidden, but the kNetworkDiagnosticsStyle block + Audio/Audio-Feed tab consolidation are really a separate UI refresh from the diagnostics feature. Easier to bisect later if they land as two commits, but it's a one-author PR so I won't push on it.
Things I didn't find issues with:
- The
Lockedsuffix convention is honored —recordAudioStreamPacketLockedis called from inside them_statsMutex-held region inprocessDatagram, andaudioStreamDiagnostics()/resetAudioStreamStats()both re-acquire the lock for snapshot/reset. Looks thread-safe. audioPayloadFramescorrectly returns 0 for unknown PCCs andrecordAudioStreamPacketLockedearly-exits onframes <= 0, so unknown codecs don't pollute the per-stream tracker.- Reset on
clearRegisteredStreamsmatches the existing pattern from #2738.
9853c41 to
7fceb3e
Compare
jensenpat
left a comment
There was a problem hiding this comment.
Slice 0 RX flow: untouched — only adds passive observation in the existing audio-decode branch. Low risk.
Transmit interlock, command ordering, timers: unchanged.
GUI restyle: affects only NetworkDiagnosticsDialog. Per CLAUDE.md, visual design is aligned per Jeremy's chat feedback.
7fceb3e to
eff0fe1
Compare
|
Claude here — merged. Thanks @rfoust, RX audio diagnostics is What stood out on review: The thread-safety design is intentional and well-marked. Naming The health-status rules are conservative in the right way. Compound And the lifecycle-reset discipline — 5 reset sites covering every Auto-merge clean against #2879's TCI tab work despite both touching Ships in v26.5.3. Operators chasing audio-stream weirdness now have 73, |
Summary
Verification
git diff --checkcmake --build build --parallel 8