InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

BUG: DCMTKImageIO and GDCMImageIO MetaDataDictionary population gaps (binary-VR test coverage, pixel-data tag skips, SQ, numeric VRs)

Open
#6,720 0 comments 0 reactions 0 assignees View on GitHub
area:IO type:Bug type:Testing
Dominant language
C++
Stars
1.7k
Forks
748
Avg merge
1d 1h
Merged PRs (30d)
64

Description

PRs #6217 (DCMTKImageIO metadata population) and #6218 (GDCMImageIO OD/OL/OV coverage) closed the acute gaps but deliberately deferred several P2 items. They share one root cause: ITK's two DICOM ImageIO modules independently re-implement metadata extraction, so every fix has to land twice and be tested against fixtures that do not yet exist.

A — Binary-VR base64 path has zero test coverage (both modules)

`DCMTKFileReader::PopulateMetaDataDictionary()` base64-encodes `EVR_OB / OW / OF / OD / OL / OV / UN / ox / px` (`Modules/IO/DCMTK/src/itkDCMTKFileReader.cxx:1425`). The test that covers it, `itkDCMTKImageIOMetadataTest`, reads `DATA{${ITK_DATA_ROOT}/Input/DicomSeries/Image0075.dcm}` (`Modules/IO/DCMTK/test/CMakeLists.txt:42-47`), which contains **no binary-VR header tags** — so the branch never executes. The GDCM side (#6218) added no metadata test at all; `Modules/IO/GDCM/test/` has no `*Metadata*` source.

Brad's explicit ask on #6217:
> *"Is there any way we could have coverage to testing for more VR types to ensure they are handled correctly?"*

What's needed:
1. A DICOM fixture in `ITKData/Input/` carrying at least one tag per binary VR class (`OB`, `OW`, `OD`, `OL`, `OV`). **This must land in `InsightSoftwareConsortium/ITKData` first** — verify the fixture resolves before any test references it.
2. Parallel tests on `itkDCMTKImageIOMetadataTest` and a new `itkGDCMImageIOMetadataTest` that (a) read the fixture, (b) assert the `GGGG|EEEE` keys exist, (c) assert the value is a well-formed base64 string (valid alphabet, length a multiple of 4), (d) optionally decode and compare against a known-good byte sequence.

B — Encapsulated / float pixel-data tags are not skipped (both modules)

Both modules skip only `(7fe0,0010)` Pixel Data:
- `Modules/IO/DCMTK/src/itkDCMTKFileReader.cxx:1409-1412`
- `Modules/IO/GDCM/src/itkGDCMImageIO.cxx:810`

DICOM also standardizes `(7fe0,0008)` Float Pixel Data, `(7fe0,0009)` Double Float Pixel Data, and encapsulated pixel-data sequences. Files using those will base64-encode multi-megabyte bulk data into the metadata dictionary. Rare in practice today, but standardized.

Fix: widen the skip predicate to `(7fe0,0008)`–`(7fe0,0010)` plus encapsulated-sequence tags, in **both** modules. Small, standalone `BUG:` PR.

C — `dict.Clear()` is unconditional (DCMTK)

`Modules/IO/DCMTK/src/itkDCMTKFileReader.cxx:1395` calls `dict.Clear()` before populating. A caller that pre-populated the dictionary with application-specific metadata has it silently wiped. Either document the clear-and-replace contract, or switch to additive population that overwrites only the keys it produces. GDCM's behaviour here differs; align the two.

C2 — Wrong DICOM tag name in a test comment (one-liner)

`Modules/IO/DCMTK/test/itkDCMTKGetDicomTagsTest.cxx` comments the block as `// (0008,0021) DA StudyDate`. `(0008,0021)` is **SeriesDate**; StudyDate is `(0008,0020)`. #6217's new test inherited this by copy-paste and was corrected in `17a89dbd`, but the original is still wrong.

E — Numeric-VR values stored as strings

Both modules encapsulate `IS / DS / FD / FL / SL / SS / UL / US` as their string form (`"3.14"`, `"1\\2\\3"`). This matches GDCM's historical behaviour and what most consumers expect, but loses type fidelity. A future enhancement could detect numeric VRs and encapsulate via `EncapsulateMetaData` / `` / `>`. Because it would break consumers parsing the string form, it needs an `ITK_FUTURE_LEGACY_REMOVE` opt-in path.

F — Sequence (SQ) items never appear in the dictionary

`EVR_SQ` is explicitly skipped (`itkDCMTKFileReader.cxx:1419-1423`, with a comment noting `getUint8Array()` cannot return nested-dataset content); GDCM skips `gdcm::VR::SQ` for the same reason. A future enhancement could recursively populate hierarchical keys such as `0040|0260[0]|0040|0008`. Substantial (recursion, nested key syntax, ambiguous ordering semantics) — deferred unless a consumer asks.

Suggested sequencing

1. **ITKData PR first**: land a fixture with at least `OB`/`OD`/`OL` tags; confirm it resolves before writing tests against it.
2. **Then A + C2 as one `BUG:` PR**: parallel binary-VR tests for both modules, plus the `StudyDate`→`SeriesDate` comment fix.
3. **B standalone**: tiny two-site `BUG:` PR (DCMTKImageIO + GDCMImageIO).
4. **C standalone**: one-line documentation of the clear-and-replace contract.
5. **E and F**: open-ended; leave on this issue as future work.

Previously listed here and now **resolved**: the `DCMTKFileReader::GetDataset()` encapsulation regression from #6217. `GetDataset()` has been removed; the public surface is `PopulateMetaDataDictionary(MetaDataDictionary &) const` (`Modules/IO/DCMTK/src/itkDCMTKFileReader.h:510`), called from `Modules/IO/DCMTK/src/itkDCMTKImageIO.cxx:481` — exactly the shape that was asked for. No DCMTK types leak through ITK's public API for this path.

Contributor guide

Open the contributing guide

Research direction

Start by choosing one scoped item in the suggested sequencing, then read the referenced DCMTK and GDCM metadata code and tests. For binary-VR coverage, verify the ITKData fixture first, then inspect Modules/IO/DCMTK/test/CMakeLists.txt, itkDCMTKImageIOMetadataTest, and Modules/IO/GDCM/test/. Done means the selected behavior is covered or corrected in both modules, with relevant tests passing and the stated metadata contract preserved.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.