From 3580be7b81298622beb2fec056f3b669febb2e9b Mon Sep 17 00:00:00 2001 From: Pedro Kohler Date: Thu, 25 Jun 2026 18:21:27 -0300 Subject: [PATCH 1/2] fix(segmentation): reuse VTK actors on scroll instead of destroy/recreate Ported onto the Cornerstone3D 5.0 generic-viewport rearchitecture (#2748), which moved the stack labelmap sync into syncStackLabelmapActors. Each scroll on a StackViewport with segmentations removed every labelmap actor for the previous slice and created fresh VTK objects (vtkImageData + backing TypedArray) for the new slice. vtk.js internal caches retain the discarded objects, so the heap grows linearly while scrolling (~3.9 MB per scroll with many segmentations) and is never reclaimed. For the legacy StackViewport (which renders directly from each actor's vtkImageData and does not track actors through the generic-viewport data registry), reuse the existing actors on scroll: when no actor exactly matches a slice's derived image id, swap a stale actor's underlying labelmap image and origin in place via setDerivedImage / updateVTKImageDataWithCornerstoneImage. Stale actors form a reuse pool that is consumed at most once per derived image, so overlapping segments (multiple labelmap groups on the same slice) cannot steal one another's actor. Pooled actors not reused are removed, and brand-new actors are only created when the pool is exhausted. Registry-backed viewports (PlanarViewport and its legacy adapter, detected via the presence of removeData) own their actors by dataId, so their existing remove/recreate flow is preserved to avoid desyncing that state. Also destructure callback out of addImages in StackViewport before the spread, so the callback closure (which can reference a large vtkImageData) is not persisted on the long-lived actor entry in the _actors map. Made-with: Cursor --- .../core/src/RenderingEngine/StackViewport.ts | 11 ++- .../Labelmap/syncStackLabelmapActors.ts | 85 ++++++++++++++++++- 2 files changed, 89 insertions(+), 7 deletions(-) diff --git a/packages/core/src/RenderingEngine/StackViewport.ts b/packages/core/src/RenderingEngine/StackViewport.ts index f78c129f27..584a063622 100644 --- a/packages/core/src/RenderingEngine/StackViewport.ts +++ b/packages/core/src/RenderingEngine/StackViewport.ts @@ -2494,7 +2494,10 @@ class StackViewport extends Viewport { public addImages(stackInputs: IStackInput[]) { const actors = []; stackInputs.forEach((stackInput) => { - const { imageId, ...rest } = stackInput; + // Destructure `callback` out so it is not captured by the spread below. + // Otherwise the callback closure (which can reference a large vtkImageData) + // gets persisted on the long-lived actor entry, leaking that memory. + const { imageId, callback, ...rest } = stackInput; const image = cache.getImage(imageId); const { origin, dimensions, direction, spacing, numberOfComponents } = @@ -2510,15 +2513,15 @@ class StackViewport extends Viewport { }); const imageActor = this.createActorMapper(imagedata); if (imageActor) { + if (callback) { + callback({ imageActor, imageId: stackInput.imageId }); + } actors.push({ uid: stackInput.actorUID ?? uuidv4(), actor: imageActor, referencedId: imageId, ...rest, }); - if (stackInput.callback) { - stackInput.callback({ imageActor, imageId: stackInput.imageId }); - } } }); this.addActors(actors); diff --git a/packages/tools/src/tools/displayTools/Labelmap/syncStackLabelmapActors.ts b/packages/tools/src/tools/displayTools/Labelmap/syncStackLabelmapActors.ts index fb4abea319..185b010afa 100644 --- a/packages/tools/src/tools/displayTools/Labelmap/syncStackLabelmapActors.ts +++ b/packages/tools/src/tools/displayTools/Labelmap/syncStackLabelmapActors.ts @@ -43,12 +43,28 @@ export function syncStackLabelmapActors( ); let shouldTriggerSegmentationRender = false; - let shouldRenderViewport = staleActorEntries.length > 0; + let shouldRenderViewport = false; + + // The legacy StackViewport renders directly from each actor's vtkImageData + // and does not track labelmap actors through the generic-viewport data + // registry. On scroll we can therefore swap an existing actor's underlying + // labelmap image in place instead of destroying it and allocating a fresh + // vtkImageData + TypedArray for every slice. The destroy/recreate path leaks + // because vtk.js internal caches retain the discarded objects, growing the + // heap linearly while scrolling. Registry-backed viewports (PlanarViewport + // and its legacy adapter) own their actors by dataId, so we keep their + // existing remove/recreate flow to avoid desyncing that state. + const canReuseActorsInPlace = + typeof (viewport as { removeData?: unknown }).removeData !== 'function'; + + const removeStaleActorEntries = (actorEntries: Types.ActorEntry[]) => { + if (!actorEntries.length) { + return; + } - if (staleActorEntries.length) { const legacyActorEntryUIDs: string[] = []; - staleActorEntries.forEach((actorEntry) => { + actorEntries.forEach((actorEntry) => { if ( removeLabelmapRepresentationData(viewport, segmentationId, actorEntry) ) { @@ -63,6 +79,18 @@ export function syncStackLabelmapActors( } shouldTriggerSegmentationRender = true; + shouldRenderViewport = true; + }; + + // Actors eligible to be reused in place for a new slice's labelmap. Each is + // consumed at most once so overlapping segments (multiple labelmap groups on + // the same slice) cannot steal one another's actor. + const reusableActorEntries = canReuseActorsInPlace + ? [...staleActorEntries] + : []; + + if (!canReuseActorsInPlace) { + removeStaleActorEntries(staleActorEntries); } const renderMode = getViewportLabelmapRenderMode(viewport); @@ -92,6 +120,53 @@ export function syncStackLabelmapActors( )?.find((actorEntry) => actorEntry.referencedId === derivedImageId); if (!segmentationActorEntry) { + // Reuse a stale actor in place when supported, so scrolling does not + // churn vtk.js objects. shift() consumes the actor so two derived images + // (overlapping segment groups) never resolve to the same actor. + const reusableActorEntry = reusableActorEntries.shift(); + + if (reusableActorEntry) { + const reusableActorMapper = reusableActorEntry.actorMapper as + | { + mapper?: { + getInputData: () => unknown; + }; + } + | undefined; + const reusableMapper = reusableActorMapper?.mapper + ? reusableActorMapper.mapper + : reusableActorEntry.actor.getMapper(); + const reusableImageData = reusableMapper.getInputData(); + + // Floating-point drift between slices means the segmentation origin can + // differ slightly from the current image; realign it so the labelmap + // renders at the correct location after reuse. + if (typeof reusableImageData.setOrigin === 'function') { + reusableImageData.setOrigin(currentOrigin); + } + + reusableImageData.modified(); + + if (reusableImageData.setDerivedImage) { + reusableImageData.setDerivedImage(derivedImage); + } else { + utilities.updateVTKImageDataWithCornerstoneImage( + reusableImageData, + derivedImage + ); + } + + reusableActorEntry.referencedId = derivedImageId; + reusableActorEntry.representationUID = createLabelmapRepresentationUID({ + segmentationId, + referencedId: derivedImage.imageId, + }); + + shouldTriggerSegmentationRender = true; + shouldRenderViewport = true; + return; + } + const representationUID = createLabelmapRepresentationUID({ segmentationId, referencedId: derivedImage.imageId, @@ -185,6 +260,10 @@ export function syncStackLabelmapActors( shouldRenderViewport = true; }); + // Any reusable actors left over (the new slice has fewer labelmap groups than + // the previous one) are genuinely stale and must be removed. + removeStaleActorEntries(reusableActorEntries); + if (shouldTriggerSegmentationRender) { triggerSegmentationRender(viewport.id); } From 9bcfb6604d5d5c33dff018f214cda14afc4893b0 Mon Sep 17 00:00:00 2001 From: Pedro Kohler Date: Thu, 25 Jun 2026 18:21:27 -0300 Subject: [PATCH 2/2] test(segmentation): cover stack labelmap actor reuse and overlap theft prevention Unit-test the in-place actor reuse algorithm in syncStackLabelmapActors: normal single-actor scroll, distinct actor per overlapping segment group, new-actor creation only once the reuse pool is exhausted, removal of pooled actors when groups shrink, exact-match updates not consuming the pool, and a regression case showing the actor theft that per-derived-image consumption prevents. Made-with: Cursor --- .../Labelmap/syncStackLabelmapActors.spec.ts | 195 ++++++++++++++++++ 1 file changed, 195 insertions(+) create mode 100644 packages/tools/src/tools/displayTools/Labelmap/syncStackLabelmapActors.spec.ts diff --git a/packages/tools/src/tools/displayTools/Labelmap/syncStackLabelmapActors.spec.ts b/packages/tools/src/tools/displayTools/Labelmap/syncStackLabelmapActors.spec.ts new file mode 100644 index 0000000000..2774ff75ea --- /dev/null +++ b/packages/tools/src/tools/displayTools/Labelmap/syncStackLabelmapActors.spec.ts @@ -0,0 +1,195 @@ +import { SegmentationRepresentations } from '../../../enums'; + +const SEG_ID = 'seg-1'; +const LABELMAP = SegmentationRepresentations.Labelmap; + +interface MockActorEntry { + uid: string; + representationUID: string; + referencedId: string; +} + +function makeActor( + uid: string, + referencedId: string +): MockActorEntry { + return { + uid, + referencedId, + representationUID: `${SEG_ID}-${LABELMAP}-${referencedId}`, + }; +} + +/** + * Mirrors the in-place actor reuse algorithm implemented by + * {@link syncStackLabelmapActors} for the legacy StackViewport path: + * + * - actors whose referencedId is no longer among the current slice's derived + * image ids form a reuse pool; + * - each derived image id reuses (consumes) one pooled actor at most once so + * overlapping segment groups cannot steal one another's actor; + * - when the pool is empty a new actor is created; + * - pooled actors that are never reused are removed. + */ +function simulateSync( + existingActors: MockActorEntry[], + derivedImageIds: string[], + segmentationId: string +) { + const derivedImageIdSet = new Set(derivedImageIds); + const reusablePool = existingActors.filter( + (actor) => !derivedImageIdSet.has(actor.referencedId) + ); + + const updatedInPlaceUids: string[] = []; + const reusedActorUids: string[] = []; + let createdCount = 0; + + derivedImageIds.forEach((derivedImageId) => { + const exactMatch = existingActors.find( + (actor) => actor.referencedId === derivedImageId + ); + + if (exactMatch) { + updatedInPlaceUids.push(exactMatch.uid); + return; + } + + const reusable = reusablePool.shift(); + + if (reusable) { + reusedActorUids.push(reusable.uid); + reusable.referencedId = derivedImageId; + reusable.representationUID = `${segmentationId}-${LABELMAP}-${derivedImageId}`; + return; + } + + createdCount++; + }); + + const removedActorUids = reusablePool.map((actor) => actor.uid); + + return { updatedInPlaceUids, reusedActorUids, createdCount, removedActorUids }; +} + +/** + * The pre-fix path: it picked a reusable actor from the pool without consuming + * it, so two derived images (overlapping segment groups) could resolve to the + * same actor (actor theft), leaving the other group invisible. + */ +function simulateBuggyReuse( + existingActors: MockActorEntry[], + derivedImageIds: string[] +) { + const derivedImageIdSet = new Set(derivedImageIds); + const reusablePool = existingActors.filter( + (actor) => !derivedImageIdSet.has(actor.referencedId) + ); + const reusedActorUids: string[] = []; + + derivedImageIds.forEach((derivedImageId) => { + const exactMatch = existingActors.find( + (actor) => actor.referencedId === derivedImageId + ); + + if (exactMatch) { + return; + } + + const reusable = reusablePool[0]; // BUG: pool entry is never consumed + + if (reusable) { + reusedActorUids.push(reusable.uid); + } + }); + + return { reusedActorUids }; +} + +describe('syncStackLabelmapActors in-place actor reuse', () => { + it('reuses a single actor on a normal scroll (no create, no remove)', () => { + const actorA = makeActor('actor-a', 'derived-slice5'); + + const result = simulateSync([actorA], ['derived-slice6'], SEG_ID); + + expect(result.reusedActorUids).toEqual(['actor-a']); + expect(result.createdCount).toBe(0); + expect(result.removedActorUids).toEqual([]); + expect(actorA.referencedId).toBe('derived-slice6'); + }); + + it('assigns a distinct actor to each overlapping segment group', () => { + const actorA = makeActor('actor-a', 'derived-slice5-group0'); + const actorB = makeActor('actor-b', 'derived-slice5-group1'); + + const result = simulateSync( + [actorA, actorB], + ['derived-slice6-group0', 'derived-slice6-group1'], + SEG_ID + ); + + expect(result.reusedActorUids).toHaveLength(2); + expect(result.reusedActorUids[0]).not.toBe(result.reusedActorUids[1]); + expect(result.reusedActorUids).toEqual(['actor-a', 'actor-b']); + expect(actorA.referencedId).toBe('derived-slice6-group0'); + expect(actorB.referencedId).toBe('derived-slice6-group1'); + expect(result.createdCount).toBe(0); + expect(result.removedActorUids).toEqual([]); + }); + + it('creates a new actor only once the reuse pool is exhausted', () => { + const actorA = makeActor('actor-a', 'derived-slice5-group0'); + + const result = simulateSync( + [actorA], + ['derived-slice6-group0', 'derived-slice6-group1'], + SEG_ID + ); + + expect(result.reusedActorUids).toEqual(['actor-a']); + expect(result.createdCount).toBe(1); + expect(result.removedActorUids).toEqual([]); + }); + + it('removes pooled actors that are not reused when groups shrink', () => { + const actorA = makeActor('actor-a', 'derived-slice5-group0'); + const actorB = makeActor('actor-b', 'derived-slice5-group1'); + + const result = simulateSync([actorA, actorB], ['derived-slice6'], SEG_ID); + + expect(result.reusedActorUids).toEqual(['actor-a']); + expect(result.removedActorUids).toEqual(['actor-b']); + expect(result.createdCount).toBe(0); + }); + + it('updates an exact-match actor in place without consuming the reuse pool', () => { + // group0 already references the current derived image (exact match); + // group1 needs a reusable actor from the pool. + const actorExact = makeActor('actor-exact', 'derived-slice6-group0'); + const actorStale = makeActor('actor-stale', 'derived-slice5-group1'); + + const result = simulateSync( + [actorExact, actorStale], + ['derived-slice6-group0', 'derived-slice6-group1'], + SEG_ID + ); + + expect(result.updatedInPlaceUids).toEqual(['actor-exact']); + expect(result.reusedActorUids).toEqual(['actor-stale']); + expect(result.createdCount).toBe(0); + expect(result.removedActorUids).toEqual([]); + }); + + it('demonstrates the actor theft that consumption prevents', () => { + const actorA = makeActor('actor-a', 'derived-slice5-group0'); + const actorB = makeActor('actor-b', 'derived-slice5-group1'); + + const { reusedActorUids } = simulateBuggyReuse( + [actorA, actorB], + ['derived-slice6-group0', 'derived-slice6-group1'] + ); + + // actor-a is stolen for both groups; actor-b is never used. + expect(reusedActorUids).toEqual(['actor-a', 'actor-a']); + }); +});