fix(dicom-image-loader): use Float32Array for non-integer rescale slopes to prevent pixel data corruption - #2707
Conversation
…pes to prevent pixel data corruption DICOM images with non-integer RescaleSlope (e.g. DTI FA maps with RescaleSlope=0.001) have their pixel data destroyed during preScale because getPixelDataTypeFromMinMax incorrectly selects Uint8Array. This happens because Number.isInteger(1.0) === true in JavaScript, so scaled min/max of 0.0 and 1.0 are treated as integers, leading to Uint8Array selection. All fractional values (0.013, 0.5, 0.999) are then truncated to 0. Fix: 1. In _handlePreScaleSetup: detect non-integer scaling parameters and force Float32Array before scaling is applied. 2. In setPixelDataType: skip re-typing if pixelData is already Float32Array, preventing the main thread from downgrading it back to Uint8Array after web worker transfer. Fixes: cornerstonejs#2706 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… division by zero The DICOM LINEAR VOI formula uses (WW-1) which becomes 0 when WW=1, producing lower === upper (degenerate range). This causes division by zero in the VOI LUT shader, resulting in a binary black/white image. Fall back to LINEAR_EXACT (lower = WC - WW/2, upper = WC + WW/2) which correctly handles WW=1 (e.g. DTI FA with WC=0.5, WW=1 gives lower=0, upper=1). Fixes: cornerstonejs#2706 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
sedghi
left a comment
sedghi
left a comment
There was a problem hiding this comment.
The three-part fix correctly addresses the reported StackViewport corruption for non-integer RescaleSlope by forcing a Float32Array in the pre-scale path, preventing the main thread from downgrading it, and avoiding a division by zero when WW=1 where lower equals upper. This core scenario works because comlink structured-clone preserves the Float32Array across the worker boundary, meaning the instanceof guard in setPixelDataType works as expected. The remaining issues are about incomplete coverage in adjacent paths rather than the main flow.
minor packages/dicomImageLoader/src/decodeImageFrameWorker.js:93
The Float32 fix only runs in _handlePreScaleSetup. When options.targetBuffer.type is provided for volume loading, postProcessDecodedPixels takes the _handleTargetBuffer branch instead. Here, validatePixelDataType(scaledMin, scaledMax, Uint8Array) returns true for a scaled range like [0,1] because both 0 and 1 are integers. This leaves invalidType as false, so the data is copied into a Uint8Array target and truncates the fractional values. As a result, loading the same DTI FA map into a volume viewport still causes corruption.
minor packages/dicomImageLoader/src/decodeImageFrameWorker.js:249
hasFloatRescale is recomputed inside _handlePreScaleSetup using the exact same Object.values(...).some(...) expression already computed at lines 79-83 in the caller, which was not passed down. This causes a redundant scan of the scaling parameters.
minor packages/core/src/utilities/windowLevel.ts:61
The WW<=1 guard only rewrites voiLUTFunction when it is LINEAR. However, SAMPLED_SIGMOID goes through the identical LINEAR-formula branch at lines 64-67 and will still produce a division by zero where lower equals upper when WW<=1. The guard should also cover SAMPLED_SIGMOID to match the branch it protects.
nit packages/dicomImageLoader/src/decodeImageFrameWorker.js:250
hasFloatRescale triggers on any non-integer numeric scaling parameter, including ones not actually used by _calculateScaledMinMax for the current modality, such as a stray suvbw on a non-PT image. This can unnecessarily force a Float32Array and double the memory use compared to Uint16 for 16-bit data that did not need it.
📝 WalkthroughWalkthroughThis PR fixes float-scaled DICOM pixel data (e.g., non-integer RescaleSlope, DTI FA maps) rendering incorrectly as truncated integers. It adds ChangesFloat scaling correctness across core and dicomImageLoader
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant decodeImageFrameWorker
participant getScalingMode
participant hasNonIntegerScaling
participant PreScaleSetup as _handlePreScaleSetup
participant setPixelDataType
decodeImageFrameWorker->>getScalingMode: scalingParameters
getScalingMode-->>decodeImageFrameWorker: ScalingMode
decodeImageFrameWorker->>hasNonIntegerScaling: scalingParameters
hasNonIntegerScaling-->>decodeImageFrameWorker: hasFloatRescale
decodeImageFrameWorker->>PreScaleSetup: pixelData, hasFloatRescale
alt hasFloatRescale is true
PreScaleSetup->>PreScaleSetup: allocate Float32Array intermediate buffer
else integer scaling
PreScaleSetup->>PreScaleSetup: keep original typed array
end
PreScaleSetup-->>decodeImageFrameWorker: scaled pixelData
decodeImageFrameWorker->>setPixelDataType: pixelData
alt pixelData is Float32Array
setPixelDataType-->>decodeImageFrameWorker: skip re-typing
else other type
setPixelDataType->>setPixelDataType: getPixelDataTypeFromMinMax
setPixelDataType-->>decodeImageFrameWorker: selected typed array
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/utilities/windowLevel.ts (1)
61-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard logic is correct; comment is slightly imprecise.
The fix correctly falls back to
LINEAR_EXACTwhenwindowWidth <= 1, avoiding both the degenerate range (WW=1 → lower===upper) and the inverted range (WW<1 → upper<lower) that the LINEAR formula produces. The downstream shader receives these bounds, so preventing a zero-width range here eliminates the division-by-zero.One minor note: the comment says "WW <= 1 makes (WW-1) = 0," but
(WW-1)is only zero when WW is exactly 1. For WW < 1, the problem is an inverted range (upper < lower), not a zero-width one. Consider refining the comment for accuracy:📝 Suggested comment refinement
- // WW <= 1 makes (WW-1) = 0, so LINEAR/SAMPLED_SIGMOID produce lower === upper - // (division by zero). Fall back to LINEAR_EXACT. See `#2706`. + // WW <= 1 makes LINEAR/SAMPLED_SIGMOID produce a degenerate (WW=1: lower === upper) + // or inverted (WW<1: upper < lower) range, causing division by zero downstream. + // Fall back to LINEAR_EXACT. See `#2706`.🤖 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/windowLevel.ts` around lines 61 - 69, Refine the explanatory comment in windowLevel logic so it matches the actual behavior: in the window-level fallback branch in windowLevel.ts, keep the existing `voiLUTFunction` guard and `LINEAR_EXACT` fallback, but update the note to distinguish WW = 1 (zero-width range from WW-1 = 0) from WW < 1 (inverted lower/upper bounds). Refer to `VOILUTFunctionType.LINEAR`, `VOILUTFunctionType.SAMPLED_SIGMOID`, and `VOILUTFunctionType.LINEAR_EXACT` when revising the comment so the rationale stays accurate.
🤖 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/src/utilities/windowLevel.ts`:
- Around line 61-69: Refine the explanatory comment in windowLevel logic so it
matches the actual behavior: in the window-level fallback branch in
windowLevel.ts, keep the existing `voiLUTFunction` guard and `LINEAR_EXACT`
fallback, but update the note to distinguish WW = 1 (zero-width range from WW-1
= 0) from WW < 1 (inverted lower/upper bounds). Refer to
`VOILUTFunctionType.LINEAR`, `VOILUTFunctionType.SAMPLED_SIGMOID`, and
`VOILUTFunctionType.LINEAR_EXACT` when revising the comment so the rationale
stays accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2dcb4df7-d657-46ab-87e6-9fef982a2275
📒 Files selected for processing (10)
packages/core/src/types/ScalingParameters.tspackages/core/src/utilities/generateVolumePropsFromImageIds.tspackages/core/src/utilities/getScalingMode.tspackages/core/src/utilities/hasFloatScalingParameters.tspackages/core/src/utilities/index.tspackages/core/src/utilities/windowLevel.tspackages/dicomImageLoader/src/decodeImageFrameWorker.jspackages/dicomImageLoader/src/imageLoader/setPixelDataType.tspackages/dicomImageLoader/src/shared/scaling/getScalingMode.tspackages/dicomImageLoader/src/shared/scaling/scaleArray.ts
|
Thank you very much for reviewing this so carefully, and for laying out each point so clearly — it was really helpful. I've addressed all four:
While addressing these I also consolidated the modality branch selection into a single Please let me know if anything still looks off — happy to make further changes. Thank you again for the thorough review! |
3 participants
Context
Fixes #2706
DICOM images with non-integer
RescaleSlope(e.g. DTI FA maps withRescaleSlope=0.001) render as completely black in StackViewport. These images display correctly in other viewers such as RadiAnt and OsiriX.The legacy Cornerstone had a similar issue (cornerstonejs/cornerstone#302, fixed in PR #303).
Two independent bugs are involved:
getPixelDataTypeFromMinMax(0.0, 1.0)selectsUint8ArraybecauseNumber.isInteger(1.0) === truein JavaScript. All fractional scaled values (0.013, 0.5, 0.999) are truncated to 0. This function is called twice (web worker + main threadsetPixelDataType), destroying the data at both stages.toLowHighRangewithWindowWidth=1produceslower === upper === 0via the LINEAR formula(WW-1) = 0, causing division by zero in the VOI shader and a binary black/white image.Changes & Results
_handlePreScaleSetupindecodeImageFrameWorker.js: Detect non-integer scaling parameters and forceFloat32ArraybeforegetPixelDataTypeFromMinMaxis called.setPixelDataTypeinsetPixelDataType.ts: Skip re-typing ifpixelDatais alreadyFloat32Array, preventing the main thread from downgrading it toUint8Array.toLowHighRangeinwindowLevel.ts: WhenWindowWidth <= 1with LINEAR function, fall back to LINEAR_EXACT (lower = WC - WW/2,upper = WC + WW/2) to produce a valid range.All three fixes are needed — removing any one results in either a black screen or a binary black/white image.
Testing
RescaleSlope=0.001in StackViewport — should display grayscale instead of blackRescaleSlope=1) — should be unaffectedWindowWidth > 1—toLowHighRangeshould use existing LINEAR formula unchangedChecklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
I may be wrong about some of the details — please correct me if I've misunderstood anything. Any feedback would be greatly appreciated. Thank you!
Summary by CodeRabbit
New Features
Bug Fixes