fix(dicom): match the volume buffer to the decoded pixel values - #934
Open
PaulHax wants to merge 1 commit into
Open
fix(dicom): match the volume buffer to the decoded pixel values#934PaulHax wants to merge 1 commit into
PaulHax wants to merge 1 commit into
Conversation
The volume buffer's element type was derived from the first instance's stored range put through its rescale, floored and ceiled to integers. ITK applies the rescale while decoding and picks the output type from it: an integral slope and intercept keep an integer type, anything fractional produces float64. A fractional rescale whose endpoints landed on integers got an integer buffer, and every fraction was dropped on write. An integral range past 32 bits threw at allocation. A later instance decoding outside the first instance's type wrote through TypedArray.set with no error, wrapping every value the type cannot hold. Allocate for the union of every instance's declared range, choosing Float64Array whenever the slope or intercept is fractional or the range is not a safe 32-bit integer range, and the narrowest integer type otherwise. Ordinary CT and MR allocation is unchanged. An instance whose tags give no finite range is left out of the union and judged on its decoded values. Check each chunk after it decodes and before it is written. Its actual range must fit the buffer's element type, and float output bound for an integer buffer must be whole numbers. Both rejections name the file, the values it decoded to, and the buffer it has to fit. Only decoder output that disagrees with the tags can fail these checks. Reproduced with a public DSC perfusion series whose slope is 112067.85375182 over 16-bit unsigned storage.
✅ Deploy Preview for volview-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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
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.
Problem
The volume buffer's element type was chosen once, from the first instance:
allocateImageFromChunkstook its stored range, put it through RescaleSlopeand RescaleIntercept, floored and ceiled the endpoints, and picked the
narrowest typed array that holds the result. ITK/GDCM applies the rescale
while decoding, so a chunk arrives already in output units, and ITK picks the
output scalar type from the rescale: an integral slope and intercept keep an
integer type, anything fractional produces float64.
That allocation went wrong three ways, and none of them raised anything:
integer buffer, and every fraction was dropped on write. This was reproduced
with a DSC perfusion series from a public archive whose slope is
112067.85375182over 16-bit unsigned storage. ITK returns float64; theallocator chose an integer buffer and truncated every voxel.
which threw at allocation and sank the whole series.
Every instance decodes to the same number of elements whatever its BitsStored
or rescale, so
TypedArray.prototype.setcopied it into its slot andconverted silently: negative values in an unsigned buffer wrap to huge
positives, 16-bit values in a
Uint8Arraykeep only their low byte. Theslice looks plausible enough to miss.
Change
Allocation follows ITK
getVolumeBufferTypeinallocateImageFromChunks.tsnow decides the way ITKdoes, for the union of every instance's declared range rather than the first
instance's:
Float64Array, evenwhen both transformed endpoints are integers.
signed or unsigned 32-bit range, allocates
Float64Arrayinstead ofthrowing at allocation.
homogeneous CT and MR allocation is unchanged.
getRescaledValueRangereturns the exact transformed endpoints. The floorand ceil that hid fractional endpoints are gone, and a negative slope orders
them.
Allocating for the union means a series that mixes bit depths, or carries a
per-instance rescale, gets a buffer every declared instance fits, so no
well-formed file is rejected for its tags. An instance whose tags give no
finite range (BitsStored absent or unparseable, or a rescale that overflows)
is left out of the union and judged on its decoded values when it decodes; the
allocator throws only when no instance is usable.
The write is guarded
dicomChunkImage.tschecks each chunk after it decodes and immediately beforepixelData.set. The per-component ranges it already computed for the scalarrange update move above the write and are reused, so the chunk's actual min
and max cost nothing extra. Two checks, each with its own diagnostic naming
the file, the values it decoded to, and the buffer it has to fit:
valuesFitBufferrejects a range outside the buffer's element type. Thebound is read off the buffer itself,
getBufferValueRange(pixelData), notre-derived from metadata.
samplesAreIntegralrejects float decoder output with fractional samplesbound for an integer buffer. It scans samples only when the decoder returned
a float or plain array, so ordinary integer data still costs one min/max
pass.
Because the buffer is allocated for what every instance declares, these
checks can only fail when decoder output disagrees with the tags. They are a
guard against that disagreement, not a rule about which series load.
Supporting changes:
getPixelFormat,getRescaledValueRange, andgetVolumeBufferTypereplacethe private
getTypedArrayConstructor.numberOrtreats a zero-length or whitespace-only element as absent.Previously
Number('')is 0, so an empty RescaleSlope read as slope 0 andcollapsed the range to the intercept.
DicomChunkImagegains an optional constructor seam forsplitAndSortandthe ITK read, defaulting to the real collaborators, so unit tests can drive
the volume path with a chosen decoder.
tests/specs/syntheticDicom.tsgains pixel-format knobs (bitsAllocated,bitsStored,highBit,pixelRepresentation,rescaleSlope,rescaleIntercept) and apixelValuewritten to every sample, defaulting to0, which is byte for byte what the helper emitted before. It pads 8-bit
pixel data to an even length, since DICOM element values must be even, and
rejects a
bitsAllocatedother than 8 or 16 instead of emitting a 32-bit OWelement no real writer would produce.
Costs
declaring BitsStored 16 allocated
Int16Arrayfrom its first instance andnow allocates
Int32Arrayfor the union, doubling memory even though everysample fits 16 bits. Narrowing to the decoded range would need reallocation
at decode time.
vtk.js converts the array to float32 for texture upload.
can decode such an instance at all is a separate question; if it cannot,
that chunk fails on its own instead of sinking the volume.
Known gaps
Chunks already marked Errored are not re-evaluated. A second
addChunksreallocates the volume, possibly wider, but a chunk that failed earlier keeps
its Errored status. This is the pre-existing pattern for every chunk error.
The guard runs per chunk at decode time. Flagging an inhomogeneous series
at grouping time would be the better place, and is a larger change.
Tests
allocateImageFromChunks.spec.ts: pixel format parsing, rescaled rangeordering, the DSC slope, slope 0.2 with integral endpoints, an integral
range past 32 bits, slope -1 ordering, a mixed union, a non-finite range, an
unusable instance left out of the union, buffer range lookups, and both
guard predicates.
dicomChunkImage.spec.ts: a volume built from the DSC slope and an observedstored maximum of 65131 keeps its
Float64Arrayand exact values; a decodedchunk outside its buffer's range, and one with fractional samples bound for
an integer buffer, are each rejected with their own diagnostic while the
image still settles.
dicom-modality-rescale.e2e.ts: three generated 16-bit files with the DSCslope load in Chrome with no notification; the cached volume's scalars are
Float64Arraywith exact rescaled values. It reads the image cache throughthe helper the DICOM and cine spacing tests already use. The window/level
range shown in the UI is not an alternative: it is set from the decoded ITK
values, not read back from the buffer, so it would show the same range over
a wrapped integer buffer.