feat(rendering): unify render backend selection with live GPU/CPU switching#2782
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds global render-backend selection, capability detection, live planar pipeline remounting, WebGL context event wiring, demo updates, and tests around backend switching and capability probing. ChangesRender backend switching
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant init
participant PlanarViewport
participant eventTarget
User->>init: setRenderBackend(cpu/gpu/auto)
init->>PlanarViewport: updateRenderingPipeline()
PlanarViewport->>eventTarget: emit RENDER_BACKEND_CHANGED
PlanarViewport->>eventTarget: emit RENDER_PATH_ERROR
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/RenderingEngine/helpers/attachWebGLContextEvents.ts (1)
21-25: 📐 Maintainability & Code Quality | 🔵 TrivialInline cast masks a missing/incomplete
VtkOffscreenMultiRenderWindowtype.
getOpenGLRenderWindow?.()is cast to an ad-hoc{ getCanvas?: ... }shape instead of the type declaring this method. IfVtkOffscreenMultiRenderWindow's OpenGL render window type already exposesgetCanvas, this cast is unnecessary; if not, consider extending the shared type definition instead of casting at each call site.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/RenderingEngine/helpers/attachWebGLContextEvents.ts` around lines 21 - 25, The inline ad-hoc cast in attachWebGLContextEvents is hiding a missing or incomplete shared type for the OpenGL render window. Update the relevant VtkOffscreenMultiRenderWindow/OpenGL render window type definitions so the object returned by getOpenGLRenderWindow() correctly exposes getCanvas, then remove the local cast and use the typed method directly in attachWebGLContextEvents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.ts`:
- Around line 1763-1779: The render-path error dedup state is never cleared, so
a later identical failure after a successful render can be swallowed. Update the
successful render flow in PlanarViewport to reset lastRenderPathErrorKey
whenever rendering completes without error, while keeping reportRenderPathError
as the place that sets and dedups the key. Make sure the reset happens on the
success path that follows a recovered render so RENDER_PATH_ERROR is emitted
again on recurring failures.
In `@packages/core/src/RenderingEngine/helpers/attachWebGLContextEvents.ts`:
- Around line 31-39: The webglcontextlost handler in attachWebGLContextEvents
should prevent the default browser behavior so context restoration can proceed.
Update the existing canvas.addEventListener('webglcontextlost', ...) callback to
accept the event object and call event.preventDefault() before logging and
triggering Events.WEBGL_CONTEXT_LOST, keeping the rest of the
renderingEngineId/contextIndex flow unchanged.
---
Nitpick comments:
In `@packages/core/src/RenderingEngine/helpers/attachWebGLContextEvents.ts`:
- Around line 21-25: The inline ad-hoc cast in attachWebGLContextEvents is
hiding a missing or incomplete shared type for the OpenGL render window. Update
the relevant VtkOffscreenMultiRenderWindow/OpenGL render window type definitions
so the object returned by getOpenGLRenderWindow() correctly exposes getCanvas,
then remove the local cast and use the typed method directly in
attachWebGLContextEvents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 63030299-86d1-438f-abfe-b3fe249eac6a
📒 Files selected for processing (24)
packages/core/examples/genericRenderBackendSwitch/index.tspackages/core/src/RenderingEngine/ContextPoolRenderingEngine.tspackages/core/src/RenderingEngine/GenericViewport/GenericViewport.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarRenderPathDecisionService.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewportTypes.tspackages/core/src/RenderingEngine/GenericViewport/Planar/VtkImageMapperRenderPath.tspackages/core/src/RenderingEngine/GenericViewport/Planar/planarRenderPathSelector.tspackages/core/src/RenderingEngine/TiledRenderingEngine.tspackages/core/src/RenderingEngine/WebGLContextPool.tspackages/core/src/RenderingEngine/helpers/attachWebGLContextEvents.tspackages/core/src/enums/Events.tspackages/core/src/enums/RenderBackend.tspackages/core/src/enums/index.tspackages/core/src/index.tspackages/core/src/init.tspackages/core/src/types/Cornerstone3DConfig.tspackages/core/src/utilities/renderingCapabilities.tspackages/core/src/utilities/textureSupport.tspackages/core/test/planarRenderBackend.jest.jspackages/core/test/renderingCapabilities.jest.jstests/genericViewport/genericRenderBackendSwitch.spec.tstests/utils/compatibilityMode.tsutils/demo/helpers/exampleParameters.ts
💤 Files with no reviewable changes (1)
- packages/core/src/RenderingEngine/GenericViewport/Planar/planarRenderPathSelector.ts
| canvas.addEventListener('webglcontextlost', () => { | ||
| console.warn( | ||
| `CornerstoneRender: WebGL context lost (renderingEngine=${renderingEngineId}, context=${contextIndex})` | ||
| ); | ||
| triggerEvent(eventTarget, Events.WEBGL_CONTEXT_LOST, { | ||
| renderingEngineId, | ||
| contextIndex, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby context
sed -n '1,220p' packages/core/src/RenderingEngine/helpers/attachWebGLContextEvents.ts
# Find related WebGL context event handling and restore logic
rg -n "webglcontextlost|webglcontextrestored|preventDefault\(|WEBGL_CONTEXT_LOST|WEBGL_CONTEXT_RESTORED|restoreContext" packages/core/src packages -g '!**/dist/**' -g '!**/build/**'
# Look for any vtk.js integration points that might already manage the event
rg -n "vtk|vtk.js|context lost|context restored|preventDefault" packages/core/src packages -g '!**/dist/**' -g '!**/build/**'Repository: cornerstonejs/cornerstone3D
Length of output: 1977
🌐 Web query:
WebGL context lost event preventDefault restore browser attempts to restore only if preventDefault is called spec
💡 Result:
Yes, that is correct. Under the WebGL specification, the browser will only attempt to restore a WebGL context if the default behavior of the webglcontextlost event is prevented [1][2][3]. By calling event.preventDefault within a listener for the webglcontextlost event, an application signals to the browser that it intends to handle the context loss and subsequent restoration [1][3]. If this method is not called, the browser treats the context loss as permanent and will not automatically attempt to restore it [1][4][5]. This requirement was established to prevent unpredictable application behavior [3]. When a WebGL context is lost, all associated resources—such as textures, buffers, framebuffers, and shaders—become invalid [1][6]. If a browser were to automatically restore the context without the application being prepared to reinitialize its state and resources, the application would likely enter an unstable or broken state [3]. Requiring the use of preventDefault ensures that developers explicitly opt-in to the recovery process [1][3].
Citations:
- 1: https://wikis.khronos.org/webgl/HandlingContextLost
- 2: https://www.khronos.org/webgl/public-mailing-list/public_webgl/1104/msg00065.php
- 3: https://www.khronos.org/webgl/public-mailing-list/public_webgl/1104/msg00067.php
- 4: https://registry.khronos.org/webgl/extensions/WEBGL_lose_context/
- 5: https://bugzilla.mozilla.org/show_bug.cgi?id=1144115
- 6: https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/webglcontextrestored_event
Call event.preventDefault() on webglcontextlost. Without it, the browser won’t attempt to restore the context, so WEBGL_CONTEXT_RESTORED can’t fire from this path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/RenderingEngine/helpers/attachWebGLContextEvents.ts` around
lines 31 - 39, The webglcontextlost handler in attachWebGLContextEvents should
prevent the default browser behavior so context restoration can proceed. Update
the existing canvas.addEventListener('webglcontextlost', ...) callback to accept
the event object and call event.preventDefault() before logging and triggering
Events.WEBGL_CONTEXT_LOST, keeping the rest of the
renderingEngineId/contextIndex flow unchanged.
- Updated CpuImageSliceRenderPath to include modality handling for fallback viewports. - Refactored PlanarRenderPathDecisionService to streamline backend resolution logic. - Improved PlanarViewport to manage render path errors more effectively and trigger events on rendering pipeline changes. - Added new RENDERING_PIPELINE_CHANGED event to notify consumers of actor replacements after viewport rebuilds. - Enhanced rendering capability detection to re-probe when WebGL2 availability changes. - Introduced compatibility checks for actor entries in labelmap render plans to ensure correct remounting after backend switches. - Added tests for CPU image render path and backend resolution behaviors.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/examples/genericRenderBackendSwitch/index.ts (1)
490-553: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated get-viewport/null-check/loop boilerplate across button handlers.
The "Next Image", "Zoom/Pan", and other toolbar handlers each repeat the same "iterate ids → getViewport → skip if falsy" pattern. Consider extracting a small helper (e.g.
withViewports(ids, fn)) to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/examples/genericRenderBackendSwitch/index.ts` around lines 490 - 553, The toolbar handlers in genericRenderBackendSwitch repeat the same viewport-iteration and null-check boilerplate, making the button callbacks harder to read and maintain. Extract a small shared helper around getViewport usage (for example, a utility that takes viewport IDs and invokes a callback for each resolved viewport) and refactor the "Next Image (stacks)", "Set VOI Range", and "Apply Zoom And Pan" onClick handlers to use it instead of open-coded loops and checks.packages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/createLabelmapRenderPlan.ts (1)
69-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider unit-testing the new remount-compatibility gate.
needsRemountnow forces a remount when any actor entry failsisActorEntryCompatible, andgetActorEntryRenderModeis the load-bearing extraction for that check across bothlegacyVolumePlan.tsandvolumeSliceImageMapperPlan.ts. None of the files in this batch include a test exercising this new incompatibility branch (e.g., UID-stable but shape-incompatible actor entry forcing a remount).Also applies to: 106-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/createLabelmapRenderPlan.ts` around lines 69 - 83, Add a unit test for the new remount-compatibility path in needsRemount, covering the case where an actor entry keeps the same UID but fails isActorEntryCompatible and therefore forces a remount. Use getActorEntryRenderMode as the key extraction point in the test setup, and exercise the affected plan logic that depends on it (including legacyVolumePlan and volumeSliceImageMapperPlan behavior) so the incompatibility branch is explicitly verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/examples/genericRenderBackendSwitch/index.ts`:
- Around line 490-553: The toolbar handlers in genericRenderBackendSwitch repeat
the same viewport-iteration and null-check boilerplate, making the button
callbacks harder to read and maintain. Extract a small shared helper around
getViewport usage (for example, a utility that takes viewport IDs and invokes a
callback for each resolved viewport) and refactor the "Next Image (stacks)",
"Set VOI Range", and "Apply Zoom And Pan" onClick handlers to use it instead of
open-coded loops and checks.
In
`@packages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/createLabelmapRenderPlan.ts`:
- Around line 69-83: Add a unit test for the new remount-compatibility path in
needsRemount, covering the case where an actor entry keeps the same UID but
fails isActorEntryCompatible and therefore forces a remount. Use
getActorEntryRenderMode as the key extraction point in the test setup, and
exercise the affected plan logic that depends on it (including legacyVolumePlan
and volumeSliceImageMapperPlan behavior) so the incompatibility branch is
explicitly verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 169219b2-fe63-485e-b0b1-b8b0b102fc81
📒 Files selected for processing (17)
packages/core/examples/genericPetRenderBackendGrid/index.tspackages/core/examples/genericRenderBackendSwitch/index.tspackages/core/src/RenderingEngine/GenericViewport/Planar/CpuImageSliceRenderPath.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarRenderPathDecisionService.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.tspackages/core/src/enums/Events.tspackages/core/src/init.tspackages/core/src/utilities/renderingCapabilities.tspackages/core/test/planarComputedCamera.jest.jspackages/core/test/planarRenderBackend.jest.jspackages/core/test/renderingCapabilities.jest.jspackages/tools/src/eventListeners/segmentation/renderingPipelineChangedListener.tspackages/tools/src/init.tspackages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/createLabelmapRenderPlan.tspackages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/legacyVolumePlan.tspackages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/types.tspackages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/volumeSliceImageMapperPlan.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/core/test/renderingCapabilities.jest.js
- packages/core/src/utilities/renderingCapabilities.ts
- packages/core/src/enums/Events.ts
- packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.ts
- packages/core/test/planarRenderBackend.jest.js
- packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarRenderPathDecisionService.ts
- packages/core/src/init.ts
…r overlays Two live render-backend switch fixes from review: - GenericViewport.addLoadedData now mounts the replacement attachment before tearing down the existing one. Teardown-first left a dead binding whose recorded renderMode made a later remount (switching the backend back after a failed or stale swap) skip as a no-op, blanking the display set. - Overlays mounted without an explicit renderBackend now inherit the source binding's mounted backend instead of the global configuration. Source and overlays must share a canvas (each backend skips the other's actors), and the source may be pinned per-mount away from the global backend. The swap loop remounts the source binding first so overlays inherit the new backend during a switch.
|
🔥🔥🔥🔥🔥🔥🔥 |
wayfarer3130
left a comment
There was a problem hiding this comment.
Just the one comment about the startup time. Otherwise I think this is a solid improvement to the stability of CS3D viewports.
Address review feedback on probe startup cost: the six texture format probes previously each created their own canvas, WebGL2 context and shader program. They now share a single context and program, with a clear between probes so results cannot leak across the shared framebuffer, and per-probe textures are deleted after readback. Measured in Chrome on Apple Silicon: the probe run drops from ~20-70ms to ~5-16ms, and it only executes on the first page load for a given renderer/browser combination since results are persisted in localStorage (keyed by renderer string, WebGL2 availability and probe version); subsequent loads read the cached profile and create no probe contexts at all.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/utilities/textureSupport.ts (1)
44-104: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLeak the WebGL2 context on shader/program failure paths.
createProbeContext()creates a canvas + WebGL2 context, but if shader compilation (lines 79, 86) or program linking (line 95) fails, the function returnsnullwithout releasing the context (noWEBGL_lose_contextcall, no shader/program deletion). Unlike the success path ingetSupportedTextureFormats(), which explicitly loses the context at the end, these failure branches leave the context alive until GC. Repeated failures (e.g. a driver that fails to compile the point-sprite shader) could accumulate live contexts toward the browser's WebGL context limit.🔧 Proposed fix
+ const loseContext = () => gl.getExtension('WEBGL_lose_context')?.loseContext(); + const vertexShader = gl.createShader(gl.VERTEX_SHADER); gl.shaderSource(vertexShader, vs); gl.compileShader(vertexShader); if (!gl.getShaderParameter(vertexShader, gl.COMPILE_STATUS)) { + loseContext(); return null; } const fragmentShader = gl.createShader(gl.FRAGMENT_SHADER); gl.shaderSource(fragmentShader, fs); gl.compileShader(fragmentShader); if (!gl.getShaderParameter(fragmentShader, gl.COMPILE_STATUS)) { + loseContext(); return null; } const program = gl.createProgram(); gl.attachShader(program, vertexShader); gl.attachShader(program, fragmentShader); gl.linkProgram(program); if (!gl.getProgramParameter(program, gl.LINK_STATUS)) { + loseContext(); return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/utilities/textureSupport.ts` around lines 44 - 104, createProbeContext() returns null on shader compilation or program link failures without cleaning up the WebGL2 context, which can leak live contexts. Update the failure paths in createProbeContext() to explicitly release resources before returning null: delete any created shaders/program and lose the context via WEBGL_lose_context, mirroring the cleanup approach used in getSupportedTextureFormats(). Ensure every early return after createShader, compileShader, or linkProgram failure funnels through the same teardown logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/utilities/textureSupport.ts`:
- Around line 44-104: createProbeContext() returns null on shader compilation or
program link failures without cleaning up the WebGL2 context, which can leak
live contexts. Update the failure paths in createProbeContext() to explicitly
release resources before returning null: delete any created shaders/program and
lose the context via WEBGL_lose_context, mirroring the cleanup approach used in
getSupportedTextureFormats(). Ensure every early return after createShader,
compileShader, or linkProgram failure funnels through the same teardown logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e8790715-d90d-418b-b5db-99a7efd79c1d
📒 Files selected for processing (1)
packages/core/src/utilities/textureSupport.ts
…runs A transient probe failure (live-context limit hit, GPU process restarting, context lost mid-probe) used to produce an all-false format profile that was persisted in localStorage under the valid renderer key, silently pinning the degraded path on every later load. getSupportedTextureFormats now returns null when the probes could not actually run, and detection skips persisting that result, so a bad run costs one re-probe on the next load instead of poisoning the cache.
Context
GenericViewport's planar render-path selection was spread across several ad-hoc
signals: a
gpuTierconfig, per-mountforceCPU, byte-sizecpuThresholds,and scattered WebGL/texture probes in
init.ts. There was no first-class way toexpress "render this on GPU / CPU / auto", no way to switch backends at runtime
without a page reload, and no signal for applications to react to GPU
degradation (context loss).
Changes & Results
Render backend model
RenderBackendenum (gpu|cpu|auto) withrenderBackendconfigurable globally (
rendering.planar.renderBackend) and per display set(mount option). Replaces
gpuTier,forceCPU, andcpuThresholds.getRenderBackend()/getEffectiveRenderBackend(override?)/setRenderBackend()centralize the precedence ladder (per-mount pin > globalpin >
autoresolved from capability detection).setUseCPURendering/resetUseCPURenderingare kept but deprecated in favor ofsetRenderBackend.Capability detection
utilities/renderingCapabilitiesprobes WebGL/WebGL2 availability,MAX_TEXTURE_SIZE, the unmasked renderer string, software-rasterizerdetection, and per-format texture support (norm16/float/halfFloat draw +
readback). Results are memoized in-process and cached in
localStorage,keyed by renderer string, WebGL2 availability, and a probe version.
Live backend switching
setRenderBackend()live-swaps every mounted GenericViewport's render pathsin place — viewport ids, mounted data, cameras, presentation state, and tool
annotations are preserved; only the render paths rebuild. Reversible in both
directions at runtime.
RENDER_BACKEND_CHANGED,WEBGL_CONTEXT_LOST/WEBGL_CONTEXT_RESTORED,RENDER_PATH_ERROR, andRENDERING_PIPELINE_CHANGED. Cornerstone never switches backends on its own;applications listen and call
setRenderBackend('cpu')themselves.Segmentation rehydration across a switch
rebuilt viewport actors are re-styled via a
RENDERING_PIPELINE_CHANGEDlistener, and the labelmap render plans force a remount when a surviving
actor has an incompatible shape (image-mapper vs volume), so stack, MPR
volume, and PET labelmaps all survive gpu↔cpu switches.
Testing
genericRenderBackendSwitch(andgenericPetRenderBackendGrid)live-switch the backend of stack, MPR, and PET viewports; verified all three
labelmaps and annotations survive gpu→cpu→gpu round-trips and CPU slice
scrolling.
planarRenderBackend.jest.js,renderingCapabilities.jest.js,labelmap render-plan specs; e2e
genericRenderBackendSwitch.spec.ts.Summary by CodeRabbit