Repository navigation
fix(spectrum): release replaced waterfall textures after the frame (#6078). Principle XI. - #6093
Conversation
…ot inside it renderGpuFrame()'s waterfall resize transaction deleted the old textures and bindings at once. render() runs initialize() in the same call, and initialize() queues uploads into those textures; when the retained waterfall image is under the 64-row floor initWaterfallPipeline() applies, the transaction runs in that same frame. Qt's D3D11 backend keeps raw resource pointers in its command list and releases on destroy, so the queued uploads ran against released textures (aethersdr#6078). deleteLater() defers the release past the frame. Experiment build: not yet run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…esize too The fallback branch of the same resize transaction released the row-frequency pipeline, bindings, textures and sampler at once through releaseWaterfallFramePipelineResources(). They carry the same queued uploads as the textures in the success branch, so they get the same end-of-frame release; the helper then only resets the pipeline mode. Reachable with AETHER_AUTOMATION_FORCE_WF_FRAME_RESIZE_FAILURE. Not yet run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
Issue fit
Yes, for the defect the issue now names. I walked the root-cause chain in the head checkout and it holds end to end: render() calls initialize(cb) then renderGpuFrame(cb, …) with the same command buffer (SpectrumWidget.cpp:15135-15165); initialize() queues batch->uploadTexture() into exactly m_wfGpuTex, m_wfSupplementalGpuTex and m_wfFrameTex and commits with cb->resourceUpdate(batch) (:13480-13513); initWaterfallPipeline() floors the texture at 64 × 64 for a shorter retained image (:12566-12569); and the resize transaction at :13640 is reached in that same frame, since desiredWidth/Height come straight from m_waterfall with no floor. The three resources the old code deleted in-frame are precisely the three initialize() uploaded into. Nothing in the diff is ornamental.
I also closed the issue's one open "Not known" — "which statement mishandles the rows between the image height and the 64-row texture". It is not an upload-size bug: prepareWaterfallFrameUpload() sizes the buffer max(visibleHeight, m_wfGpuTexH) and pads the floor rows with the current center/bandwidth (:13318-13360), and the color/supplemental uploads are legal partial uploads into a taller texture. That is consistent with this PR's account — the mismatch is benign, the in-frame release was not.
The mechanism claim I could check: qrhi_p.h:178-183 in the Qt 6.11.2 kit here is
void addDeleteLater(QRhiResource *res)
{ if (inFrame) pendingDeleteResources.insert(res); else delete res; }so deleteLater() defers only while a frame is recording, deletes immediately otherwise, and pendingDeleteResources is a QSet (double-deferral of one pointer is harmless). What I could not check here: that the set is flushed after the backend's endOffscreenFrame (that is in qrhi.cpp, not shipped in this kit), and the PR's specific statement that the D3D11 backend alone lacks a deferred-release queue. Those rest on your 160-click before/after runs, not on anything I verified. Everything below is reasoned from source; I have no build and no GUI here.
What stays unfixed: #6078's Expected has two halves — "pop-out and dock-back keep working" (this PR) and "if the D3D11 device is lost, the pan reports a render failure instead of the process crashing". The thread's triage comment is entirely about that second half (device-loss latch, gating update() drivers, the VCRUNTIME140/QBackingStoreDefaultCompositor crash path with no AetherSDR frame in it), and the PR correctly scopes it out. Fixes #6078 will auto-close the issue carrying that work. Worth either demoting to Refs #6078 or filing the containment follow-up before merge — maintainer's call, not a blocker on this code. Nothing in the thread rules against fixing the trigger instead; the one alternative the triage floated (single-init dock) was measured and did not help.
Scope
| File / group | What it changes | Claimed by title/body | Verdict |
|---|---|---|---|
src/gui/SpectrumWidget.cpp:13709-13718 |
releaseAfterFrame lambda + comment; new qDebug resize line |
Yes (commit 1a2d0545, the log line is quoted in the Proof section) |
In scope |
src/gui/SpectrumWidget.cpp:13731-13737 |
three in-frame deletes → deleteLater() |
Yes | In scope |
src/gui/SpectrumWidget.cpp:13738-13747 |
fallback branch: five std::exchange + deferred release before the helper |
Yes (commit 7021c4e0) |
In scope |
src/gui/SpectrumWidget.cpp:13753-13754 |
oldLegacySrb / oldColorTexture → deferred |
Yes | In scope |
One file, +27/−5, no new public surface, no settings or protocol touched, no guard or early return removed, CHANGELOG.md untouched (correct — don't add one). The checklist's "no unrelated files" self-certification is accurate. No sibling implementation left behind: grep finds no other delete between renderGpuFrame's start and releaseResources(), and the five setPixelSize()+create() recreates (:14005, :14011, :14035, :14783, :14851) hit textures initialize()'s batch never names, each one preceding its own upload later in the same frame (:14360, :14382, :14794, :14882) — so the body's "these are the only two places" claim checks out. No test changes, no sockets, no fake peers; a GPU resource-lifetime fault has no socket-free seam and the "bridge-only proof" framing is the honest one. Not a preference change: no default, no UI element, no workflow moved.
Blockers
None.
Nits (non-blocking)
- The five
std::exchangelines duplicatereleaseWaterfallFramePipelineResources()'s body (inline comment at:13741). BecauseaddDeleteLateralready falls back to plaindeletewhen no frame is recording, the helper itself could usedeleteLater()and both call sites — this one andreleaseResources()at:15171— would be correct with no duplication. As written, anyone who adds a sixth resource to the helper silently reintroduces the in-framedeleteon this path. Fixes #6078vs. the containment half of that issue's Expected, as above.- The new
qDebug()line fires once per waterfall resize, so a dock burst or a resize settle writes several; it is default-category like its siblings (:12757,:13405), which is what makes your proof log readable in a bundle, so I would not change the category — just be aware it competes for the bundle tail with whatever else is happening during a reparent.
What I tried to break
- Double release across the two branches.
oldFrameSrb/oldFrameTexture/oldSupplementalTextureare the same objects the fallback branchstd::exchanges out of the members. Each path releases them exactly once, and the helper'sdelete nullptrafter the exchange is a no-op;pendingDeleteResourcesbeing aQSetmakes even a repeat insert harmless. No double free. - Resources still deleted inside the frame. On every failure path (
!legacyResizeReady, row-texturecreate()failure,frameSrb->create()failure) theunique_ptrs destruct immediately — but those are the textures/SRBs created this frame, never enqueued on the batch and never bound in a recorded pass. Safe. - A stale SRB surviving the resize. If
m_wfPipelineMode == RowFrequencyFramesbutrowPipelineWasActivewere false,m_wfFrameSrbwould keep sampling the color texture that was just released, and the draw gate at:14944-14951would still use it. I could not reach that state:m_wfSupplementalGpuTexis only assigned at:12749/:13731(alongside the rest) or nulled by the helper, which also forcesm_wfPipelineMode = Legacyandm_wfFrameTexReady = false. The deferral makes this strictly safer than before, not worse. - The live-resize preview path.
render()can also callrenderGpuFrame(cb, m_resizePresentationSize, true)during the settle timer — same function, same transaction, so it is covered by the same change; the preview flag does not bypass the resize block. - Compile portability.
std::exchangeis covered (<utility>at line 94), and the hunk is inside#ifdef AETHER_GPU_SPECTRUM. All 5 checks are green on7021c4e0, but noteCMakeLists.txt:640-647silently setsAETHER_GPU_SPECTRUM OFFwhenQt6GuiPrivateis absent, so green CI is only evidence this compiled wherever that module was found — the macOS/Linux float/dock smoke you list as untested is still the real gap, anddeleteLater()is backend-agnostic so I see no reason to expect a platform difference.
Recommendation
Approve with nits. The diff is small, the root cause is traceable in the source rather than asserted, the before/after runs cover both adapters plus the forced-failure branch, and I could not construct a double free, a leak, or a stale binding out of either hunk. The two things I would like settled before merge are bookkeeping, not code: collapse the duplicated release list into the helper (or say why not), and decide whether this PR should close #6078 given the device-loss containment work the thread asks for is explicitly not in it. Thanks for the measurement table — the 64/67-row boundary is what made this reviewable.
🤖 aethersdr-agent · cost: $4.9145 · model: claude-opus-5
|
Two line references in my review above are wrong: 🤖 aethersdr-agent · cost: $5.5679 · model: claude-opus-5 |
Merge current upstream main and keep the row-frequency cleanup resource list in one helper. QRhiResource::deleteLater preserves queued frame commands during resize fallback and deletes immediately during idle cleanup.
|
Landing disposition at The operator explicitly chose keep |
jensenpat
left a comment
There was a problem hiding this comment.
Approve exact head 85943b0482f0aad5a8bdfcb1bb6558d2b17cf07f after manual adversarial review, remediation and runtime proof.
- Fixed the duplicated row-pipeline cleanup list in
SpectrumWidget.cpp:12538: the existing helper uses null-guardeddeleteLater()andstd::exchange, shared by in-frame fallback and idle teardown. The final-head GPU-enabled macOS build passed, four focused headless socket-free CTests passed, and the forced-fallback Metal bridge run exercised 61 fallback events while rendering remained healthy. - Closed the flush-order/raw-pointer caveats with public Qt v6.12.0 source: backend
endOffscreenFrame()precedes draining pending resources; the D3D11 queued update structure retains raw destination pointers. Both inline threads have evidence replies and are resolved; no blocking reviews needed dismissal. - Final-head isolated
DEMO-0001, TX-disabled Metal smoke: normal and forced-fallback runs each passed 40 alternating two-panadapter float/dock clicks and four window resizes. Logs captured 54 normal and 51 forced-fallback transitions from the 64-row floor to a smaller image. Screenshots showed spectrum/waterfall; both instances disconnected and exited cleanly. The shared helper's null/idle paths and failure branches were also audited in source. - Operator decision: retain
Fixes #6078and close the issue with this trigger fix. Arbitrary device-loss containment is outside the change and is stated in the body. Resize logging is retained as reviewed. No settings, radio protocol, defaults, workflows, changelog or architecture ratchets changed; all four local static-gate findings match the merge base.
Author-supplied Windows before/after proof is at 7021c4e0; Windows runtime smoke at this final head and Linux rendering smoke were not rerun. Hosted Windows/Linux build checks are separate CI evidence. The automated code-review skill was unavailable; this was a manual adversarial pass. Fresh hosted CI and current-main freshness are verified separately before arming the merge.
There was a problem hiding this comment.
Approve exact head 150059be0321c35d132a271d0d5917c1b118633e after manual adversarial review, remediation and runtime proof.
- Fixed the duplicated row-pipeline cleanup list in
SpectrumWidget.cpp:12538: the existing helper uses null-guardeddeleteLater()andstd::exchange, shared by in-frame fallback and idle teardown. The final-head GPU-enabled macOS build passed, four focused headless socket-free CTests passed, and the forced-fallback Metal bridge run exercised 61 fallback events while rendering remained healthy. - Closed the flush-order/raw-pointer caveats with public Qt v6.12.0 source: backend
endOffscreenFrame()precedes draining pending resources; the D3D11 queued update structure retains raw destination pointers. Both inline threads have evidence replies and are resolved; no blocking reviews needed dismissal. - Final-head isolated
DEMO-0001, TX-disabled Metal smoke: normal and forced-fallback runs each passed 40 alternating two-panadapter float/dock clicks and four window resizes. Logs captured 51 normal and 51 forced-fallback transitions from the 64-row floor to a smaller image. Screenshots showed spectrum/waterfall; both instances disconnected and exited cleanly. The shared helper's null/idle paths and failure branches were also audited in source. - Operator decision: retain
Fixes #6078and close the issue with this trigger fix. Arbitrary device-loss containment is outside the change and is stated in the body. Resize logging is retained as reviewed. No settings, radio protocol, defaults, workflows, changelog or architecture ratchets changed; all four local static-gate findings match the merge base.
Author-supplied Windows before/after proof is at 7021c4e0; Windows runtime smoke at this final head and Linux rendering smoke were not rerun. Hosted Windows/Linux build checks are separate CI evidence. The automated code-review skill was unavailable; this was a manual adversarial pass. Fresh hosted CI and current-main freshness are verified separately before arming the merge.
Revalidated after cleanly incorporating the two intervening main commits (5766bb13, a7eccda4). Earliest final-head CI run creation is 2026-10-02T20:20:50Z, later than the incorporated source base’s 2026-10-02T20:15:54Z commit time. Required checks are pending. Auto-merge is confirmed armed at this head. An initial request with an explicit contributor --author-email override was rejected with a misleading verified-email message; removing that override succeeded. No operator account change was needed. All required checks must pass before merge.
After the final-head CI runs were created, main gained 889bbe8d23735b60ce08e8f7bd44d1a45b4b34ea (20:35:38Z), docs(bridge): automation identity also picks the radio's restore. Principle VIII. (#6071). Its only changed file is docs/automation-bridge.md; no src/, tests/, tools/, workflow or CMake changes. The pr-land docs-only freshness exemption applies. Merge-tree is clean and preserves that documentation update.
|
Linux rendering smoke at Setup: Ubuntu 24.04, X11, Qt 6.12.0 (pinned kit from
Between — authored by agent (Claude Code) on behalf of @skerker |
Summary
Fixes #6078.
On Windows, docking a floated panadapter lost the D3D11 device (NVIDIA: driver error and TDR reset; Intel: crash in the driver) when the pan's retained waterfall image was under 64 rows. The waterfall resize transaction in
SpectrumWidget::renderGpuFrame()deleted the textures it replaced at once, in a frame that had already queued uploads into them. This PR releases them after the frame.Root cause. In the first paint after a pan changes top-level window:
SpectrumWidget::render()callsinitialize(cb)and thenrenderGpuFrame(cb, …)with the same command buffer.initialize()→initWaterfallPipeline()sizes the waterfall textures from the retained image, raised to at least 64 × 64, and queues the initial uploads into them.renderGpuFrame()compares the image with the texture. When the image is under the floor (measured: image 764 × 59, texture 764 × 64) they differ, so the resize transaction runs in that same frame, creates textures at the image size, anddeletes the old ones.ID3D11Resource *dstin each queued upload command and has no deferred release (qrhid3d11_p.h, the same in Qt 6.8.3 and 6.12), so they execute at frame end against released objects.With a retained image of 64 rows or more the texture is built at the image size, step 3 does not run in that frame, and nothing is released. That boundary is measured: 67 rows survives, the 64-row floor fails (table in #6078).
The change (one file,
src/gui/SpectrumWidget.cpp):QRhiResource::deleteLater()so queued uploads finish before destruction. The debug resize line records the texture dimensions.releaseWaterfallFramePipelineResources()owns the five-resource cleanup list for both the row-texture resize fallback and idle teardown. It clears member pointers withstd::exchangeand usesdeleteLater(), which defers during a frame and deletes immediately outside one. Both call sites share this lifetime rule without duplicating the resource list.The 64 × 64 floor in
initWaterfallPipeline()is unchanged, so the size mismatch still occurs; it is the in-frame release that was unsafe. These are the only two places inrenderGpuFrame()that release resources with commands already recorded on the frame's command buffer. The in-placesetPixelSize()+create()re-sizes of the overlay, frequency-overlay, background, 3DSS and FFT-column textures also release native textures, but they run before the frame's own batch is recorded, andinitialize()queues nothing into those textures.Containing an unrelated device loss after it has happened is outside this change. The operator explicitly chose to retain
Fixes #6078and close that issue with this trigger fix.Possibly the same bug elsewhere. The attached logs of #5990 / #5935 (v26.9.5 and v26.8.4, AMD Radeon HD 7000, right-click → Pop out) end, in 5 of 7 files, inside the re-init after the pop-out, with
waterfall row-frequency pipeline created 1624 x 64: the retained image's width and the 64-row floor. The logs of #5915 (Intel Arc, leaving Minimal Mode; closed by #5916) showwaterfall row-frequency pipeline created 64 x 198, the floor on the width. That is a signature match, not a reproduction: those logs do not carry the image size. The reporter of #5990 can confirm on a build with this change. Refs #5990.Constitution principle honored
Principle XI — Fixes Are Demonstrated. The before/after below is the implementer's evidence; the maintainer's reproduction is four steps (in #6078's body) on any Windows build.
Proof
Proof shape: automation bridge only. A GPU resource-lifetime fault has no socket-free test seam; no ctest is added.
Author evidence at
7021c4e0: one Windows 11 laptop with NVIDIA and Intel adapters. The Windows runs below were supplied by @skerker; they have not been rerun at the landing head. Independent macOS validation of the final head follows the author evidence. Linux rendering smoke has not been run.All author runs: Windows 11 Pro 10.0.26200, x64, i9-14900HX, RTX 5060 Laptop GPU (driver 32.0.15.8205) + Intel UHD Graphics (32.0.101.7076), FLEX-8400 RX only (
AETHER_AUTOMATION_NO_TX=1), an isolated copy of my settings (SpectrumSplitRatio0.787, default floating size → 59-row floated waterfall, read withget rhi). Script:invoke "pan 0/panFloatToggle"/invoke "pan 1/panFloatToggle"alternately, 1.2 s apart.38792429(Qt 6.12) and v26.9.5 portable (Qt 6.8.3)38792429QT_D3D_ADAPTER_INDEX=1)igd10um64xe.DLLunderd3d11!…UpdateSubresource←QRhi::endOffscreenFrame, on the 1st dock7021c4e0(bridgepingSHA)764 x 64, 80764 x 64 -> 764 x 59lines, no0x887a0005line, no driver event, no crash7021c4e0get rhi: Intel(R) UHD Graphics)764 x 64, 80764 x 64 -> 764 x 59lines, no crash7021c4e0,AETHER_AUTOMATION_FORCE_WF_FRAME_RESIZE_FAILURE=1using legacy waterfall pipeline: row texture resize transaction failed), no loss, no driver event, no crash. Main was not run with this switch, so this shows the changed branch is safe, not that it failed before.At the first commit
1a2d0545the same script survived 160 clicks on each adapter: 80 inits at764 x 64, 80764 x 64 -> 764 x 59lines, no0x887a0005line, no driver event, no crash.The fix acting, from the after-run's log (the dock that lost the device in every earlier run):
Evidence bundle: head
7021c4e0· whole logs and driver records for the three head runs, one before pair on unchanged main (NVIDIA loss, Intel crash with the debugger stack), the bridge scripts, both commits as a patch · aethersdr-pr6093-waterfall-resize-release-2026-10-02.zip (sha25633a5822a8728fdfd89bb794d6b43190ec36aad77370198a860928be41f2467fb)Landing validation at
150059be0321c35d132a271d0d5917c1b118633eUpstream source base
a7eccda4was merged without rewriting the contributor commits. The final PR diff remains one source file. The cleanup and clean integration commits are signed and GitHub verified. Main advanced twice during the pass: split-QSY handling (5766bb13) and WNB startup replay removal (a7eccda4). Both were incorporated before the final-head build and runtime proof.cmake --build build -j22 --target AetherSDR spectrum_preview_logic_test waterfall_history_buffer_test waterfall_time_markers_test waterfall_rate_testpassed.spectrum_preview_logic_test,waterfall_history_buffer_test,waterfall_time_markers_test,waterfall_rate_test— 4/4 passed withQT_QPA_PLATFORM=offscreen.DEMO-0001, TX disabled: 40 alternating float/dock clicks across two panadapters and four window resizes passed. The log contains 51 transitions from the 64-row texture floor to a smaller retained waterfall. Bridge state remained healthy (rendererFailed: false, texture/image dimensions matched after settling).AETHER_AUTOMATION_FORCE_WF_FRAME_RESIZE_FAILURE=1: 40 float/dock clicks and four window resizes passed, including 61 forced fallback events and 51 transitions from the 64-row floor. This exercises the shared cleanup helper during a recording frame. Both screenshots showed spectrum/waterfall rendering; both instances disconnected and exited cleanly.git diff --checkpassed.The four CTests are adjacent logic coverage, not a reproduction of the GPU lifetime fault. The author’s Windows before/after table is the regression reproduction; the final-head Metal smoke independently covers normal, forced-fallback, two-panadapter, resize and idle-teardown paths. No live hardware was used during landing validation.
After the final-head CI runs were created, main gained
889bbe8d23735b60ce08e8f7bd44d1a45b4b34ea(20:35:38Z),docs(bridge): automation identity also picks the radio's restore. Principle VIII. (#6071). Its only changed file isdocs/automation-bridge.md; nosrc/,tests/,tools/, workflow or CMake changes. The pr-land docs-only freshness exemption applies. Merge-tree is clean and preserves that documentation update.Not tested
7021c4e0head. Hosted Windows CI is separate build evidence.Test plan
7021c4e0.Checklist
docs/COMMIT-SIGNING.md)AppSettingscalls — n/a, no settings touchedMeterSmoother— n/a, no meter UICHANGELOG.mduntouchedSource integration base: upstream main
a7eccda4ec9341485982091734301b8b116f21caincorporated in the signed landing commit; no unrelated PR diff.Original fix and Windows evidence by @skerker, authored with Claude Code. Landing cleanup and macOS validation by Codex under @jensenpat.
Post-merge verification: squash commit
712bd84994f55b8c484e3cb95a9e02b469242508has the expected tree and a valid GitHub signature. Platform CI and Full Suite passed on that exact main commit. C++ CodeQL and System Libraries Canary were still running at 21:57Z. #6078 closed with the trigger fix, as requested.Generated with OpenAI Codex (GPT-6 Astra)