Skip to content

feat(rendering): unify render backend selection with live GPU/CPU switching#2782

Merged
sedghi merged 5 commits into
mainfrom
cornerstoneCPUGPUDetect
Jul 7, 2026
Merged

feat(rendering): unify render backend selection with live GPU/CPU switching#2782
sedghi merged 5 commits into
mainfrom
cornerstoneCPUGPUDetect

Conversation

@sedghi

@sedghi sedghi commented Jul 3, 2026

Copy link
Copy Markdown
Member

Context

GenericViewport's planar render-path selection was spread across several ad-hoc
signals: a gpuTier config, per-mount forceCPU, byte-size cpuThresholds,
and scattered WebGL/texture probes in init.ts. There was no first-class way to
express "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

  • New RenderBackend enum (gpu | cpu | auto) with renderBackend
    configurable globally (rendering.planar.renderBackend) and per display set
    (mount option). Replaces gpuTier, forceCPU, and cpuThresholds.
  • getRenderBackend() / getEffectiveRenderBackend(override?) /
    setRenderBackend() centralize the precedence ladder (per-mount pin > global
    pin > auto resolved from capability detection). setUseCPURendering /
    resetUseCPURendering are kept but deprecated in favor of setRenderBackend.

Capability detection

  • New utilities/renderingCapabilities probes WebGL/WebGL2 availability,
    MAX_TEXTURE_SIZE, the unmasked renderer string, software-rasterizer
    detection, 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 paths
    in place — viewport ids, mounted data, cameras, presentation state, and tool
    annotations are preserved; only the render paths rebuild. Reversible in both
    directions at runtime.
  • New degradation events on the eventTarget: RENDER_BACKEND_CHANGED,
    WEBGL_CONTEXT_LOST / WEBGL_CONTEXT_RESTORED, RENDER_PATH_ERROR, and
    RENDERING_PIPELINE_CHANGED. Cornerstone never switches backends on its own;
    applications listen and call setRenderBackend('cpu') themselves.

Segmentation rehydration across a switch

  • Labelmap representations now re-reconcile after a live backend switch: the
    rebuilt viewport actors are re-styled via a RENDERING_PIPELINE_CHANGED
    listener, 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

  • New example genericRenderBackendSwitch (and genericPetRenderBackendGrid)
    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.
  • Unit tests: planarRenderBackend.jest.js, renderingCapabilities.jest.js,
    labelmap render-plan specs; e2e genericRenderBackendSwitch.spec.ts.

Summary by CodeRabbit

  • New Features
    • Added live render-backend switching (auto/GPU/CPU) with per-viewport backend pinning and smooth pipeline updates.
    • Enhanced demos with multi-viewport CT/MPR/PET, mock segmentations/annotations, richer status/event reporting, and WebGL context-lost simulation.
    • Introduced rendering capability detection APIs to drive planar backend selection.
  • Bug Fixes
    • Improved stability of backend render-path switching and segmentation refresh after pipeline changes.
    • Strengthened labelmap remounting by honoring actor-entry compatibility across backend switches.
  • Tests
    • Added/expanded automated coverage for backend switching, capability detection, render-path/pipeline behavior, and CPU fallback correctness.

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5ec85a0a-72c5-46be-802b-6de28d94f624

📥 Commits

Reviewing files that changed from the base of the PR and between a43de47 and 7494a40.

📒 Files selected for processing (3)
  • packages/core/src/utilities/renderingCapabilities.ts
  • packages/core/src/utilities/textureSupport.ts
  • packages/core/test/renderingCapabilities.jest.js
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/core/test/renderingCapabilities.jest.js
  • packages/core/src/utilities/textureSupport.ts
  • packages/core/src/utilities/renderingCapabilities.ts

📝 Walkthrough

Walkthrough

This 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.

Changes

Render backend switching

Layer / File(s) Summary
Backend enums and capability detection
packages/core/src/enums/RenderBackend.ts, packages/core/src/enums/index.ts, packages/core/src/enums/Events.ts, packages/core/src/utilities/renderingCapabilities.ts, packages/core/src/utilities/textureSupport.ts, packages/core/test/renderingCapabilities.jest.js
Adds RenderBackend, render/WebGL events, rendering-capability probing and caching, expanded texture-format support, and capability tests.
Backend APIs and config
packages/core/src/init.ts, packages/core/src/index.ts, packages/core/src/types/Cornerstone3DConfig.ts
Adds render-backend getters/setters, effective-backend resolution, config defaults, backend-change events, and public exports.
Planar backend selection and CPU fallback
packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarRenderPathDecisionService.ts, packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewportTypes.ts, packages/core/src/RenderingEngine/GenericViewport/Planar/planarRenderPathSelector.ts, packages/core/src/RenderingEngine/GenericViewport/Planar/CpuImageSliceRenderPath.ts, packages/core/test/planarRenderBackend.jest.js, packages/core/test/planarComputedCamera.jest.js, utils/demo/helpers/exampleParameters.ts
Replaces threshold-based planar CPU selection with render-backend-based decisions, updates per-display-set overrides, adjusts CPU fallback viewport sizing, and extends tests and demo URL overrides.
Live planar pipeline swapping
packages/core/src/RenderingEngine/GenericViewport/GenericViewport.ts, packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.ts, packages/core/src/RenderingEngine/GenericViewport/Planar/VtkImageMapperRenderPath.ts, packages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/createLabelmapRenderPlan.ts, packages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/legacyVolumePlan.ts, packages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/types.ts, packages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/volumeSliceImageMapperPlan.ts
Adds live pipeline updates, guarded render-path error reporting, and render-path compatibility checks for labelmap remounting.
WebGL context event wiring
packages/core/src/RenderingEngine/helpers/attachWebGLContextEvents.ts, packages/core/src/RenderingEngine/WebGLContextPool.ts, packages/core/src/RenderingEngine/TiledRenderingEngine.ts, packages/core/src/RenderingEngine/ContextPoolRenderingEngine.ts
Adds WebGL context loss/restored helpers and attaches them to offscreen render windows.
Demo, E2E, and compatibility updates
packages/core/examples/genericRenderBackendSwitch/index.ts, packages/core/examples/genericPetRenderBackendGrid/index.ts, tests/genericViewport/genericRenderBackendSwitch.spec.ts, tests/utils/compatibilityMode.ts, packages/tools/src/eventListeners/segmentation/renderingPipelineChangedListener.ts, packages/tools/src/init.ts
Expands demos, adds end-to-end coverage, updates compatibility checks, and triggers segmentation rerenders after pipeline changes.

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
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning Context, Changes & Results, and Testing are present, but the required checklist section and tested-environment details are missing. Add the full checklist with checked boxes, include OS/Node/Browser details, and note any public docs updates or why none are needed.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is descriptive, matches the main change, and follows the semantic-release format.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cornerstoneCPUGPUDetect

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
packages/core/src/RenderingEngine/helpers/attachWebGLContextEvents.ts (1)

