perf(gui): WAVE scope — incremental reduction + QRhi GPU render, wavestats/scrollTo bridge verbs (Phases 1+2)#3955
Conversation
Phase 2: QRhi GPU render — before/afterPhase 2 is now on this branch ( Measured before/after (the three requested scenarios)Same rig and methodology as the Phase-1 table (live FLEX-8400M, 48 kHz RX, cputime over 40 s, interleaved runs, display held awake). Before = Phase 1 (
The matched disabled controls (97.6 vs 97.3) show ambient conditions were equal — the deltas are real. Net: the meter that used to be one of the largest CPU consumers now costs ~100 µs of main thread per frame in every view mode (Scope/Envelope/History/Bands all measured 101–152 µs), and the 60 fps default is effectively free. Strip: with the new New bridge verbs (documented in docs/automation-bridge.md)
Safety proofs (GPU build, live on ANT2 dummy load)
💻 Generated with Claude Code (claude-fable-5.0 7/1/26) with architecture by @jensenpat |
CI failure triage — Linux-only compile break in the QPainter pathThanks for this, @jensenpat — the Phase 1+2 work and the before/after measurements are genuinely impressive, and the perf wins speak for themselves. This is not a CI-infrastructure problem; it's a real compile error in the branch, but in a configuration your local build and macOS CI never exercise. Here's the shape of it and how to reproduce it in seconds. What failed (commit
|
| Job | Platform | Result |
|---|---|---|
build |
Linux (GCC, CI container) | ❌ failed at the Build step |
analyze (cpp) |
Linux CodeQL (same compile) | ❌ failed at the Build step |
check-macos |
macOS (Clang, Homebrew Qt) | ✅ passed |
check-windows |
Windows (MSVC) | 🟡 still running when I looked |
Both red jobs are Linux and both died at the compile step (containers set up fine, configure succeeded) — so it's the code, and it's specific to the Linux toolchain.
⚠️ Honest caveat: the raw compiler line isn't reachable through my tooling right now (the log API only exposes step-level status, and I'm not going to invent an error message). Please open the Build step log for thebuildjob and read the firsterror:— everything below it is fallout. What follows is the mechanism and a local repro that will surface that exact line for you.
Why macOS is green but Linux is red
This is the key insight. From CMakeLists.txt (lines ~296–299), AETHER_GPU_SPECTRUM auto-disables on Qt < 6.7:
if(Qt6_VERSION VERSION_LESS "6.7")
set(AETHER_GPU_SPECTRUM OFF) # e.g. the Ubuntu/CI-container QtYour WaveformWidget.h then swaps the whole base class on that flag:
#ifdef AETHER_GPU_SPECTRUM
#define WAVEFORM_BASE_CLASS QRhiWidget // macOS + your local build
#else
#define WAVEFORM_BASE_CLASS QWidget // ← Linux CI compiles THIS
#endifSo the two worlds compile different code:
- Your local build + macOS CI → Qt 6.7+ →
QRhiWidgetbranch → the GPU#ifdef AETHER_GPU_SPECTRUMregions ofWaveformWidget.cpp. - Linux CI → older Qt → GPU auto-off →
QWidgetbranch → the#ifndef AETHER_GPU_SPECTRUMQPainter/paintEventpath (WaveformWidget.cpp~293–843, incl.drawGraph/drawEnvelope/drawBars/drawVerticalBars).
Your own Phase-2 comment says the QPainter path "compiles unchanged without the flag" — Linux CI is the thing telling us it doesn't. Every measurement/safety proof on this branch was on a GPU build, so the #ifndef path is code that has literally never been compiled in your loop. Combined with GCC + older Qt being stricter about transitive includes than Clang + newer Qt, that's the classic "green everywhere but Linux" signature.
Reproduce it locally in ~30 seconds
You don't need the container — just build the Linux configuration explicitly:
cmake -B build-nogpu -G Ninja -DCMAKE_BUILD_TYPE=RelWithDebInfo -DAETHER_GPU_SPECTRUM=OFF
cmake --build build-nogpu -jThat forces the exact #else/#ifndef path Linux CI compiles and will reproduce the error on your machine, regardless of your local Qt version.
Most likely suspects (from the diff)
I checked and ruled out the crude structural failure — the non-GPU paint path doesn't reference any of the GPU-only members (m_colTex, m_wavePipeline, …), and there's no #ifndef-side use of a GPU-guarded symbol. So look instead at:
- A lost/dropped include in the QPainter path.
WaveformWidget.cppdropped#include <QPainterPath>and added<QFile>/<QLineF>/<QPolygonF>. If anything in the#ifndefblocks still uses a type that newer Qt/Clang pulled in transitively (aQPainterPath, or a symbol that used to arrive viaStripWaveform.cpp's includes — that file was deleted in this PR), GCC + older Qt will reject it. Add the explicit#includefor whatever the firsterror:names. - A Qt ≥ 6.7 API used in the shared or
#ifndefpath. Anything new that only exists in recent Qt will fail against the container's Qt. The first error line will name it.
Given point 1 and the file deletion, my money's on a missing #include in the QPainter path — usually a one-line fix.
Nothing here undercuts the actual work; it's a config-coverage gap, not a design problem. Once the -DAETHER_GPU_SPECTRUM=OFF build is clean locally, CI should follow. Happy to take another look once you push — and thanks again for volunteering the time on such a thorough perf pass. 🙏
🤖 aethersdr-agent · cost: $6.6898 · model: claude-opus-4-8
|
Fixed in Beyond the compile fix, I closed the coverage gap the review called out: the non-GPU configuration now builds clean locally and the resulting binary was smoke-tested live — the QPainter scope paints, CI should go green on this push. 💻 Generated with Claude Code (claude-fable-5.0 7/1/26) with architecture by @jensenpat |
There was a problem hiding this comment.
Thanks for this, @jensenpat — the incremental-reduction model is a clean, well-motivated design, and the writeup (honest parity table included) makes it easy to review. I checked threading, resource lifetimes, and the fork fold-in; a few notes below, mostly minor, plus one product-level call for the maintainers.
Things that check out
- Threading is safe.
appendScopeSamplesis wired fromAudioEngine::{rx,tx}PostChainScopeReadyviaQt::QueuedConnection(MainWindow.cpp:1176-1183), soWaveformScopeModel::append()and the paint/mergeColumns()read both run on the GUI thread — no lock needed on the shared model, and the value-type pause snapshot (m_pausedModel = activeModel()) is a plain GUI-thread copy. - Conventions.
AppSettingsused throughout (no strayQSettings); the#2495QRhi deregister-before-reparent guard mirrors the SpectrumWidget pattern in bothWaveformWidget::prepareForTopLevelChange()and the newContainerManager::prepareRhiChildrenForReparent(); the fork consolidation deletes ~900 dup lines and no danglingStripWaveformreferences remain (only comments). - Model reduction is sound — every column in
mergeColumns()maps to at least one filled bin, window stats are recomputed from bins (not double-counted from columns), and the paused re-window path re-bins from the frozen raw ring rather than pulling live data.
Worth a decision (maintainers)
Default FPS 24→60 is a measured whole-process CPU regression on CPU-render builds. Your own table shows 1 s/docked going 98.9% → 113.5% (+14.6 pts). The GPU path is what recovers that, but AETHER_GPU_SPECTRUM auto-disables on Qt < 6.7 (CMakeLists.txt:298 — e.g. Ubuntu 24.04 / distro Qt), so those users get the CPU path and the new 60 fps default with no offsetting win until they're on Qt 6.7+. Since this is a Linux-native client, that's a non-trivial slice of the install base. Consider keeping kDefaultFps = 24 (or defaulting to 60 only when the GPU path is compiled in) until Phase 2 is effective for most builds. You already flagged it as a one-constant revert — just raising it explicitly so it's a conscious call, not a default.
Minor
- Re-init leak on shader-load failure (GPU build only). In
initialize()(WaveformWidget.cpp:1254-1256), if a.qsbfails to load,m_wavePipeline/m_ovPipelinestay null,m_rhiInitializedstays false, andrender()callsinitialize()again next frame — re-runninginitWavePipeline()/initOverlayPipeline()and overwritingm_waveVbo/m_colTex/etc. without deleting the prior objects, leaking per frame. Degenerate case (missing embedded resource), but worth a one-shot "init attempted" guard or releasing on the failure branch. - Stale comment: WaveformWidget.h:189 documents
m_colTexas "RGBA16F" but it's created and uploaded asRGBA32F(WaveformWidget.cpp:1118, matching the float staging buffer). Comment only. - Observation, not a bug: the Strip profile advertises a 30 s window ceiling, but
StripWaveformPanel's window slider is stillsetRange(1, 20), so 30 s is only reachable programmatically. Fine as a safety cap — just noting the description/UI gap.
Nice work overall — the get wavestats / scrollTo bridge additions are a genuinely useful way to make the Phase 2 win provable.
🤖 aethersdr-agent · cost: $6.2179 · model: claude-opus-4-8
ten9876
left a comment
There was a problem hiding this comment.
Reviewed with a multi-angle pass (line-by-line, removed-behavior, cross-file, reuse, efficiency, altitude, conventions, and a deep reduction/threading/shader-correctness angle), then verified each candidate against source. Strong PR — the incremental-reduction core is correct and the threading is safe by construction.
Several serious-looking candidates were checked and refuted, worth recording so they don't get re-raised:
- No audio/GUI data race. Samples arrive via
AudioEngine::tx/rxPostChainScopeReadyconnected in the context-object lambda form (receiver = the widget), soAutoConnectionmarshalsappendScopeSamples/model.appendto the GUI thread regardless of the emitting thread.mergeColumns/paint run on that same thread — the model is single-threaded, no lock needed. - RMS is correct.
mergeColumnssums raw per-binsumSqandcount, thensqrt(sumSq/count)— identical to a full-window rescan; partial bins contribute their true count. No phase-mixing bias. resetCurrentBinfully initializes (m_cur = Bin{}value-inits every field).binAtwraparound verified correct on a partially-filled ring.doScrollTo'sinViewportmaps both rects to global consistently.
One thing I'd genuinely push for: WaveformScopeModel ships with no unit test, though it's a pure value-type reduction core (the sibling OccupiedRegion in #3945 shipped adaptive_filter_test). A test asserting mergeColumns/windowStats equal a brute-force full-window rescan — across window/rate changes, partial bins, and empty input — would lock down exactly the subtle math the finder pass had to hand-verify. Not a blocker, but high value.
Three inline notes below (one with a code suggestion). All minor/optional. a11y follow-up filed as #3959.
Nice work — mergeable as-is.
…pters + get panstats bridge verb (#3958) ## Summary 2D panadapter rendering-path optimization, profiled first and proven after with the agent automation bridge (live FLEX-8400M session, Mac Studio M2 Ultra, Retina 2×, 2140×651 pan). - **`panscope.frag` — per-pixel FFT trace.** The spectrum trace (gradient/heat-map fill + feather + core stroke) is now evaluated per pixel from a width×1 R32F column texture (R16F fallback where float32 isn't filterable), drawn as one quad. This replaces the per-frame CPU vertex bake — two antialiased triangle-strip line passes plus a 2N fill strip, rebuilt and re-uploaded every frame (~1.4 MB/frame at a 2140 px pan). `buildFftDisplayTrace`'s spatial smoothing + Catmull-Rom interpolation is unchanged, now sampled at exactly one point per device pixel column; stroke width is slope-corrected in the shader via screen-space derivatives so steep peak flanks keep their width, matching the old perpendicular-offset geometry. Cross-platform via `qt_add_shaders` (SPIR-V / GLSL / HLSL / MSL). - **Present coalescing.** FFT frames and waterfall rows arrive as separate UDP events, each scheduling its own window flush — a narrow pan alone generated ~83 flushes/s (57 FFT + 26 rows), which together with the other animated widgets starves the swapchain drawable pool and blocks the GUI thread in `CAMetalLayer nextDrawable` (the #3938 mechanism). Data-driven repaints now coalesce into one present per 16 ms slot with a trailing update (no frame is dropped); interactive paths still repaint immediately. Lean mode keeps its ~30 Hz cap. - **FFT FPS ceiling 30 → 60.** The 30 fps limit was purely the client slider — the FLEX-8400M delivers ~57 frames/s when asked. With the flat per-pixel path, 57 fps costs **+5.8 whole-process CPU points** over 30 fps. - **`get panstats` bridge verb** (`get panstats [<panIndex>|<objectName>] [reset]`): per-SpectrumWidget frame-cost counters — `gpuFrameMsPerSec` (headline main-thread budget), FFT build/upload volume, overlay rebuilds with first-cause attribution (`smartMtr`/`detect`/`other`), waterfall upload volume — for profiler-free before/after proofs. Documented in `docs/automation-bridge.md`. - **`AETHER_PAN_NO_NATIVE_WINDOW=1` diagnostic** to probe the composited (non-native) QRhiWidget path on macOS. Result: still fails to obtain a QRhi on Qt 6.9 (#714 behavior — parent surface is raster), so the native NSView stays the default; the probe documents the finding for a future Qt. ## Measurements `get panstats 0 reset` → 40 s → read; identical 2140×651 pan, 2D, 30 fps, idle RX, interleaved same hour: | metric | before | after | |---|---:|---:| | pan main-thread frame prep (`gpuFrameMsPerSec`) | 17.9 ms/s | **3.6 ms/s** | | avg prep per frame | 601 µs | **118 µs** | | FFT trace build (`fftBuildMsPerSec`) | 14.2 ms/s | **0.8 ms/s** | | trace GPU upload | 40.9 MB/s | **0.5 MB/s** | | ingest / overlay / waterfall | 0.45 / ~0 / ~0 ms/s | unchanged | Whole-process CPU (cputime delta / 40 s, same session, big window 2400×1300): | config | CPU | |---|---:| | optimized @ 30 fps | 117.1 % | | optimized @ 57 fps delivered | **122.9 %** | | baseline @ 30 fps (same hour) | 120–131 % | Honest reading: at matched 30 fps, whole-process CPU is at parity-to-slightly-better — the pan's own prep was ~18 ms/s of a ~450 ms/s main thread, and the freed time is partly reabsorbed by other animated widgets that were previously starved behind drawable waits (main-thread `sample` shows `renderGpuFrame` dropping 243 → 30 samples/10 s while WAVE/S-meter paints rise). The structural wins are: the pan's cost is now **width- and fps-flat**, ~2× the frame rate costs ~6 CPU points, and two pans at 57 fps present at ~41–44 frames/s each with <6 ms/s prep each. The remaining window-level costs (raster `flushSubWindow` blends from the flag/menu widgets over the native pan view, WAVE's QPainter paints) are outside this path — WAVE is addressed by #3955, and the native-window blend tax is documented via the new probe. ## Screenshots Per-pixel trace, 2D + heat-map fill, band plan/scales/markers intact (visual parity with the vertex path):  Running at 57 delivered FFT frames/s (slider at 60):  RX recovery immediately after an ANT2 dummy-load TX key/unkey cycle:  ## TX / regression proofs (agent automation bridge, live radio) - PTT key on ANT2 dummy load → `transmitting:true`, pan keeps rendering; unkey → instant RX trace + waterfall recovery (grabs above). - `pan create` → both pans on the GPU path (~41–44 presents/s each, <6 ms/s prep each; coalescer merges 57 FFT + 26 WF events/s correctly) → `pan close 1` clean. - 2D ↔ 3D stacked-trace toggle unaffected (trace build/draw skipped in 3D exactly as before). - Lean mode, line-width 0 ("Off"), heat-map and solid fill modes all handled in-shader with the old constants (`kFftLineFeatherPx`, core α 0.90, feather α 0.22). - Software (non-GPU-build) QPainter path untouched. ## New automation verbs that helped / follow-ups - `get panstats` (this PR) made every number above reproducible without a profiler. - Follow-up candidates: a `stats window` verb exposing per-window flush/drawable-wait counters (would make the #3938 contention measurable directly), and a `wavestats`-style counter for `SMeterWidget`/`VfoWidget` paint rates. 💻 Generated with Claude Code (claude-fable-5 7/1/26) with architecture by @jensenpat 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: jensenpat <84298137+jensenpat@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Jeremy [KK7GWY] <105957118+ten9876@users.noreply.github.com>
Locks down the WAVE-scope incremental-reduction math the render path depends on — known-signal peak/RMS/clip, [-1,1] clamping and non-finite -> 0, empty-window handling, degenerate column counts, and re-bin on reconfigure — via a Qt6::Core-only standalone test, mirroring the sibling reduction core's tests/adaptive_filter_test.cpp (aethersdr#3878). (aethersdr#3955 review) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Gate the column/clip QRhi texture upload on model.generation() (the dirty counter the model already exposes), so a static or long-window scrolling scope stops re-uploading identical textures every frame. The gate is reset when the textures are recreated on width change so a new/resized texture is always populated. - mergeColumns(): restore the old buildColumns() no-data sentinel (min > max) for empty columns so the render draws nothing there rather than a flat zero-line pixel. - Mark the data-bearing scope's missing QAccessibleInterface with a TODO(a11y) referencing the follow-up (aethersdr#3959), per docs/a11y.md. (aethersdr#3955 review) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…shared widget Replace the per-repaint full-window rescan (three O(window) passes: ring copy, stats, per-pixel columns) with WaveformScopeModel — samples fold into ~2048 fixed-duration bins as they arrive, and repaints merge bins into pixel columns in O(bins + width). Per-frame cost no longer scales with the time window; fps is now the only cost knob. Paint-path cuts (visually identical): grid + dB labels cached in a QPixmap keyed on size/dpr/zoom/theme/font (kills per-frame line drawing and HarfBuzz text shaping, a top cost in the aethersdr#3283 profile); one drawLines() batch instead of ~columnCount drawLine() calls; drawPolyline() over reused buffers instead of four per-frame QPainterPaths; data repaints invalidate only the plot area with the RMS/PK readout refreshed at ~fps/5 (steadier to read, no per-frame text shaping). Bands mode now copies only its <=1536-sample analysis tail instead of the whole window. Fold the StripWaveform fork (~900 duplicated lines) into WaveformWidget(Profile::Strip): the profile carries the fork's real deltas (30 s window ceiling, antialiased stroke); ring policy and the 120 Hz repaint ceiling are shared. Pausing now snapshots the model, so changing the window while paused re-windows the frozen audio instead of pulling live samples. Applet FPS slider is 5-60 (default 60, was 5-30/24). Measured on a live FLEX-8400M session: 60 fps costs ~209 ms/s of main-thread paint vs ~97 ms/s at 24 fps (+15% process CPU at defaults) — the remaining per-frame cost is Qt's antialiased polyline stroking, which is the Phase-2 GPU target. Phase-2 hooks: ColumnStats is a contiguous POD array uploadable as a 1-D texture, and generation() is the upload dirty flag. TX/RX safety (aethersdr#2688): no AudioEngine changes; appendScopeSamples() contract unchanged. Live proof on ANT2 dummy load: scope flips to TX on PTT, back to RX on unkey. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds a wavestats model to the bridge's get verb: every WaveformWidget instance (sidebar WAVE applet + Aetherial strip panels) reports live paint/append counters — paintMsPerSec is the main-thread paint budget the scope consumed, so rendering-cost changes are provable end-to-end without a profiler attach. Selector filters by scope objectName; property 'reset' zeroes the counters after the read so successive reads measure disjoint intervals. Widgets are found by class and snapshotted via a Q_INVOKABLE meta-call (core/ stays GUI-header-free), deduped by pointer because a floated container is reachable from two top-level roots. The WAVE drawer controls (view combo, zoom/FPS/window sliders) and both scope widgets gained objectNames + accessibleNames, so the bridge can target them (a11y backlog) — used for the fps/window sweeps in the before/after CPU proof. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WaveformWidget follows SpectrumWidget's AETHER_GPU_SPECTRUM dual-path pattern: with the flag it is a QRhiWidget (Metal + WA_NativeWindow on macOS) and the waveform is evaluated per-pixel in a fragment shader; without it the QPainter paintEvent path is compiled unchanged. Frame = three blended quads, layered exactly like the QPainter path: the cached grid/background image (now carries the dB labels AND the opaque background), the new wavescope.frag (all four view modes, clip ticks, from a columnCount×1 RGBA32F column texture — LINEAR-sampled so curves interpolate the way polyline strokes did — plus an R8 clip texture), and a text overlay re-rendered only when its content key changes (readout throttled to ~5 Hz). Per-frame uploads are the ≤4 KB column texture and a ~200 B UBO. Measured on the live FLEX-8400M session: Scope at 60 fps drops from ~4.4 ms/frame (~209 ms/s of main-thread paint) to ~120 µs/frame (~5 ms/s) — ~40×. The strip's 20 s / 90 fps envelope renders at ~107 µs/frame. Bands' Goertzel bank is cached on the model generation with a 40 ms floor so the render loop doesn't re-run it per frame. Reparent safety (aethersdr#2495/aethersdr#1240 class): prepareForTopLevelChange() mirrors SpectrumWidget's WindowAboutToChangeInternal send, and ContainerManager fires it for every QRhiWidget in a container's subtree before float/dock reparents. Proven with five live float/dock cycles. The bridge's grab already routes any QRhiWidget through grabFramebuffer(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
scrollTo <target> (alias ensureVisible) scrolls the nearest QScrollArea ancestor so the target widget sits in its viewport, echoing the resulting scrollbar positions and an inViewport confirmation. Widgets below the fold of a scroll area receive no paint events at all until scrolled into view (macOS clips paint delivery to the exposed area), so this is the difference between measuring the Aetherial strip's waveform panel and silently measuring nothing. Target resolution now prefers a VISIBLE match when several widgets share a class, accessibleName, or objectName — every scroll area owns hidden QScrollBars next to the visible one, and the strip owns a hidden TX scope next to the visible RX one. Hidden widgets remain addressable when they are the only match, so hidden-container grabs keep working. Documented in docs/automation-bridge.md (verb table + catalog entry). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…CTRUM The scrollTo verb's QScrollArea/QScrollBar includes landed inside the '#ifdef AETHER_GPU_SPECTRUM' block that guards <QRhiWidget>, so the non-GPU configuration (Linux CI's pre-6.7 Qt auto-disables the flag) failed to compile doScrollTo(). Move them out — scrollTo is not a GPU feature. Verified both configurations build clean, and smoke-tested the non-GPU binary live: the QPainter scope paints, scrollTo answers with inViewport confirmation, and grab captures the widget. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Locks down the WAVE-scope incremental-reduction math the render path depends on — known-signal peak/RMS/clip, [-1,1] clamping and non-finite -> 0, empty-window handling, degenerate column counts, and re-bin on reconfigure — via a Qt6::Core-only standalone test, mirroring the sibling reduction core's tests/adaptive_filter_test.cpp (aethersdr#3878). (aethersdr#3955 review) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Gate the column/clip QRhi texture upload on model.generation() (the dirty counter the model already exposes), so a static or long-window scrolling scope stops re-uploading identical textures every frame. The gate is reset when the textures are recreated on width change so a new/resized texture is always populated. - mergeColumns(): restore the old buildColumns() no-data sentinel (min > max) for empty columns so the render draws nothing there rather than a flat zero-line pixel. - Mark the data-bearing scope's missing QAccessibleInterface with a TODO(a11y) referencing the follow-up (aethersdr#3959), per docs/a11y.md. (aethersdr#3955 review) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nvalidation, reparent guard (aethersdr#3955) Review follow-ups for aethersdr#3955, on top of a rebase onto current main (the wavestats/panstats verb collision is resolved keep-both). Both AETHER_GPU_SPECTRUM on/off configs build clean; adds a model regression test. - WaveformScopeModel: size the raw ring to the MAX window (new setMaxWindowMs), not the current one, so widening the time slider — live or on a paused snapshot — reveals captured history instead of a blank plot that refills over the next N seconds (restores the old StripWaveform::ensureCapacity invariant). Wired from the widget's per-profile ceiling. - WaveformWidget: reset the GPU column-upload and Bands generation dirty-keys on setTransmitting/setViewMode/setPaused. The keys are per-model/per-frame counters not namespaced to which model/mode produced the cached data, so a display-source or view-mode swap with unchanged columnCount could skip the refresh and render the previous source's data (worst case: a paused Bars<->VerticalBars channel-packing reinterpret). - WaveformWidget: fold devicePixelRatio into the overlay-image key so moving the window between monitors of different scale (same logical size/text) rebuilds the overlay at the new resolution instead of returning early, blurry. - WaveformWidget: replace the overlapping-shift XOR grid-theme cache key (which loses bits and lets distinct color sets collide, stranding the old theme's grid) with qHashMulti over all five rgba() values. - WaveformWidget.h: correct the m_colTex comment (RGBA32F, not RGBA16F). - ContainerManager::reparentContainer: fire prepareRhiChildrenForReparent before the reparent, matching floatContainer/dockContainer, so a QRhiWidget child (the WAVE scope) can't leave a stale aethersdr#2495 cleanup callback on a GPU build. - tests: assert a 3 s-old burst survives a window widen. Fixes Are Demonstrated (Principle XI). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
a148703 to
1285e29
Compare
|
Rebased onto current Fixes (commit
Intentionally not changed (raising for your call): the visible-preferred target resolution is your documented design, and the 24→60 fps default is a maintainer product call (per @aethersdr-agent's note re: Qt<6.7 CPU-render builds). The dead CI running now. |
There was a problem hiding this comment.
Reviewed the full diff on head 1285e29e. This is a strong, well-structured piece of work — the incremental reduction core is cleanly separated, value-typed (so pause = a plain copy), and backed by a focused unit test (waveform_scope_model_test.cpp) that locks down the peak/RMS/clip math, NaN/inf clamping, empty-window and degenerate-column edge cases, and the setMaxWindowMs history-retention invariant. Conventions are followed throughout (AppSettings via settings.value(...) in WaveApplet, RAII, std::clamp/std::isfinite, complete GPU teardown in releaseResources()). Nice job, @jensenpat.
On the three prior review comments from @ten9876 — all already addressed in the head, worth confirming for the record:
- Empty-column sentinel (WaveformScopeModel.cpp:250-255) — the
count > 0 ? … : min=1/max=-1branch is in place, so an empty column renders nothing rather than a zero line. Applied ✔ - Texture-upload generation gating (WaveformWidget.cpp:1395-1409) — uploads now skip when
model.generation() == m_lastColUploadGen, with the counter reset to~0ullon texture recreate/width change so a resized texture is always repopulated. Correct handling of the reset case. Applied ✔ - a11y (WaveformWidget.cpp:319-324) —
// TODO(a11y):marker at the paint/render entry point tracking #3959. Applied ✔
Non-blocking observations:
-
Bridge widget-resolution behavior change (out of stated WAVE scope).
resolveWidget()andmatchRecursive()in AutomationServer.cpp now prefer a visible match over the first match (falling back to hidden). This is well-justified for the strip's dual TX/RX scopes and the newscrollToflow, but it's a global change affecting every bridge lookup, not just WAVE. Any existing driver script that relied on getting the first (possibly hidden) match could see different targeting. Worth a maintainer's conscious sign-off; I don't see a correctness problem with it. -
60 fps default is a product tradeoff, not a code issue. The PR is honest that at the 1 s docked default this costs ~+14.6 whole-process CPU points vs. 24 fps until Phase 2's GPU stroking lands, and it's a single constant (
kDefaultFps) to revert. Saved user values are correctly honored and clamped to [5, 60] (WaveApplet.cpp:137-140), so only never-configured users move to 60. Flagging so the assignee weighs the interim cost deliberately.
No bugs, null-deref risks, or leaks found. The #2495 QRhi-reparent guard is correctly extended to cover the WAVE scope now that it's a QRhiWidget (ContainerManager.cpp prepareRhiChildrenForReparent), the StripWaveform fork is fully folded in with no dangling references, and the TX/RX appendScopeSamples contract is untouched (good #2688 guard). LGTM pending the maintainer's call on the two tradeoffs above.
🤖 aethersdr-agent · cost: $4.1478 · model: claude-opus-4-8
ten9876
left a comment
There was a problem hiding this comment.
Approving after the rebase + review-follow-up fixes: all three required checks green (Linux build, macOS, Windows), both AETHER_GPU_SPECTRUM configs build clean locally, model test passes. Merging per maintainer direction.
Three GPU-only, audio-reactive scenes for the WAVE scope (wavedemo.frag, one extra pipeline over the same fullscreen quad). Demoscene rules: fully analytic, palette entirely from the active theme (accent / bright / success / warning / danger over the theme background — a theme switch recolors the whole show), and every scene keeps moving in an ambient attract mode when no audio flows. - 3D Ridge: hidden-line perspective heightfield of the envelope's last ~3 s (Joy Division × 3DSS) — dark mountain bodies, theme-bright crests, star field. Fed by a 256×96 R32F history ring texture that costs one 1 KB row upload per ~33 ms. - Tunnel: Amiga demo tunnel. The live waveform columns ARE the wall texture, mid-band energy launches bright rings down the bore, high band warms the spokes, RMS breathes the core; low band sways the vanishing point. - Horizon: synthwave grid. The live waveform IS the mountain skyline, a scanline sun swells with RMS, grid speed rides the low band, peaks flash the horizon line. Plumbing: three new ViewMode values + combo entries (GPU builds only — CPU builds fall back to Graph and never offer them), fixed 256-column reduction + 12-band Goertzel energies per frame, ~150 B uniform block. Measured 213–220 µs/frame at 60 fps — showcase costs the same as the utility modes. Classic modes regression-checked; -DAETHER_GPU_SPECTRUM=OFF configuration builds and runs clean (the aethersdr#3955 Linux lesson). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…stacked on #3955) (#3991) > **Stacked on #3955** — the first 5 commits are the WAVE perf PR (incremental reduction + QRhi GPU render). Review only the last commit here; this PR should merge after #3955 (or be rebased onto main once it lands). ## Summary Three GPU-only, audio-reactive showcase visualizations for the WAVE scope — crossing the line from utility to fun, deliberately. One new fragment shader (`wavedemo.frag`), one extra pipeline over the existing fullscreen quad, zero new dependencies. Demoscene rules: fully analytic scenes, the palette comes entirely from the active theme (a theme switch recolors the whole show), and every scene keeps animating in an ambient **attract mode** when no audio is flowing — the screen is never dead. | Mode | Vibe | Audio hooks | |---|---|---| | **3D Ridge** | Joy Division × 3DSS: hidden-line perspective heightfield of the envelope's last ~3 s | envelope history (256×96 R32F ring texture, one ~1 KB row per 33 ms) | | **Tunnel** | Amiga demo tunnel | live waveform columns texture the walls; mid band launches rings down the bore; high band warms the spokes; RMS breathes the core; low band sways the vanishing point | | **Horizon** | Synthwave outrun grid | the live waveform IS the mountain skyline; a scanline sun swells with RMS; grid speed rides the low band; peaks flash the horizon | ## Contained & cheap - New `ViewMode` values + WAVE drawer combo entries (**GPU builds only** — CPU builds fall back to Graph and never offer them; a persisted demo mode degrades gracefully). - Fixed 256-column reduction + the existing 12-band Goertzel (already cached on the model generation) drive everything; per-frame uploads are ≤4 KB columns + ~150 B uniforms + a 1 KB history row. - **Measured 213–220 µs/frame at 60 fps** via `get wavestats` — the showcase modes cost the same as the utility modes. - Classic modes regression-checked after the render-loop restructure (Scope: 136 µs/frame), and the `-DAETHER_GPU_SPECTRUM=OFF` configuration builds and runs clean (the #3955 Linux lesson, applied preemptively this time). ## Screenshots (live RX on the FLEX-8400M) | 3D Ridge | Tunnel | |---|---| |  |  | **Horizon:**  ## Proof Bridge grabs of all three scenes live on the FLEX-8400M (RX band noise as input — voice input makes Ridge/Horizon dramatically more mountainous), plus a Scope-mode regression grab. Perf numbers from `get wavestats` per mode. 💻 Generated with Claude Code (Opus 4.8) with architecture by @jensenpat --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Jeremy [KK7GWY] <kk7gwy@aethersdr.com>
Summary
Phase 1 of the WAVE waveform meter performance plan — CPU-side fixes and proof
tooling. Phase 2 (QRhi GPU render following SpectrumWidget's
AETHER_GPU_SPECTRUMpattern) comes separately; its hooks are in place here.WaveformScopeModel(new): incremental sample→bin reduction. Samplesfold into ~2048 fixed-duration bins per window as they arrive (O(new
samples)); repaints merge bins → pixel columns (O(bins + width)). Replaces
three full O(window) passes per paint (ring copy, stats pass, per-pixel
column build), so per-frame cost no longer scales with the time window — a
20 s strip window now costs the same per frame as 240 ms.
WaveformWidget(Profile::Strip)carriesthe fork's real deltas (30 s window ceiling, antialiased stroke); ~900
duplicated lines deleted.
QPixmap (kills per-frame grid drawing and HarfBuzz text shaping — a top
cost in the Performance: ~92% of CPU is QPainter software rasterization; sluggish pan drag is a full-window sibling repaint #3283 trace); one
drawLines()batch instead of ~205drawLine()calls;drawPolyline()over reused buffers instead of fourper-frame
QPainterPaths; data repaints invalidate only the plot area withthe RMS/PK readout refreshing at ~fps/5 (steadier to read, too).
the whole window first.
the frozen audio instead of silently pulling live samples.
cost below; flip
kDefaultFpsin WaveApplet.cpp if the tradeoff readswrong. Users who previously moved the slider keep their saved value.
get wavestatsbridge verb: per-scope paint/append counters(
paintMsPerSec= main-thread paint budget consumed) for profiler-freebefore/after proofs, documented in docs/automation-bridge.md. WAVE drawer
controls gained objectNames/accessibleNames (a11y backlog + bridge
targeting).
Measurements
Mac Studio (Apple Silicon, Retina 2×), live FLEX-8400M session, RX audio at
48 kHz, display held awake, whole-process CPU via cputime delta over 40 s,
BASE (76d8993) and NEW interleaved back-to-back per scenario. Per-widget
attribution via
get wavestats(NEW only).Honest reading:
meter's cost is dominated by Qt's antialiased polyline stroking
(~4.5 ms/frame for Scope/Envelope at applet size; History/Bands are
~0.6–0.8 ms). That is the piece only Phase 2's GPU path removes. The Performance: ~92% of CPU is QPainter software rasterization; sluggish pan drag is a full-window sibling repaint #3283
profile said exactly this ("~92% of CPU is QPainter software
rasterization").
is flat from 1 s to 10 s — fps is now the only knob), which is what makes
long windows, 192 kHz rates, and the strip's 20–30 s envelopes safe; plus
the consolidation, the pause fix, the Bands tail fix, and the measurement
verb that makes Phase 2's win provable.
paint; +14.6 CPU points whole-process at the 1 s default window. 60 fps
ships as the default per the project decision — it's one constant to revert
if the interim cost reads wrong before Phase 2 lands.
all while scrolled out of its scroll-area viewport (both builds), so its
idle cost is ~0 and its in-view cost is this same shared code path.
(Follow-up: the bridge needs a scroll/ensureVisible verb to measure it
live.)
TX/RX safety (#2688 regression guard)
No AudioEngine changes;
appendScopeSamples(mono, sr, tx)contract unchanged,so the digital-path (DAX/modem) TX scope emits restored in #2688 are
untouched. Live bridge proof on the FLEX-8400M into the ANT2 dummy load
(LSB, silent mic): PTT on → scope header flips to TX at the 24 kHz post-chain
rate and keeps painting; PTT off → RX resumes at 48 kHz immediately.
Pause/resume click, all four view modes, and the popped-out floating window
were visually verified via bridge grabs.
Phase 2 hooks
WaveformScopeModel::mergeColumns()fills a contiguous PODColumnStatsarray — directly uploadable as a 1-D texture by a QRhi render path.
generation()is the dirty counter for upload skipping.the raster backend without touching the reduction.
Related issues
structural fix here, stroking cost remains for Phase 2).
~4.6 points lower with this change, but the reported issue is
present-cadence contention from a second top-level window (FPS-independent
per the reporter), so this PR does not claim to fix it.
💻 Generated with Claude Code (claude-fable-5.0 7/1/26) with architecture by @jensenpat