Skip to content

fix(OpenGL): stop image mappers leaking their default lookup textures - #3683

Open
jensenbox wants to merge 1 commit into
Kitware:masterfrom
jensenbox:fix-imagemapper-texture-leak
Open

jensenbox wants to merge 1 commit into
Kitware:masterfrom
jensenbox:fix-imagemapper-texture-leak

Conversation

@jensenbox

Copy link
Copy Markdown

Context

When an image property has no RGB transfer function or no piecewise function, vtkOpenGLImageMapper creates a new color or opacity lookup texture on every buffer object rebuild and never deletes the old one. vtkOpenGLImageResliceMapper and vtkOpenGLImageCPRMapper have the same code. On top of that, vtkOpenGLImageMapper and vtkOpenGLImageCPRMapper rebuild on every render after any actor or property change that leaves the vertices alone, such as a new color window. Together they leak one texture per missing function, per image slice, on every rendered frame after the first window/level change. In an app that shows many image slices without transfer functions, the live texture count grows without bound.

Affected versions: 36.2.0 to 37.4.2 and master (checked at 009a4b2a). Between 36.2.0 and master the three mappers only changed how the quad's cells are built (and the Reslice slab shader). Older versions were not checked.

Workaround until a release: give every image property an RGB transfer function and a piecewise function. They can be shared between properties. The lookup textures are then cached once per render window. This stops the leak but not the per-frame rebuild.

Root cause

Links are pinned to master at 009a4b2a9be3164820803a8d76065b45bbae0be3.

  1. The default textures are never cached or released.

    • vtkImageProperty.getRGBTransferFunction() and getPiecewiseFunction() return null when nothing is set (ImageProperty L57-L58, L78-L79).
    • The mapper looks up its lookup textures in the render window's shared cache keyed on that function, so the lookup with null always misses. In the color block (ImageMapper L979-L1067) a new vtkOpenGLTexture is created at L987 and filled with a grayscale ramp. The previous model.colorTexture is dropped without releaseGraphicsResources.
    • setGraphicsResourceForObject and registerGraphicsResourceUser sit inside if (firstColorTransferFunc) (L1047), and setGraphicsResourceForObject ignores a null key anyway (RenderWindow L1245-L1248). So the texture is never owned by the render window and never deleted, not even when the mapper is deleted: unregisterGraphicsResources L57-L69 only knows the scalar texture and the shared ones.
    • The opacity block has the same shape (L1081-L1172: new texture at L1094, cache guard at L1152).
    • vtkOpenGLImageResliceMapper: color L433-L513, opacity L536-L612.
    • vtkOpenGLImageCPRMapper: color L277-L356, opacity L370-L450.
  2. The rebuild never settles after a property change.

    • getNeedToRebuildBufferObjects rebuilds when VBOBuildTime is older than the actor or its property (ImageMapper L931-L942).
    • VBOBuildTime.modified() (L1417) only runs inside if (model.VBOBuildString !== toString) (L1213). That string is built from the slice, the image and scalars mtimes, the mapper mtime, the slicing mode and the interpolation type. A color window, opacity or actor change leaves it alone, so every later render runs buildBufferObjects again and goes through the texture code above.
    • vtkOpenGLImageCPRMapper has the same split: the check at L175-L187 includes the actor, but VBOBuildTime.modified() (L606) only runs when the mapper or the centerline changed (L453-L456).
    • vtkOpenGLImageResliceMapper already marks VBOBuildTime at the end of every rebuild (L657). It leaks once per rebuild (each slice plane move or property change), not on every frame.

Results

All numbers come from the new tests, run on master and on this branch. The scene is one slice of a 16 bit image with no transfer functions. Its first render makes 5 GL objects: the scalar, color and opacity textures, a vertex buffer and a VAO.

Scenario master this PR
ImageMapper: buffer object builds over 20 renders after one color window change 20 1
ImageMapper: live GL objects after 15 slice changes 35 5
ImageResliceMapper: live GL objects after 15 slice plane moves and a color window change 37 5
ImageCPRMapper: buffer object builds over 20 renders after one color window change 20 1
ImageCPRMapper: live GL objects after 15 width changes 35 5
Any of the three: GL objects freed when the slice is removed from the scene 1 3

On this branch the ImageMapper slice test then switches the interpolation type twice, which replaces the default color texture, and the count stays at 5.

Each test targets one of the two fixes. With only the texture change applied, the two "rebuilds once" tests fail with 20 builds. With only the VBOBuildTime change applied, the six texture tests fail, and the two rebuild tests count one build but still see the two textures it leaked.

The existing tests pass, including every image comparison test for the image, reslice and image array mappers. The full suite passes: 233 test files, 649 tests passed and 13 skipped. It ran in headless Chromium with SwiftShader (CI=1) in the mcr.microsoft.com/playwright:v1.60.0-noble image.

