Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an asynchronous race condition in the DICOM image loading pipeline where PALETTE COLOR images could render as black because palette LUT data was not fully fetched/converted before pixel data was used.
Changes:
- Made
convertPALETTECOLORasynchronous and ensured palette LUT fetching is awaited before conversion. - Made
convertColorSpaceasynchronous and awaited PALETTE COLOR conversion. - Updated
createImagetoawait convertColorSpaceduring decode post-processing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/dicomImageLoader/src/imageLoader/createImage.ts | Awaits color-space conversion during image creation to prevent premature use of unconverted pixel data. |
| packages/dicomImageLoader/src/imageLoader/convertColorSpace.ts | Converts the color conversion dispatcher to async so PALETTE COLOR conversion can be awaited. |
| packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertPALETTECOLOR.ts | Converts the PALETTE COLOR converter to async/await to properly synchronize palette data fetching and conversion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertPALETTECOLOR.ts
Outdated
Show resolved
Hide resolved
3bf7822 to
d65319c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
packages/dicomImageLoader/src/imageLoader/createImage.ts:48
- PR description says
convertColorSpaceand the decode callback were converted to async/await, but in the current codeconvertColorSpace(...)is still called synchronously and thedecodePromise.then(...)handler is notasync. Please either update the PR description to match the implemented approach (palette prefetch increateImage) or update the code so it actually follows the described async/await pipeline.
async function createImage(
imageId: string,
pixelData: ByteArray,
transferSyntax: string,
options: DICOMLoaderImageOptions = {}
): Promise<DICOMLoaderIImage | Types.IImageFrame> {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertPALETTECOLOR.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
This new PALETTE COLOR preload path is central to fixing the black-image race. There are existing automated tests that cover createImage, but none appear to exercise PALETTE COLOR with palette bulkdata coming back asynchronously (e.g., WADO-RS). Please add a regression test that loads a palette-color DICOM and asserts the converted pixel data is produced deterministically (not all zeros) and that palette LUT data is present after createImage resolves.
packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertPALETTECOLOR.ts
Outdated
Show resolved
Hide resolved
packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertPALETTECOLOR.ts
Outdated
Show resolved
Hide resolved
sedghi
left a comment
There was a problem hiding this comment.
it is not a request change but just to grab your attention
-
can you go over this example, and compare your local version of rendering to the deploy and make sure we don't have regression (i'm not sure if we have test for these)
https://www.cornerstonejs.org/live-examples/dicomimageloaderwadouri -
also can you connect it to OHIF and make sure US studies work fine, actually run the OHIF tests with your local version of cornerstone
|
Look in to the example: one difference. In deflate option it gives an error and OSS silent give the same error, so i think the PR is better in not hiding problems. OHIF appears to look ok (didn't open all US examples) |
wayfarer3130
left a comment
There was a problem hiding this comment.
When I link to OHIF and run, I get an exception that hte palette colour hasn't been loaded yet. I suspect the load palette data is a little too late.
Context
The convertPALETTECOLOR function was using Promise.all().then() without returning or awaiting the promise. This caused the function to complete immediately before the palette color data was fully fetched and the pixel data was properly converted. As a result, images with PALETTE COLOR photometric interpretation would sometimes render as black images because the pixel data was being used before it was ready.
Solution
Convert the color space conversion pipeline to use async/await pattern to ensure proper synchronization:
convertPALETTECOLOR - Changed from synchronous to async function that awaits palette data fetching and conversion
convertColorSpace - Converted to async function and awaits the palette color conversion when needed
createImage - Updated the decode promise handler to be async and await color space conversion
Changes
Modified convertPALETTECOLOR.ts: Changed function signature to async and replaced .then() with await
Modified convertColorSpace.ts: Made function async and added await for PALETTE COLOR conversion
Modified createImage.ts: Made the decode callback async to properly await color conversion
Impact
This ensures that palette color images are fully processed before rendering, eliminating the race condition that caused black images to appear intermittently.
Changes & Results
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment