fix: Use the image loader for dicom-seg to allow compressed images#2611
Open
wayfarer3130 wants to merge 87 commits into
Open
fix: Use the image loader for dicom-seg to allow compressed images#2611wayfarer3130 wants to merge 87 commits into
wayfarer3130 wants to merge 87 commits into
Conversation
* fix(security): Update lodash to 4.17.23 using a resolution to address CVE-2025-13465. * Removed lodash from the dev dependencies since it is in the resolutions now. * Update lodash to 4.17.23 in the docs package. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Joe Boccanfuso <joe.boccanfuso@radicalimaging.com>
* refactor: replace string literals with OrientationAxis enums for better type safety and clarity in BaseVolumeViewport and getCameraVectors * feat(rendering): Add function to calculate acquisition plane reformat orientation based on image orientation patient values * feat: enhance reformat orientation handling in VolumeViewport and BaseVolumeViewport, allowing for specific axial, sagittal, and coronal reformats while maintaining base orientation references * feat: add buttons for setting viewport orientations to non-reformat, reformat, and acquisition types, enhancing user control over viewport display * fix: suppress events during camera reset in VolumeViewport to prevent premature rendering * fix(getCameraVectors): import OrientationVectors type * fix(tests): update button text in MPR Reformat visual tests * fix(tests): update MPR Reformat screenshots for Chromium and WebKit --------- Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
--------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Joe Boccanfuso <joe.boccanfuso@radicalimaging.com>
* refactor the solution given * fix fetch color tables * important refactors to optimize worker functionality * refactor fetch palette data * fix clearly wrong test values * refactor as reviewer suggestions * fix: Convert 16 bit declared as 8 palette color data * fix: or condition on setting 16 bit palette data --------- Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
…eehandROI open U-shape contours (#2615) * feat(tools): implement orthogonalT and lineSegment modes for PlanarFreehandROI open U-shape contours Add two new rendering modes for the open U-shaped contour T-overlay: - 'orthogonalT': computes the T-line endpoint as the perpendicular ray-contour intersection from the chord midpoint, instead of the farthest point. Useful for LA cardiac workflows where the T must be perpendicular to the chord. - 'lineSegment': draws only the dashed chord connecting endpoints without any T-line, closing the contour visually. Also adds optional shaded fill for the enclosed region (contour + chord) when fillOpacity > 0, and centralizes variant dispatch into a single resolveVectorToPeak function to eliminate duplicated branching across drawLoop, openContourEditLoop, and renderMethods. * fix(tools): address PR review for PlanarFreehandROI U-shape modes - Use vec2.sub instead of create+set for chord vector - Replace double-loop ray intersection with single-pass dot-product approach for orthogonalT peak finding - Return null with console.warn instead of falling back to farthestT when no orthogonal intersection is found - Add 'none' option to U-shape dropdown to remove U-shape rendering - Auto-apply U-shape mode on dropdown change, remove separate button - Apply configured U-shape mode to newly drawn open contours - Add fill opacity dropdown using tool configuration - Read fillOpacity from tool config in renderMethods * refactor(tools): use vec2.dot for orthogonal T dot product calculation * Move open u shaped to utils helper --------- Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
* fix(security): Various dependency updates as a result of CVE-2026-26996. Ultimately the vulnerability was ignored because it is only exposed in itk-wasm via CLI and it is limited to build/dev environments.
* refactor: update getTextBoxCoordsCanvas function to accept element and textLines parameters for improved textbox placement across multiple annotation tools * fix: correct bounding box calculations in _drawTextGroup function to accurately reflect text box position and size * feat: implement text box overlap management with registry for improved placement accuracy - Added a text box overlap registry to track rendered text box positions. - Integrated overlap avoidance logic in getTextBoxCoordsCanvas to prevent occlusion of existing text boxes. - Updated draw and drawTextBox functions to clear and register text boxes during rendering. * refactor: enhance overlap detection in getTextBoxCoordsCanvas using AABB intersection - Replaced the previous rectangle overlap logic with a more accurate AABB intersection method. - Introduced a new utility function to convert text box rectangles to AABB format, allowing for better handling of overlap checks. - Updated the overlap detection functions to utilize the new intersection logic for improved accuracy in text box placement. * test: add unit tests for getTextBoxCoordsCanvas to validate positioning logic - Introduced a new test suite for getTextBoxCoordsCanvas to ensure correct coordinate calculations for text boxes. - Added tests for default positioning, overlap handling, and fallback mechanisms when space is limited. - Utilized a mock viewport element to simulate various scenarios for accurate testing. * style: format getTextBoxCoordsCanvas test cases for improved readability - Reformatted the test cases in getTextBoxCoordsCanvas.spec.ts to enhance code clarity. - Adjusted the indentation and line breaks for better visual structure without altering functionality. * refactor: update type assertions in getTextBoxCoordsCanvas tests for better type safety - Changed type assertions from any to unknown in getTextBoxCoordsCanvas test cases to enhance type safety and maintain code quality. - Ensured consistency in type handling across the test suite without altering existing functionality. * refactor: remove unnecessary type assertions in getTextBoxCoordsCanvas tests - Updated test cases in getTextBoxCoordsCanvas.spec.ts to eliminate type assertions from unknown to the actual type for annotationPoints and related variables. - Improved code clarity and type safety without changing the existing functionality of the tests. * chore: update tsconfig.json to exclude test files from compilation - Added an "exclude" property to tsconfig.json to prevent test files from being included in the TypeScript compilation process. - This change helps streamline the build process by omitting unnecessary test files. * refactor: integrate getTextBoxCoordsCanvas into ArrowAnnotateTool for improved text box positioning - Replaced direct canvas coordinate assignment with a call to getTextBoxCoordsCanvas to enhance the accuracy of text box placement. - This change ensures that text box coordinates are calculated based on the current canvas state and element, improving overall annotation functionality. * refactor: streamline text box rendering across annotation tools - Replaced direct text box rendering logic with a unified renderLinkedTextBoxAnnotation method in multiple annotation tools. - This change enhances code maintainability and consistency in text box visibility and positioning logic. - Improved handling of text box state and coordinates, ensuring better integration with existing annotation functionalities. --------- Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
* fix: Continuous flicker on viewer redraw * fix: Comment on size 0
…kfile changes. (#2629) Disabled all dependabot pull requests for bun and npm version updates.
…erstonejs/cornerstone3D into fix/use-imageLoader-for-seg
wayfarer3130
changed the title
jbocce
reviewed
May 19, 2026
| @@ -1,5 +1,6 @@ | |||
| { | |||
| "lockfileVersion": 1, | |||
| "configVersion": 0, | |||
Collaborator
There was a problem hiding this comment.
You might consider using the version of bun that CI uses. That said, I'm sure we should look at upgrading considering I suspect you are experiencing no problems with it?
jbocce
reviewed
May 19, 2026
Collaborator
There was a problem hiding this comment.
I think should be able to revert the changes in this file as you have no dependency changes and cornerstone is now at a later version. I suppose just update your branch.
jbocce
reviewed
May 19, 2026
| @@ -0,0 +1,210 @@ | |||
| const RLE_LOSSLESS_TRANSFER_SYNTAX_UID = '1.2.840.10008.1.2.5'; | |||
Collaborator
There was a problem hiding this comment.
Does it make sense to have these declared as constants in some central location?
Collaborator
|
Here are a list of things Claude considered must fixes...
|
Collaborator
|
@claude review |
jbocce
requested changes
May 19, 2026
jbocce
left a comment
jbocce
left a comment
Collaborator
There was a problem hiding this comment.
See the comments I have left thus far.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
…or-seg Resolve conflicts: adopt pnpm package manager from main (drop yarn/bun lockfiles and the yarn resolutions block), and align dcmjs to 0.52.0 across adapters/ai/metadata/utils with a regenerated pnpm-lock.yaml. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR upgrades SEG DICOM export and import with bitmap/RLE Lossless transfer-syntax support and per-frame functional group management. It adds new pixel-data encoding/decoding infrastructure, refactors segmentation generation for bitmap vs labelmap exports, extends RLE decoding for 1-bit frames, migrates labelmap import to frame-driven imageId resolution, and bumps dcmjs to 0.52.0 across packages. ChangesSEG DICOM Encoding and Export
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 6
♻️ Duplicate comments (1)
packages/adapters/src/adapters/Cornerstone/Segmentation_4X.js (1)
78-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame narrow frame regex issue as in generateSegmentation.ts.
The regex on line 96 has the same limitation as
generateSegmentation.ts- it won't match wadors imageId format (/frames/N). Consider extracting a shared helper function to avoid duplicating this logic and the bug.🤖 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/adapters/src/adapters/Cornerstone/Segmentation_4X.js` around lines 78 - 107, getReferencedSourceImageSequenceItemFromImage uses a narrow regex to extract frame from image.imageId and will miss wadors "/frames/N" paths (same bug as in generateSegmentation.ts); fix by extracting a shared helper (e.g., parseImageFrameNumber or getFrameNumberFromImageId) that accepts an imageId and returns a numeric frame by matching both query param patterns ([?&]frame=N) and path patterns (/frames/N or /frames/N/), update getReferencedSourceImageSequenceItemFromImage to call that helper and set ReferencedFrameNumber when the helper returns a finite >0 number, and replace the duplicate logic in generateSegmentation.ts to call the same helper so the behavior is centralized and consistent.
🤖 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.
Inline comments:
In `@packages/adapters/src/adapters/Cornerstone3D/encodePixelData.ts`:
- Around line 343-346: The current buffer creation in encodePixelData.ts treats
non-Uint16Array PixelData with new Uint16Array(dataset.PixelData as
ArrayBuffer), which wrongly interprets a Uint8Array by copying bytes into 16-bit
elements; update the logic to detect when dataset.PixelData is a Uint8Array and
create a byte-reinterpreted view using the underlying ArrayBuffer with the
Uint8Array's byteOffset and byteLength/2 so you get a proper Uint16Array view of
the same bytes; keep the existing branch for when dataset.PixelData is already a
Uint16Array and ensure the length calculation uses byteLength/2 to avoid
truncation.
- Around line 60-73: The out buffer is sized only to packedByteLength but the
loop treats multiple segments as if they can write into separate regions
(outIndex = s * packedByteLength), causing buffer overflows; in encodePixelData,
allocate out to accommodate all segments (e.g., new Uint8Array(packedByteLength
* numSegments) if segments are planar byte-planes) or, if only single-segment
packing is supported, validate numSegments === 1 and throw/handle otherwise;
update outIndex and endOfSegment calculations to use the new total length and
add bounds checks when copying segment data to prevent writes past out.buffer.
In
`@packages/adapters/src/adapters/Cornerstone3D/Segmentation/generateSegmentation.ts`:
- Around line 82-101: Extract a shared helper (e.g., getFrameNumberFromImageId)
that encapsulates the frame-extraction logic and replace the duplicated regex in
getReferencedSourceImageSequenceItem and
getReferencedSourceImageSequenceItemFromImage with calls to that helper; the
helper should accept an imageId string and return a numeric frame (or
undefined), matching both wadors "/frames/N" and query-param formats like
"?frame=N" (and fall back safely), then update
getReferencedSourceImageSequenceItem to call
getFrameNumberFromImageId(image?.imageId) and set ReferencedFrameNumber when
Number.isFinite and > 0.
In
`@packages/adapters/src/adapters/Cornerstone3D/Segmentation/labelmapImagesFromBuffer.ts`:
- Around line 1510-1523: The refactor changed the second parameter from
ArrayBuffer to segImageId causing silent failures (e.g.,
metadataProvider.get('instance', <ArrayBuffer>)) — update
createLabelmapsFromBufferInternal and createFromDICOMSegBuffer to detect the
legacy ArrayBuffer argument at runtime (using instanceof ArrayBuffer), log a
clear deprecation warning including which function was called and that callers
should pass a segImageId, then convert the ArrayBuffer to the new segImageId
flow or call the new createLabelmapsFromSegImageIds path (so
metadataProvider.get receives an instance id/string), and consider renaming
exports (e.g., createFromDICOMSegImageId) or add a deprecated wrapper that
forwards to the new implementation to avoid silent breakage; ensure warnings
reference the functions createLabelmapsFromBufferInternal,
createLabelmapsFromSegImageIds and createFromDICOMSegBuffer to help downstream
integrators migrate.
- Around line 182-198: The chunkPixelData helper currently sets maxBytesPerChunk
to Number.POSITIVE_INFINITY which disables chunking; change the default back to
a sensible finite value (e.g., 199_000_000) by updating the default
destructuring in chunkPixelData (function name: chunkPixelData, parameter:
options { maxBytesPerChunk?: number }) so that when options is omitted large
pixelData is split into chunks again; keep the existing loop and subarray logic
unchanged.
- Around line 200-252: The normalizeDecodedPixelData function incorrectly
converts Uint8Array frames to Uint16Array by calling new Uint16Array(frame),
which copies bytes as individual 16-bit values instead of reinterpreting the
underlying bytes; update the conversion in normalizeDecodedPixelData (the branch
that builds normalizedFrames when hasUint16Frame is true) to create a
Uint16Array view over the Uint8Array's buffer using new
Uint16Array(frame.buffer, frame.byteOffset, Math.floor(frame.byteLength / 2)) so
the bytes are reinterpreted as 16-bit samples (handle odd byteLength by using
Math.floor or pad upstream if needed); ensure the rest of the code that sums
frame.length and copies into combined continues to treat normalizedFrames
elements as 16-bit lengths.
---
Duplicate comments:
In `@packages/adapters/src/adapters/Cornerstone/Segmentation_4X.js`:
- Around line 78-107: getReferencedSourceImageSequenceItemFromImage uses a
narrow regex to extract frame from image.imageId and will miss wadors
"/frames/N" paths (same bug as in generateSegmentation.ts); fix by extracting a
shared helper (e.g., parseImageFrameNumber or getFrameNumberFromImageId) that
accepts an imageId and returns a numeric frame by matching both query param
patterns ([?&]frame=N) and path patterns (/frames/N or /frames/N/), update
getReferencedSourceImageSequenceItemFromImage to call that helper and set
ReferencedFrameNumber when the helper returns a finite >0 number, and replace
the duplicate logic in generateSegmentation.ts to call the same helper so the
behavior is centralized and consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 649559e5-084e-499e-986e-0118e23b8503
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
packages/adapters/package.jsonpackages/adapters/src/adapters/Cornerstone/Segmentation_4X.jspackages/adapters/src/adapters/Cornerstone3D/Segmentation/generateSegmentation.tspackages/adapters/src/adapters/Cornerstone3D/Segmentation/generateToolState.tspackages/adapters/src/adapters/Cornerstone3D/Segmentation/index.tspackages/adapters/src/adapters/Cornerstone3D/Segmentation/labelmapImagesFromBuffer.tspackages/adapters/src/adapters/Cornerstone3D/Segmentation/perFrameFunctionalGroups.jspackages/adapters/src/adapters/Cornerstone3D/encodePixelData.tspackages/ai/package.jsonpackages/dicomImageLoader/src/shared/decoders/decodeRLE.tspackages/metadata/package.jsonpackages/utils/package.json
Comment on lines
+60
to
+73
| const out = new Uint8Array(packedByteLength); | ||
| const numSegments = header.getInt32(0, true); | ||
|
|
||
| for (let s = 0; s < numSegments; s++) { | ||
| let outIndex = numSegments === 1 ? 0 : s * packedByteLength; | ||
| let inIndex = header.getInt32((s + 1) * 4, true); | ||
| let maxIndex = header.getInt32((s + 2) * 4, true); | ||
|
|
||
| if (maxIndex === 0) { | ||
| maxIndex = frameData.length; | ||
| } | ||
|
|
||
| const endOfSegment = | ||
| numSegments === 1 ? packedByteLength : (s + 1) * packedByteLength; |
There was a problem hiding this comment.
Buffer overflow for multi-segment RLE frames.
For multi-segment RLE data (e.g., 16-bit with 2 byte planes), outIndex is initialized to s * packedByteLength on line 64, which exceeds the out array length (packedByteLength) when s >= 1. This causes writes beyond the buffer bounds.
The output buffer should be sized for all segments if you intend to interleave, or segment data should be handled differently.
🐛 Proposed fix
If segments should be interleaved (planar byte planes), the buffer and indexing need adjustment:
- const out = new Uint8Array(packedByteLength);
+ const out = new Uint8Array(packedByteLength * numSegments);Or if the function should only handle single-segment 1-bit packed data, document and validate:
const numSegments = header.getInt32(0, true);
+
+ if (numSegments > 1) {
+ throw new Error(
+ `decodeRleLosslessToPackedBytes only supports single-segment RLE (got ${numSegments})`
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const out = new Uint8Array(packedByteLength); | |
| const numSegments = header.getInt32(0, true); | |
| for (let s = 0; s < numSegments; s++) { | |
| let outIndex = numSegments === 1 ? 0 : s * packedByteLength; | |
| let inIndex = header.getInt32((s + 1) * 4, true); | |
| let maxIndex = header.getInt32((s + 2) * 4, true); | |
| if (maxIndex === 0) { | |
| maxIndex = frameData.length; | |
| } | |
| const endOfSegment = | |
| numSegments === 1 ? packedByteLength : (s + 1) * packedByteLength; | |
| const out = new Uint8Array(packedByteLength * numSegments); | |
| const numSegments = header.getInt32(0, true); | |
| for (let s = 0; s < numSegments; s++) { | |
| let outIndex = numSegments === 1 ? 0 : s * packedByteLength; | |
| let inIndex = header.getInt32((s + 1) * 4, true); | |
| let maxIndex = header.getInt32((s + 2) * 4, true); | |
| if (maxIndex === 0) { | |
| maxIndex = frameData.length; | |
| } | |
| const endOfSegment = | |
| numSegments === 1 ? packedByteLength : (s + 1) * packedByteLength; |
🤖 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/adapters/src/adapters/Cornerstone3D/encodePixelData.ts` around lines
60 - 73, The out buffer is sized only to packedByteLength but the loop treats
multiple segments as if they can write into separate regions (outIndex = s *
packedByteLength), causing buffer overflows; in encodePixelData, allocate out to
accommodate all segments (e.g., new Uint8Array(packedByteLength * numSegments)
if segments are planar byte-planes) or, if only single-segment packing is
supported, validate numSegments === 1 and throw/handle otherwise; update
outIndex and endOfSegment calculations to use the new total length and add
bounds checks when copying segment data to prevent writes past out.buffer.
Comment on lines
+343
to
+346
| const buffer = | ||
| dataset.PixelData instanceof Uint16Array | ||
| ? dataset.PixelData | ||
| : new Uint16Array(dataset.PixelData as ArrayBuffer); |
There was a problem hiding this comment.
Incorrect byte reinterpretation for 16-bit PixelData when source is Uint8Array.
Using new Uint16Array(dataset.PixelData as ArrayBuffer) performs element-wise copy if PixelData is a Uint8Array, not byte reinterpretation. Each byte becomes a 16-bit value, producing wrong data.
🐛 Proposed fix to properly reinterpret bytes
const buffer =
dataset.PixelData instanceof Uint16Array
? dataset.PixelData
- : new Uint16Array(dataset.PixelData as ArrayBuffer);
+ : dataset.PixelData instanceof Uint8Array
+ ? new Uint16Array(
+ dataset.PixelData.buffer,
+ dataset.PixelData.byteOffset,
+ dataset.PixelData.byteLength / 2
+ )
+ : new Uint16Array(dataset.PixelData as ArrayBuffer);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const buffer = | |
| dataset.PixelData instanceof Uint16Array | |
| ? dataset.PixelData | |
| : new Uint16Array(dataset.PixelData as ArrayBuffer); | |
| const buffer = | |
| dataset.PixelData instanceof Uint16Array | |
| ? dataset.PixelData | |
| : dataset.PixelData instanceof Uint8Array | |
| ? new Uint16Array( | |
| dataset.PixelData.buffer, | |
| dataset.PixelData.byteOffset, | |
| dataset.PixelData.byteLength / 2 | |
| ) | |
| : new Uint16Array(dataset.PixelData as ArrayBuffer); |
🤖 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/adapters/src/adapters/Cornerstone3D/encodePixelData.ts` around lines
343 - 346, The current buffer creation in encodePixelData.ts treats
non-Uint16Array PixelData with new Uint16Array(dataset.PixelData as
ArrayBuffer), which wrongly interprets a Uint8Array by copying bytes into 16-bit
elements; update the logic to detect when dataset.PixelData is a Uint8Array and
create a byte-reinterpreted view using the underlying ArrayBuffer with the
Uint8Array's byteOffset and byteLength/2 so you get a proper Uint16Array view of
the same bytes; keep the existing branch for when dataset.PixelData is already a
Uint16Array and ensure the length calculation uses byteLength/2 to avoid
truncation.
Comment on lines
+82
to
+101
| function getReferencedSourceImageSequenceItem(image, metadata) { | ||
| const imageData = | ||
| metadata.get(MetadataModules.IMAGE_DATA, image?.imageId) || {}; | ||
| const referencedSOPInstanceUID = imageData.SOPInstanceUID; | ||
|
|
||
| const frameMatch = image?.imageId?.match?.(/[?&]frame=(\d+)/); | ||
| const referencedFrameNumber = frameMatch ? Number(frameMatch[1]) : undefined; | ||
| const item: { | ||
| ReferencedSOPInstanceUID: string; | ||
| ReferencedFrameNumber?: number; | ||
| } = { | ||
| ReferencedSOPInstanceUID: referencedSOPInstanceUID, | ||
| }; | ||
|
|
||
| if (Number.isFinite(referencedFrameNumber) && referencedFrameNumber > 0) { | ||
| item.ReferencedFrameNumber = referencedFrameNumber; | ||
| } | ||
|
|
||
| return item; | ||
| } |
There was a problem hiding this comment.
Duplicate frame-extraction regex in generateSegmentation.ts and Segmentation_4X.js.
Both getReferencedSourceImageSequenceItem (in generateSegmentation.ts) and getReferencedSourceImageSequenceItemFromImage (in Segmentation_4X.js) use the same narrow regex /[?&]frame=(\d+)/ that won't match wadors imageId format (/frames/N). Extract a shared helper (e.g., getFrameNumberFromImageId) in a common location to avoid duplicating this logic and ensure consistent behavior across both export paths.
🤖 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/adapters/src/adapters/Cornerstone3D/Segmentation/generateSegmentation.ts`
around lines 82 - 101, Extract a shared helper (e.g., getFrameNumberFromImageId)
that encapsulates the frame-extraction logic and replace the duplicated regex in
getReferencedSourceImageSequenceItem and
getReferencedSourceImageSequenceItemFromImage with calls to that helper; the
helper should accept an imageId string and return a numeric frame (or
undefined), matching both wadors "/frames/N" and query-param formats like
"?frame=N" (and fall back safely), then update
getReferencedSourceImageSequenceItem to call
getFrameNumberFromImageId(image?.imageId) and set ReferencedFrameNumber when
Number.isFinite and > 0.
Comment on lines
+182
to
+198
| function chunkPixelData( | ||
| pixelData, | ||
| options: { maxBytesPerChunk?: number } = {} | ||
| ) { | ||
| const { maxBytesPerChunk = Number.POSITIVE_INFINITY } = options; | ||
|
|
||
| if (pixelData.length <= maxBytesPerChunk) { | ||
| return [pixelData]; | ||
| } | ||
|
|
||
| const chunks = []; | ||
| for (let offset = 0; offset < pixelData.length; offset += maxBytesPerChunk) { | ||
| chunks.push(pixelData.subarray(offset, offset + maxBytesPerChunk)); | ||
| } | ||
|
|
||
| return chunks; | ||
| } |
There was a problem hiding this comment.
Restore the sensible default for maxBytesPerChunk.
The default was changed from 199_000_000 to Number.POSITIVE_INFINITY, which effectively disables chunking. Large SEG files may now block the main thread or exceed memory expectations on constrained devices.
🛠️ Suggested fix
function chunkPixelData(
pixelData,
options: { maxBytesPerChunk?: number } = {}
) {
- const { maxBytesPerChunk = Number.POSITIVE_INFINITY } = options;
+ const { maxBytesPerChunk = 199_000_000 } = options;
if (pixelData.length <= maxBytesPerChunk) {
return [pixelData];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function chunkPixelData( | |
| pixelData, | |
| options: { maxBytesPerChunk?: number } = {} | |
| ) { | |
| const { maxBytesPerChunk = Number.POSITIVE_INFINITY } = options; | |
| if (pixelData.length <= maxBytesPerChunk) { | |
| return [pixelData]; | |
| } | |
| const chunks = []; | |
| for (let offset = 0; offset < pixelData.length; offset += maxBytesPerChunk) { | |
| chunks.push(pixelData.subarray(offset, offset + maxBytesPerChunk)); | |
| } | |
| return chunks; | |
| } | |
| function chunkPixelData( | |
| pixelData, | |
| options: { maxBytesPerChunk?: number } = {} | |
| ) { | |
| const { maxBytesPerChunk = 199_000_000 } = options; | |
| if (pixelData.length <= maxBytesPerChunk) { | |
| return [pixelData]; | |
| } | |
| const chunks = []; | |
| for (let offset = 0; offset < pixelData.length; offset += maxBytesPerChunk) { | |
| chunks.push(pixelData.subarray(offset, offset + maxBytesPerChunk)); | |
| } | |
| return chunks; | |
| } |
🤖 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/adapters/src/adapters/Cornerstone3D/Segmentation/labelmapImagesFromBuffer.ts`
around lines 182 - 198, The chunkPixelData helper currently sets
maxBytesPerChunk to Number.POSITIVE_INFINITY which disables chunking; change the
default back to a sensible finite value (e.g., 199_000_000) by updating the
default destructuring in chunkPixelData (function name: chunkPixelData,
parameter: options { maxBytesPerChunk?: number }) so that when options is
omitted large pixelData is split into chunks again; keep the existing loop and
subarray logic unchanged.
Comment on lines
+200
to
+252
| function normalizeDecodedPixelData(pixelData) { | ||
| if (Array.isArray(pixelData)) { | ||
| if (pixelData.length === 1) { | ||
| return normalizeDecodedPixelData(pixelData[0]); | ||
| } | ||
|
|
||
| const hasUint16Frame = pixelData.some( | ||
| (frame) => frame instanceof Uint16Array | ||
| ); | ||
|
|
||
| if (hasUint16Frame) { | ||
| const normalizedFrames = pixelData.map((frame) => | ||
| frame instanceof Uint16Array | ||
| ? frame | ||
| : frame instanceof Uint8Array | ||
| ? new Uint16Array(frame) | ||
| : new Uint16Array(frame) | ||
| ); | ||
| const totalLength = normalizedFrames.reduce( | ||
| (acc, frame) => acc + frame.length, | ||
| 0 | ||
| ); | ||
| const combined = new Uint16Array(totalLength); | ||
| let offset = 0; | ||
| for (const frame of normalizedFrames) { | ||
| combined.set(frame, offset); | ||
| offset += frame.length; | ||
| } | ||
| return combined; | ||
| } | ||
|
|
||
| const normalizedFrames = pixelData.map((frame) => | ||
| frame instanceof Uint8Array ? frame : new Uint8Array(frame) | ||
| ); | ||
| const totalLength = normalizedFrames.reduce( | ||
| (acc, frame) => acc + frame.length, | ||
| 0 | ||
| ); | ||
| const combined = new Uint8Array(totalLength); | ||
| let offset = 0; | ||
| for (const frame of normalizedFrames) { | ||
| combined.set(frame, offset); | ||
| offset += frame.length; | ||
| } | ||
| return combined; | ||
| } | ||
|
|
||
| if (pixelData instanceof Uint8Array || pixelData instanceof Uint16Array) { | ||
| return pixelData; | ||
| } | ||
|
|
||
| return new Uint8Array(pixelData); | ||
| } |
There was a problem hiding this comment.
Incorrect byte interpretation when converting Uint8Array to Uint16Array.
Line 215 uses new Uint16Array(frame) which copies values rather than reinterpreting bytes. If frame is a Uint8Array containing packed 16-bit pixel data (2 bytes per sample), this creates a Uint16Array where each original byte becomes a separate 16-bit value, causing length/value mismatch.
While SEG typically uses 1- or 8-bit today, this is a latent bug if 16-bit bitmap SEG appears.
🐛 Suggested fix for proper byte reinterpretation
if (hasUint16Frame) {
const normalizedFrames = pixelData.map((frame) =>
frame instanceof Uint16Array
? frame
: frame instanceof Uint8Array
- ? new Uint16Array(frame)
- : new Uint16Array(frame)
+ ? new Uint16Array(frame.buffer, frame.byteOffset, frame.byteLength / 2)
+ : new Uint16Array(frame)
);🤖 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/adapters/src/adapters/Cornerstone3D/Segmentation/labelmapImagesFromBuffer.ts`
around lines 200 - 252, The normalizeDecodedPixelData function incorrectly
converts Uint8Array frames to Uint16Array by calling new Uint16Array(frame),
which copies bytes as individual 16-bit values instead of reinterpreting the
underlying bytes; update the conversion in normalizeDecodedPixelData (the branch
that builds normalizedFrames when hasUint16Frame is true) to create a
Uint16Array view over the Uint8Array's buffer using new
Uint16Array(frame.buffer, frame.byteOffset, Math.floor(frame.byteLength / 2)) so
the bytes are reinterpreted as 16-bit samples (handle odd byteLength by using
Math.floor or pad upstream if needed); ensure the rest of the code that sums
frame.length and copies into combined continues to treat normalizedFrames
elements as 16-bit lengths.
Comment on lines
+1510
to
+1523
| /** @deprecated Use createLabelmapsFromSegImageIds */ | ||
| async function createLabelmapsFromBufferInternal( | ||
| referencedImageIds, | ||
| segImageId, | ||
| metadataProvider, | ||
| options | ||
| ) { | ||
| return createLabelmapsFromSegImageIds( | ||
| referencedImageIds, | ||
| segImageId, | ||
| metadataProvider, | ||
| options | ||
| ); | ||
| } |
There was a problem hiding this comment.
Breaking API changes affecting downstream consumers in labelmapImagesFromBuffer.ts and generateToolState.ts.
Both createLabelmapsFromBufferInternal (in labelmapImagesFromBuffer.ts) and createFromDICOMSegBuffer (in generateToolState.ts) changed their second argument from ArrayBuffer to segImageId (string). Downstream consumers (e.g., OHIF integrators) still passing an ArrayBuffer will experience silent failures when metadataProvider.get('instance', <ArrayBuffer>) is called.
Root cause: The refactor to use frame-driven imageId resolution changed the API contract without backward compatibility or clear migration path.
Recommended actions:
- Add runtime detection for legacy
ArrayBufferargument and emit deprecation warning - Rename functions to avoid silent breakage (e.g.,
createFromDICOMSegImageId) - Document breaking changes in PR body and changelog
🤖 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/adapters/src/adapters/Cornerstone3D/Segmentation/labelmapImagesFromBuffer.ts`
around lines 1510 - 1523, The refactor changed the second parameter from
ArrayBuffer to segImageId causing silent failures (e.g.,
metadataProvider.get('instance', <ArrayBuffer>)) — update
createLabelmapsFromBufferInternal and createFromDICOMSegBuffer to detect the
legacy ArrayBuffer argument at runtime (using instanceof ArrayBuffer), log a
clear deprecation warning including which function was called and that callers
should pass a segImageId, then convert the ArrayBuffer to the new segImageId
flow or call the new createLabelmapsFromSegImageIds path (so
metadataProvider.get receives an instance id/string), and consider renaming
exports (e.g., createFromDICOMSegImageId) or add a deprecated wrapper that
forwards to the new implementation to avoid silent breakage; ensure warnings
reference the functions createLabelmapsFromBufferInternal,
createLabelmapsFromSegImageIds and createFromDICOMSegBuffer to help downstream
integrators migrate.
…or-seg Resolve packages/metadata conflict: take main's @cornerstonejs/utils 5.0.8 (matches the rest of the monorepo) and keep dcmjs 0.52.0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Used the new metadata module to get this
This was correct as a value copy |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
The previous version of the segmentation loader claimed to support labelmap, but didn't actually work for loading them. As well, both bitmap and labelmap supposedly had RLE support, but again it didn't quite work.
This change adds storing as either labelmap or bitmap, and allows storing in RLE. The RLE encoder is also fixed to support 1,8,16 bits, and actually does the RLE correctly instead of using a count of 1 for every pixel.
The result of this is far faster loading of seg images from standard back ends supporting the SEG frame retrieve rather than trying to retrieve the entire DICOM file. On storage, the store size is also significantly smaller as RLE compresses extremely well for segmentation images.
OHIF_REF: feat/load-seg-images
Changes & Results
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
Chores
New Features
Bug Fixes