Changes

  • vtkOpenGLImageMapper, vtkOpenGLImageResliceMapper and vtkOpenGLImageCPRMapper: with no transfer function, the mapper keeps its default color and opacity textures in model._defaultColorTexture and model._defaultPwfTexture, each with a hash of what it was built from. They are rebuilt only when that hash changes: the texture size, plus the interpolation type in vtkOpenGLImageMapper, which sets the filters when it builds the texture. The texture they replace is released, and unregisterGraphicsResources releases them on delete and when the mapper moves to another render window. Textures in the render window's shared cache are left to the cache, as before.
  • Each mapper owns its defaults rather than sharing them across the render window, because the default color texture depends on the mapper's texture width (and, in vtkOpenGLImageMapper, its interpolation type). That costs two small textures per mapper, about 4 KB for one component at the default width of 1024.
  • vtkOpenGLImageMapper and vtkOpenGLImageCPRMapper: VBOBuildTime.modified() moves to the end of buildBufferObjects, so a rebuild caused by an actor or property change is marked like any other. The vertex rebuild conditions inside the method are unchanged, and the early returns (no input, no scalars) still skip the mark. vtkImageProperty.getMTime() includes its transfer functions, so editing a transfer function in place still triggers a rebuild.
  • New tests: Sources/Rendering/OpenGL/{ImageMapper,ImageResliceMapper,ImageCPRMapper}/test/testReleaseGraphicsResources.js. They count live GL objects with createTrackedRenderView, like the other testReleaseGraphicsResources.js files. publicAPI objects are frozen, so to count rebuilds the tests register a small subclass of the OpenGL mapper with registerOverride, under a test-only class name.
  • No public API, documentation or TypeScript changes.

Notes for review

  • vtkOpenGLVolumeMapper is not affected: vtkVolumeProperty.getRGBTransferFunction() and getScalarOpacity() create a default function when none is set (VolumeProperty L129-L130, L165-L166), so its cache key is never null.
  • In setMapperShaderParameters, model.VBOBuildTime > cellBO.getAttributeUpdateTime().getMTime() (ImageMapper L618, same in SphereMapper L145 and StickMapper L175) compares the time stamp object with a number. A macro.obj has no valueOf, so this is always false and the VAO attributes are only re-added when the shader changes. The image mapper's vertex layout never changes and uploads reuse the same GL buffer, so this is harmless today, and marking VBOBuildTime more often does not touch the VAO. This PR leaves that line alone.
  • Not fixed here, found while testing: none of the three image mappers ever calls model.tris.releaseGraphicsResources() (Helper L34-L38). Each removed or deleted image slice leaves its vertex buffer and VAO behind, which is why the removal tests check for at least 3 freed objects rather than an empty scene. It needs its own change, so this PR leaves it out.
  • Not fixed here, from reading the code and not measured: vtkOpenGLImageResliceMapper releases its label outline textures with releaseGraphicsResources() and no render window (L1970-L1972, L2019-L2026). vtkOpenGLTexture.releaseGraphicsResources only deletes the GL texture when it gets one (Texture L290-L291), so the old texture survives each label outline change and the mapper's deletion.
  • The WebGPU image mappers get their lookup textures from the device texture manager, which keys them on a hash string, so they do not use this cache. I did not check them in depth.

PR Checklist

  • GitHub Actions CI passed: semantic-release commit messages, lint, and tests (locally the full suite, oxfmt --check, oxlint and commitlint pass)
  • Test coverage added
  • Documentation and TypeScript definitions are updated to match these changes (no public API change)

With no RGB transfer function or piecewise function on the property,
vtkOpenGLImageMapper, vtkOpenGLImageResliceMapper and
vtkOpenGLImageCPRMapper built a new default color or opacity texture
on every buffer object rebuild. The shared texture cache is keyed on
the transfer function, so the default texture was never cached, and
the texture it replaced was never released. Each rebuild leaked two
textures. The mappers now keep their default textures, rebuild them
only when their size or filtering changes, release the one they
replace and release them with the rest of the mapper's resources.

vtkOpenGLImageMapper and vtkOpenGLImageCPRMapper also rebuilt on every
render after an actor or property change that left the vertices alone,
such as a new color window. They compare VBOBuildTime with the actor
and property mtimes but only marked it when the vertices changed.
VBOBuildTime is now marked at the end of every rebuild.
mapper.setSlice(slice);
renderWindow.render();
}
expect(tracker.count()).toBe(objectsAfterFirstRender);

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.

Could we additionally check that the default texture handles stay unchanged across slice-only changes, and cover replacement after changing the color/opacity texture widths? That would exercise both reuse and cache invalidation. The same request for CPR and reslice tests.

@sankhesh

Copy link
Copy Markdown
Collaborator

The ownership change looks good.

Please shorten the PR description to the problem, the fix, why the default textures are mapper-owned, and the validation results. The repeated source walkthrough and unrelated leak investigations would be easier to follow in linked issues.

This branch has not been deployed

No deployments
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.

2 participants