Repository navigation
Add direct ITK to DICOM Labelmap conversion - #536
Conversation
When converting DICOM binary segmentations to DICOM labelmaps, some pixels are likely not covered from any of the the segments. Since the pixels are initially set to 0, all non-covered pixels will be "background" and must be marked as such. This is done by scanning all pixels after conversion and if a 0 is found, a segment using codes for denoting "background" will be inserted automatically. The alternative would be to set Pixel Padding Value. This could be introduced as an option later, but for now the background segment seems to be better supported by consuming applications.
Also for the newly added background segment, don't write (0062,000D) Recommended Display CIELab Value either. Also some minor changes.
Performance optimizations for segmentation/parametric maps (dcmiod, dcmfg, dcmseg). It also includes a simpler mechanism to add external modules to the DCMTK CMake build (e.g. the JPEG2000 codec from DCMTKcs, see https://github.com/lassoan/DCMTKcs).
Extend itkimage2segimage with a --segmentationType option (default
"binary", preserving prior behavior) that, when set to "labelmap",
emits a DICOM Labelmap Segmentation (Sup 243) directly from an ITK
image. Previously the only way to produce a labelmap was to convert
to a binary Segmentation first and then run bin2labelsegimage;
this removes the intermediate step.
In labelmap mode foreground segments are numbered 1..N (or taken
from labelIDs when --useLabelIDAsSegmentNumber is set); pixel value
0 is reserved for the implicit background. If any frame contains
pixel 0 a Background segment (Property Type DCM 125040) is added
automatically so every pixel value is covered by a Segment Sequence
item, as required by Sup 243. Bit depth is 8-bit when the segment
count fits, else 16-bit. Overlapping foreground segments are
rejected because each labelmap pixel can carry only one segment
number.
The frame-scan + background-segment-creation logic that this new
path needs is identical to what Bin2Label already does for
binary->labelmap conversion. It is consolidated into a shared
ConverterBase::addBackgroundSegmentIfNeeded() helper, and
DcmBinToLabelConverter's member function becomes a thin wrapper
that delegates and adds the PALETTE-mode LUT prepend on top.
Also adds a 2-segment metadata example
(doc/examples/seg-example_liver_spine.json) used by new CTest
entries:
- itkimage2segimage_makeSEG_labelmap: single-segment conversion.
- itkimage2segimage_makeSEG_labelmap_multi: multi-segment
conversion. Regression guard for segment numbering, which is
not exercised by single-segment inputs.
- itkimage2segimage_makeSEG_labelmap_multi_labelID: same with
--useLabelIDAsSegmentNumber.
- itkimage2segimage_makeSEG_labelmap_overlap: liver/heart pair
overlaps on 522 pixels; conversion must fail (WILL_FAIL).
- segimage2itkimage_makeNRRD_from_labelmap_{direct,multi}:
pixel-perfect round-trip back to NRRD against existing baselines.
dciodvfy is intentionally not run on the new labelmap outputs;
all labelmap segmentations emitted by dcmqi today (both Bin2Label
and this new path) fail dciodvfy because Type 2C PatientOrientation
is not populated under GeneralImage. This is a pre-existing
gap and is documented inline in the CMakeLists.
There was a problem hiding this comment.
Review: Direct ITK-to-DICOM Labelmap Segmentation
+743/-45 lines | 13 files changed
Overview
Adds a --segmentationType labelmap option to itkimage2segimage, enabling direct ITK→DICOM Labelmap Segmentation (Sup 243) conversion without the previous two-step binary→labelmap pipeline. Consolidates shared background-segment creation logic into ConverterBase::addBackgroundSegmentIfNeeded(), fixes a stale fgder deletion guard, improves memory safety by replacing new Uint8[] with std::vector, removes debug prints, and bumps the DCMTK dependency to a May 2026 commit.
Positives
- Refactoring is well-designed. Extracting the background-segment logic into
ConverterBaseis clean;DcmBinToLabelConverter::addBackgroundSegmentIfNeeded()becomes a thin wrapper adding only the PALETTE-specific LUT prepend. - Test coverage is thorough: single-segment, multi-segment,
--useLabelIDAsSegmentNumber, expected-failure overlap, and pixel-perfect NRRD round-trips. - Memory safety improvement.
Uint8 *frameData = new Uint8[frameSize]is now astd::vector<Uint8>— both for the binary path (fix) and the new labelmap path (clean from the start). - Correct derivation-image pre-computation. The new
slice2derimgPerFilevector computes geometry maps once before the slice loop, avoiding repeated work per segment file. segmentsOverlap = "NO"for labelmap output is correct since the converter explicitly rejects overlapping segments.fgderdeletion fix (removingif(referencesGeometryCheck)guard) is correct —fgderis always allocated.- Documentation is solid. Docstrings cite Sup 243 section numbers; the dciodvfy gap is explained inline with a clear TODO path.
Issues and Concerns
1. Wrong <channel> in the XML descriptor (itkimage2segimage.xml)
<channel>output</channel>segmentationType controls conversion input behavior. Should almost certainly be <channel>input</channel>, like the other control flags (useLabelIDAsSegmentNumber, compress, etc.). As written, Slicer module auto-generation may misclassify this parameter.
2. Memory leak on CHECK_COND early exits inside the labelmap block (Itk2DicomConverter.cpp)
CHECK_COND expands to return NULL on failure. fgppp, fgfc, and fgder are allocated before the labelmap block and deleted at the end, but CHECK_COND calls like fgfc->setStackID(...), fgfc->setInStackPositionNumber(...), etc., can bypass those deletes. The releaseFGs lambda handles the two explicit error paths, but CHECK_COND bypasses it entirely. This is a pre-existing pattern in the binary path too, but the new path adds substantially more CHECK_COND calls. Consider replacing the raw pointers with std::unique_ptr or wrapping them in RAII.
3. CIELabColor::prepend() as
8000
sumes nothrow new (Bin2Label.h)
Uint16* newL = new Uint16[newSize];
Uint16* newA = new Uint16[newSize];
Uint16* newB = new Uint16[newSize];
if (!newL || !newA || !newB) // null-checks after standard newStandard operator new throws std::bad_alloc on failure rather than returning null. The null guard is only effective if DCMTK is built with a nothrow allocator. Should use new (std::nothrow) explicitly, or restructure with std::vector.
4. Unconditional DCM_RecommendedDisplayCIELabValue rule deletion in copySegments (Bin2Label.cpp)
The old code deleted this rule only in PALETTE mode. The new code deletes it unconditionally. If this is intentional for MONOCHROME2 labelmap output too (e.g., "Sup 243 prohibits per-segment CIELab in labelmap mode regardless of color model"), a brief comment would prevent future readers from reverting it.
5. Pre-existing dciodvfy compliance gap (documented but worth a separate issue)
The Testing/CMakeLists.txt comment clearly documents that all labelmap output fails dciodvfy validation because PatientOrientation is not populated in GeneralImage. This should be tracked as a separate issue so it doesn't get lost.
Summary
Solid PR overall. The architecture is clean, tests are comprehensive, and the DICOM spec compliance reasoning is well-documented. The <channel>output</channel> bug in the XML (issue 1) is the most impactful issue — it affects Slicer module auto-generation. The memory safety concern (issue 2) is worth addressing before merge. Items 3 and 4 are lower priority but worth a comment or minor fix.
|
@michaelonken I think this one (pre-existing) is incorrect as well - should be input! dcmqi/apps/seg/itkimage2segimage.xml Lines 85 to 92 in 8b161f9 |
yes, its actually an input flag; since it controls the checking of the output I accidentally selected output. I must check this issue though:
|
This regression has been introduced in 8bf5e5 and has not fully been fixed in later commits.
|
I think its debatable whether the --noDicomValueChecks is an input or output option. Since the tool (itk2segimage) writes DICOM as an output and the the option allows skipping checks while writing the output, I would prefer keeping it as an output option. |
Previously noDicomValueChecks has been marked as output parameter which is not correct in the semantics of the Slicer execution model.
|
@pieper can you help us resolve this - I believe in Slicer SEM any parameter that is passed to a CLI should be "input" channel, right? |
|
Yes, by Slicer CLI semantics it's a |
|
Got it 👍 |
DICOM Labelmap Segmentation objects reserve pixel value 0 for background and may include it as a defined segment (SegmentNumber 0) in the segment sequence. The DICOMSegmentationPlugin was assigning metadata positionally from the JSON produced by segimage2itkimage, which lists the background entry first. Since ImportLabelmapToSegmentationNode does not create a VTK segment for pixel value 0, every subsequent segment received the metadata of its predecessor (e.g. skin appeared as costal cartilage). dcmqi 1.5.4 labelmap conversion will generate a segment entry for the background (introduced in QIICR/dcmqi#536, leading to shift of metadata assignment. Fix: skip any segment entry with labelID == 0 before building the labelAttributes list. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
DICOM Labelmap Segmentation objects reserve pixel value 0 for background and may include it as a defined segment (SegmentNumber 0) in the segment sequence. The DICOMSegmentationPlugin was assigning metadata positionally from the JSON produced by segimage2itkimage, which lists the background entry first. Since ImportLabelmapToSegmentationNode does not create a VTK segment for pixel value 0, every subsequent segment received the metadata of its predecessor (e.g. skin appeared as costal cartilage). dcmqi 1.5.4 labelmap conversion will generate a segment entry for the background (introduced in QIICR/dcmqi#536, leading to shift of metadata assignment. Fix: skip any segment entry with labelID == 0 before building the labelAttributes list. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
DICOM Labelmap Segmentation objects reserve pixel value 0 for background and may include it as a defined segment (SegmentNumber 0) in the segment sequence. The DICOMSegmentationPlugin was assigning metadata positionally from the JSON produced by segimage2itkimage, which lists the background entry first. Since ImportLabelmapToSegmentationNode does not create a VTK segment for pixel value 0, every subsequent segment received the metadata of its predecessor (e.g. skin appeared as costal cartilage). dcmqi 1.5.4 labelmap conversion will generate a segment entry for the background (introduced in QIICR/dcmqi#536, leading to shift of metadata assignment. Fix: skip any segment entry with labelID == 0 before building the labelAttributes list. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
DICOM Labelmap Segmentation objects reserve pixel value 0 for background and may include it as a defined segment (SegmentNumber 0) in the segment sequence. The DICOMSegmentationPlugin was assigning metadata positionally from the JSON produced by segimage2itkimage, which lists the background entry first. Since ImportLabelmapToSegmentationNode does not create a VTK segment for pixel value 0, every subsequent segment received the metadata of its predecessor (e.g. skin appeared as costal cartilage). dcmqi 1.5.4 labelmap conversion will generate a segment entry for the background (introduced in QIICR/dcmqi#536, leading to shift of metadata assignment. Fix: skip any segment entry with labelID == 0 before building the labelAttributes list. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…9163) * BUG: Fix segment metadata off-by-one when loading DICOM Labelmap SEG DICOM Labelmap Segmentation objects reserve pixel value 0 for background and may include it as a defined segment (SegmentNumber 0) in the segment sequence. The DICOMSegmentationPlugin was assigning metadata positionally from the JSON produced by segimage2itkimage, which lists the background entry first. Since ImportLabelmapToSegmentationNode does not create a VTK segment for pixel value 0, every subsequent segment received the metadata of its predecessor (e.g. skin appeared as costal cartilage). dcmqi 1.5.4 labelmap conversion will generate a segment entry for the background (introduced in QIICR/dcmqi#536, leading to shift of metadata assignment. Fix: skip any segment entry with labelID == 0 before building the labelAttributes list. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * ENH: Warn when labelID 0 segment type code is not Background in DICOM SEG Log a warning if a segment with labelID 0 has a type code other than DCM:125040 (Background), noting it will not be loaded as a Slicer segment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Add direct ITK to DICOM Labelmap conversion.
Also includes PRs #534 and #535 (the latter being probably optional).
Extend itkimage2segimage with a --segmentationType option (default
"binary", preserving prior behavior) that, when set to "labelmap",
emits a DICOM Labelmap Segmentation (Sup 243) directly from an ITK
image. Previously the only way to produce a labelmap was to convert
to a binary Segmentation first and then run bin2labelsegimage;
this removes the intermediate step.
In labelmap mode foreground segments are numbered 1..N (or taken
from labelIDs when --useLabelIDAsSegmentNumber is set); pixel value
0 is reserved for the implicit background. If any frame contains
pixel 0 a Background segment (Property Type DCM 125040) is added
automatically so every pixel value is covered by a Segment Sequence
item, as required by Sup 243. Bit depth is 8-bit when the segment
count fits, else 16-bit. Overlapping foreground segments are
rejected because each labelmap pixel can carry only one segment
number.
The frame-scan + background-segment-creation logic that this new
path needs is identical to what Bin2Label already does for
binary->labelmap conversion. It is consolidated into a shared
ConverterBase::addBackgroundSegmentIfNeeded() helper, and
DcmBinToLabelConverter's member function becomes a thin wrapper
that delegates and adds the PALETTE-mode LUT prepend on top.
Also adds a 2-segment metadata example
(doc/examples/seg-example_liver_spine.json) used by new CTest
entries:
conversion. Regression guard for segment numbering, which is
not exercised by single-segment inputs.
--useLabelIDAsSegmentNumber.
overlaps on 522 pixels; conversion must fail (WILL_FAIL).
pixel-perfect round-trip back to NRRD against existing baselines.
dciodvfy is intentionally not run on the new labelmap outputs;
all labelmap segmentations emitted by dcmqi today (both Bin2Label
and this new path) fail dciodvfy because Type 2C PatientOrientation
is not populated under GeneralImage. This is a pre-existing
gap and is documented inline in the CMakeLists.