21-25: 📐 Maintainability & Code Quality | 🔵 Trivial

Inline cast masks a missing/incomplete VtkOffscreenMultiRenderWindow type.

getOpenGLRenderWindow?.() is cast to an ad-hoc { getCanvas?: ... } shape instead of the type declaring this method. If VtkOffscreenMultiRenderWindow's OpenGL render window type already exposes getCanvas, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 357796f and ec15509.

📒 Files selected for processing (24)
  • packages/core/examples/genericRenderBackendSwitch/index.ts
  • packages/core/src/RenderingEngine/ContextPoolRenderingEngine.ts
  • packages/core/src/RenderingEngine/GenericViewport/GenericViewport.ts
  • packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarRenderPathDecisionService.ts
  • packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.ts
  • packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewportTypes.ts
  • packages/core/src/RenderingEngine/GenericViewport/Planar/VtkImageMapperRenderPath.ts
  • packages/core/src/RenderingEngine/GenericViewport/Planar/planarRenderPathSelector.ts
  • packages/core/src/RenderingEngine/TiledRenderingEngine.ts
  • packages/core/src/RenderingEngine/WebGLContextPool.ts
  • packages/core/src/RenderingEngine/helpers/attachWebGLContextEvents.ts
  • packages/core/src/enums/Events.ts
  • packages/core/src/enums/RenderBackend.ts
  • packages/core/src/enums/index.ts
  • packages/core/src/index.ts
  • packages/core/src/init.ts
  • packages/core/src/types/Cornerstone3DConfig.ts
  • packages/core/src/utilities/renderingCapabilities.ts
  • packages/core/src/utilities/textureSupport.ts
  • packages/core/test/planarRenderBackend.jest.js
  • packages/core/test/renderingCapabilities.jest.js
  • tests/genericViewport/genericRenderBackendSwitch.spec.ts
  • tests/utils/compatibilityMode.ts
  • utils/demo/helpers/exampleParameters.ts
💤 Files with no reviewable changes (1)
  • packages/core/src/RenderingEngine/GenericViewport/Planar/planarRenderPathSelector.ts

Comment on lines +31 to +39
canvas.addEventListener('webglcontextlost', () => {
console.warn(
`CornerstoneRender: WebGL context lost (renderingEngine=${renderingEngineId}, context=${contextIndex})`
);
triggerEvent(eventTarget, Events.WEBGL_CONTEXT_LOST, {
renderingEngineId,
contextIndex,
});
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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:


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.
@sedghi sedghi changed the title wip feat(rendering): unify render backend selection with live GPU/CPU switching Jul 3, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
packages/core/examples/genericRenderBackendSwitch/index.ts (1)

490-553: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Repeated 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 win

Consider unit-testing the new remount-compatibility gate.

needsRemount now forces a remount when any actor entry fails isActorEntryCompatible, and getActorEntryRenderMode is the load-bearing extraction for that check across both legacyVolumePlan.ts and volumeSliceImageMapperPlan.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

📥 Commits

Reviewing files that changed from the base of the PR and between ec15509 and 57467ba.

📒 Files selected for processing (17)
  • packages/core/examples/genericPetRenderBackendGrid/index.ts
  • packages/core/examples/genericRenderBackendSwitch/index.ts
  • packages/core/src/RenderingEngine/GenericViewport/Planar/CpuImageSliceRenderPath.ts
  • packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarRenderPathDecisionService.ts
  • packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.ts
  • packages/core/src/enums/Events.ts
  • packages/core/src/init.ts
  • packages/core/src/utilities/renderingCapabilities.ts
  • packages/core/test/planarComputedCamera.jest.js
  • packages/core/test/planarRenderBackend.jest.js
  • packages/core/test/renderingCapabilities.jest.js
  • packages/tools/src/eventListeners/segmentation/renderingPipelineChangedListener.ts
  • packages/tools/src/init.ts
  • packages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/createLabelmapRenderPlan.ts
  • packages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/legacyVolumePlan.ts
  • packages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/types.ts
  • packages/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.
@IbrahimCSAE

Copy link
Copy Markdown
Member

🔥🔥🔥🔥🔥🔥🔥

Comment thread packages/core/src/utilities/textureSupport.ts

@wayfarer3130 wayfarer3130 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Leak 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 returns null without releasing the context (no WEBGL_lose_context call, no shader/program deletion). Unlike the success path in getSupportedTextureFormats(), 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

📥 Commits

Reviewing files that changed from the base of the PR and between e201f93 and a43de47.

📒 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants