perf(spectrum): pause flag re-raster when not presented + immediate sprite on new slice (#3746)#3764
Conversation
…esented (aethersdr#3746) Follow-up to aethersdr#3695. The 12.5 Hz flag refresh timer re-rasterized every non-live flag every 80 ms regardless of whether the panadapter was on screen. Also adds an aether.perf "FlagGrab" trace (grabbed / total / ms / visible per tick; gated on the category, zero cost when off) as committed measurement scaffolding so the before/after numbers are reproducible on any platform. Run the timer only while the panadapter is actually presented -- visible and not minimized. A minimized top-level keeps child isVisible()==true (measured), so a WindowStateChange watcher on the top-level drives the minimize case; Show/Hide and ParentChange (float/dock reparent) drive the hidden-tab / reparent cases, all funnelled through updateFlagRefreshTimer(). The watcher is reattached on reparent and detached in prepareForTopLevelChange(). Measured (4 slices, i9-13900K, aether.perf): while minimized 0 grabs -- was ~2.1 ms/tick = ~27 ms/s main-thread, continuing unchanged in the old build; visible-period grab cost unchanged at 2.2 ms/tick (no regression); clean START/STOP at every show/minimize/restore transition. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ersdr#3746) A newly created non-live slice flag had no GPU sprite until the next refresh-timer tick: addVfoWidget() showed the widget but did not grab it, and repositionVfoFlags() hides the (non-live) panel -- so for the gap the flag was neither a live widget nor a sprite, i.e. a brief blank. Measured (aether.perf FlagCreated -> FlagFirstGrab, 4 new non-live slices): ~41 ms gap (max 49), about half the 80 ms timer period, as expected from the random phase. grabFlagSprites() at the end of addVfoWidget() rasterizes the new flag's sprite synchronously (off-frame, safe). Measured after: ~3 ms (just the grab itself); the blank is gone. repositionVfoFlags() still positions the quad next frame. Adds the gated FlagCreated/FlagFirstGrab aether.perf trace used to measure it (zero cost when the category is off). Refs aethersdr#3695, aethersdr#3746. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed the diff against SpectrumWidget.cpp/.h at the current tree. This is a clean, well-scoped change — both edits stay within the GPU flag re-raster path, the measurements are reproducible behind the gated aether.perf trace, and all six CI checks are green (build / CodeQL / analyze / check-windows / check-macos / Qt Accessibility). Thanks for the thorough writeup and the one-change-at-a-time methodology.
I traced the trickier lifecycle paths and they hold up:
- Top-level fed through
eventFilter— installing the filter onwindow()means the top-level QWidget now flows througheventFilter(). The newWindowStateChangeblock handles it and falls through; the existing logic then computesvfoDescendant == false(the window is an ancestor ofthis, never aVfoWidget) and returns the base handler, so there's no cursor/drag side effect. Good. - Construction order —
m_gpuFlagModeis set at line 604 before the watcher/timer setup, andisVisible()is false at construction, so the timer correctly stays stopped until the firstShow. Matches the documented intent. - Reparent —
prepareForTopLevelChange()detaches and nulls the pointer;ParentChange/Showre-attach. Clean.
A few minor, non-blocking observations:
-
m_flagTimerFilteredWindowis a raw pointer to the watched top-level. In normal Qt teardown the ancestor window outlives this child (and Qt auto-removes the filter whenSpectrumWidgetis destroyed), so this is effectively safe today. But it's a latent footgun if a code path ever destroys the watched window without first routing throughprepareForTopLevelChange()—removeEventFilter()on a stale pointer would be UB. AQPointer<QWidget>(or aconnect(top, &QObject::destroyed, ...)to clear it) would harden this defensively. Optional. -
attachFlagTimerWindowWatcher()installs the filter even whenAETHER_NO_GPU_FLAGSis set (m_gpuFlagMode == false).updateFlagRefreshTimer()then always computespresent == false, so it's harmless, but you could early-return on!m_gpuFlagModein the attach helper to avoid filtering the window entirely in that mode. -
The immediate grab in
addVfoWidget()depends on the freshly-show()nVfoWidgetalready having a non-empty size —grabFlagSprites()skips any flag wheref->size().isEmpty(). Your 41 ms → ~3 ms measurement confirms it does have a size at that point in practice, so no action needed; just flagging the implicit ordering dependency in case the flag's sizing ever moves to a deferred layout pass.
None of these block merge. Nice work.
🤖 aethersdr-agent · cost: $4.3461 · model: claude-opus-4-8
ten9876
left a comment
There was a problem hiding this comment.
Traced the lifecycle paths independently and they hold up. The present predicate correctly separates the hidden (isVisible) and minimized (WindowStateChange watcher, since children stay isVisible under a minimized top-level) cases, and Show/Hide/ParentChange funnel the tab + float/dock-reparent cases through the same updateFlagRefreshTimer(). Verified that routing the top-level window through eventFilter is side-effect-free: the vfoDescendant walk starts at an ancestor of this, never matches a VfoWidget, and returns the base handler — per-event cost is a pointer compare + anyDragActive() + a one-hop parent walk. The immediate grabFlagSprites() in addVfoWidget() closes the blank gap and is a harmless no-op in the worst case (flag not yet sized).
Directly addresses the off-screen/minimized re-raster waste tracked in #3746: ~27 ms/s ongoing main-thread → 0 when minimized, visible-path cost unchanged. Measurements are one-change-at-a-time behind the gated aether.perf trace (Principle XI). The ③b dirty-skip was correctly measured-and-deferred (S-meter noise → negligible idle saving).
One non-blocking note: m_flagTimerFilteredWindow is a raw QWidget* — safe on today's teardown paths (prepareForTopLevelChange detaches + nulls it, Qt auto-removes the filter on destruction), but a QPointer would harden the removeEventFilter() calls against any future path that destroys the watched window without routing through prepareForTopLevelChange(). Worth a follow-up, not blocking.
Nice work @svabi79 — clean, well-scoped, and reproducibly measured.
## Summary Closes #3769. Defensive hardening raised in the #3764 review. `SpectrumWidget::m_flagTimerFilteredWindow` — the top-level window watched for minimize/restore (added in #3764 for the #3746 idle-pause) — was a raw `QWidget*`. It's safe on today's teardown paths: `prepareForTopLevelChange()` detaches the event filter and nulls it, and Qt auto-removes the filter when either object is destroyed. But the `removeEventFilter()` calls in `attachFlagTimerWindowWatcher()` and `prepareForTopLevelChange()` would be UB if a future code path ever destroyed the watched window *without* first routing through `prepareForTopLevelChange()` — the raw pointer would dangle non-null and pass the `if (m_flagTimerFilteredWindow && ...)` guard. `QPointer<QWidget>` fixes that for ~zero cost: a destroyed window reads back as null, so the guarded `removeEventFilter()` calls become automatic no-ops. ## What changed - `src/gui/SpectrumWidget.h` — `m_flagTimerFilteredWindow` is now `QPointer<QWidget>`; added `#include <QPointer>`. - No `.cpp` changes: every site (`== window()`, `!= this`, `= top`, `= nullptr`, `->removeEventFilter`, `watched == ...`) works unchanged via QPointer's implicit conversion to `QWidget*`. No behavior change — purely a dangling-pointer safety improvement. ## Test plan - [x] Local build passes (`cmake --build build --target AetherSDR`) — all comparison/assignment sites compile unchanged - [ ] Existing tests pass (CI) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Two of the idle re-raster follow-ups tracked in #3746 (itself a follow-up to the GPU slice-flag work in #3695). Refs #3746 — this does not close it: option (b) AA-consistency stays deferred per the issue's own rationale (ship (a) first, revisit after real-world exposure), and the dirty-skip was measured and deferred (see below).
③a — pause the flag re-raster timer when the panadapter isn't presented. The 12.5 Hz refresh timer re-rasterized every non-live slice flag every 80 ms regardless of whether the panadapter was on screen. It now runs only while the panadapter is visible and not minimized. A minimized top-level keeps child
isVisible() == true, so aWindowStateChangewatcher on the top-level handles minimize/restore;Show/HideandParentChange(float/dock reparent) handle the hidden-tab and reparent cases — all funnelled throughupdateFlagRefreshTimer(). The watcher reattaches on reparent and detaches inprepareForTopLevelChange().② — rasterize a new slice's flag sprite immediately. A newly created non-live flag had no GPU sprite until the next timer tick:
addVfoWidget()showed the widget but didn't grab it, andrepositionVfoFlags()hides the non-live panel — so for the gap the flag was neither a live widget nor a sprite (a brief blank).grabFlagSprites()at the end ofaddVfoWidget()closes it.Plus a gated
aether.perftrace (FlagGrab/FlagCreated/FlagFirstGrab, zero cost when the category is off) used to measure all of this — committed so the numbers are reproducible on any platform.Measured (i9-13900K / RTX 4090, 4 slices,
aether.perf)Each change made and measured one at a time.
Measured and deferred — dirty-skip (③b)
Also evaluated skipping the re-raster of a flag whose content is unchanged. Measured the skip ceiling by hashing each render: each slice's S-meter animates on its own noise floor, so even on a dead / dummy-load input only ~2.7 of 4 flags were ever byte-identical tick-to-tick (fewer on an active band). The realizable idle saving is negligible (≤ ~0.02 cores, idle-only), and a correct pre-render skip would need fragile per-widget dirty-tracking. Not worth it versus ③a, which already removes the larger hidden/minimized waste. Left deferred in #3746.
Constitution principle honored
Principle XI — Fixes Are Demonstrated. Every change has a before/after
aether.perfmeasurement (tables above), taken one change at a time.Test plan
QRhiWidget/QWidgetcode; cross-platform numbers welcomeChecklist
AppSettingscalls (N/A)MeterSmoother(N/A — no meter changes)