fix: Fix HTJ2K decoder leak #1388
Conversation
Commit 85fd193 changed decodeHTJ2K.js to allocate a new HTJ2KDecoder with each call to decodeAsync. The old decoders somehow stay alive, and are not cleaned from the WASM memory. Each new HTJ2KDecoder allocates in turns buffers for the encoded and decoded pixel data while decoding. The change to allocate a new decoder per frame argued that reusing the old decoder is "much slower", but I could not reproduce any slowdowns either in the browser or on Node. This commit therefore reverts the change, so that each call to decodeAsync reuses the same decoder instance.
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -57,8 +57,7 @@ export function initialize(decodeConfig?: LoaderDecodeOptions): Promise<void> { | |||
| // https://github.com/chafey/openjpegjs/blob/master/test/browser/index.html | |||
| async function decodeAsync(compressedImageFrame: ByteArray, imageInfo) { | |||
| await initialize(); | |||
| // const decoder = local.decoder; // This is much slower for some reason | |||
| const decoder = new local.codec.HTJ2KDecoder(); | |||
| const decoder = local.decoder; | |||
There was a problem hiding this comment.
This was the original implementation, but it causes a bug where some images can't be decoded and the decoding gets slower and slower over time.
The problem is that some of the state in the decoder isn't being cleared correctly. I would try updating the openjph library to latest, make sure the malloc library used is the same one we use for openjpeg, and rebuild that part of the system. Then you can test to see if it works using the CS3D progressive loading tests. Test the decompression time for the second decompression, and then for the last decompression time on the HTJ2K volume, as well as running the HTJ2K stack tests, with multiple runs through the stack. The decompression time should stay roughly constant.
| @@ -57,8 +57,7 @@ export function initialize(decodeConfig?: LoaderDecodeOptions): Promise<void> { | |||
| // https://github.com/chafey/openjpegjs/blob/master/test/browser/index.html | |||
| async function decodeAsync(compressedImageFrame: ByteArray, imageInfo) { | |||
| await initialize(); | |||
| // const decoder = local.decoder; // This is much slower for some reason | |||
| const decoder = new local.codec.HTJ2KDecoder(); | |||
| const decoder = local.decoder; | |||
There was a problem hiding this comment.
Should be
const { decoder } = local;
|
If you update this PR to include an openjph version change for the release after cornerstonejs/codecs#46 |
Context
Commit 85fd193 changed decodeHTJ2K.js to allocate a new HTJ2KDecoder with each call to decodeAsync. The old decoders somehow stay alive, and are not cleaned from the WASM memory. Each new HTJ2KDecoder allocates in turns buffers for the encoded and decoded pixel data while decoding.
Changes & Results
The change to allocate a new decoder per frame argued that reusing the old decoder is "much slower", but I could not reproduce any slowdowns either in the browser or on Node. This commit therefore reverts the change, so that each call to decodeAsync reuses the same decoder instance.
Testing
Checklist
PR
semantic-release format and guidelines.
Code
[] My code has been well-documented (function documentation, inline comments,
etc.)
[] I have run the
yarn build:update-apito update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment