Repository navigation
Add support for reading/writing VTK XML ImageData (.vti) format - #6032
Conversation
github-actions
Bot
added
type:Infrastructure
|
It would be good to use an XML parsing library like expat which is already in ITK. |
github-actions
Bot
added
area:Python wrapping
|
Force-pushed CI failures fixed
Review feedback addressed
The XML header parsing is now done by expat (the same library
Switching to expat removes all the ad-hoc Tests addedThe previous commit shipped no VTI tests at all. This commit adds 1. Round-trip via
2. Behavior tests:
3. Hand-crafted-file readability tests for code paths the writer never produces but the reader must support (cross-checked against VTK's
This matches the relevant subset of VTK's Local resultsTest output (every assertion passes):
Out-of-scope items (potential follow-ups)
|
|
We should add a few test files converted into .vti format by ParaView, and regression test them against the existing .nrrd/.mha versions. And of course, manually review this. |
|
Legacy removed tests failed: Modules/IO/VTK/test/itkVTIImageIOTest.cxx:100:56: warning: 'itk::ImageConstIterator::IndexType itk::ImageConstIterator::GetIndex() const [with TImage = itk::Image<unsigned char, 1>; itk::ImageConstIterator::IndexType = itk::Index<1>]' is deprecated: Please use |
|
@greptileai review this. |
|
| Filename | Overview |
|---|---|
| Modules/IO/VTK/src/itkVTIImageIO.cxx | ~1400-line new ImageIO implementation; expat-based XML parsing, all major encoding paths (ASCII, base64, appended raw/base64, zlib), byte-swap fix, tensor remap, direction cosines — two P2 notes around ZLib header endianness and a misleading comment in the writer |
| Modules/IO/VTK/include/itkVTIImageIO.h | Clean header; public SwapBufferForByteOrder, private encode/decode helpers, DataEncoding enum, and well-documented deferred-feature list |
| Modules/IO/VTK/test/itkVTIImageIOTest.cxx | Comprehensive round-trip test covering scalar, RGB, RGBA, vector, tensor, ASCII, binary, appended, and compressed paths with real pixel comparisons |
| Modules/IO/VTK/test/itkVTIImageIOSwapBufferTest.cxx | 17-case unit test for SwapBufferForByteOrder covering all component sizes and endianness combinations without needing a BE runner |
| Modules/IO/VTK/test/itkVTIImageIODirectionTest.cxx | Round-trip test for oblique direction cosines against a 45° Z-rotation fixture with per-element tolerance checking |
| Modules/IO/VTK/test/itkVTIImageIOGeneratedFixturesTest.cxx | Cross-implementation validation: reads Python-stdlib-generated fixtures (scalar, RGBA, vector, tensor, zlib) and pixel-compares to MetaIO oracles |
| Modules/IO/VTK/test/itkVTIImageIOFutureFeaturesTest.cxx | Guard tests for F-001/F-002/F-005/F-010 deferred features; each asserts the tagged exception fires on purpose-built fixture files |
| Modules/IO/VTK/test/generate_vti_fixtures.py | Pure Python-stdlib fixture generator producing sub-KB .vti + MetaIO oracle pairs for 6 encoding/pixel-type combinations including guard-only fixtures |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ReadImageInformation] --> B{Security pre-scan\nDOCTYPE/ENTITY?}
B -- reject --> ERR1[itkExceptionMacro]
B -- ok --> C[Expat chunk parser]
C --> D{AppendedData\nencountered?}
D -- yes --> E[XML_StopParser\nSuspended]
D -- no,EOF --> F[Parse complete]
E --> G[Seek to _ marker\nrecord m_AppendedDataOffset]
G --> H{encoding=base64?}
H -- yes --> I[Read m_AppendedBase64Content]
H -- no --> J[RawAppended offset stored]
F --> K[Populate m_DataEncoding\ngeometry/pixel-type fields]
subgraph Read
K --> L{m_DataEncoding}
L -- ASCII --> M[ReadBufferAsASCII]
L -- Base64/ZLibBase64 --> N[DecodeBase64 + memcpy\nor DecompressZLib]
L -- Base64Appended/ZLibBase64Appended --> O[DecodeBase64 m_AppendedBase64Content\nextract at m_DataArrayOffset]
L -- ZLibAppended --> P[Seek in file stream header\nstream payload DecompressZLib]
L -- RawAppended --> Q[Seek in file read directly]
M & N & O & P & Q --> R[SwapBufferIfNeeded\npixel byte-order]
R --> S{Tensor pixel type?}
S -- yes --> T[TensorRemapGuard\nVTK XX,YY,ZZ,XY,YZ,XZ to ITK e00..e22]
S -- no --> U[return]
T --> U
end
subgraph Write
V[Write] --> W{FileType}
W -- ASCII --> X[WriteBufferAsASCII\ntensor remap VTK canonical]
W -- Binary+UseCompression --> Y[CompressZLibVTK\nAppendedData encoding=raw]
W -- Binary inline --> Z[UInt64 header + EncodeBase64]
end
Reviews (4): Last reviewed commit: "ENH: Exercise compression for multiple p..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
dzenanz
left a comment
dzenanz
left a comment
There was a problem hiding this comment.
Having this work correctly would be great!
|
Thank you for pushing this forward Hans! |
2e4b57c to
d290d3b
Compare
|
@dzenanz Thank you for your response, and for allowing me to "see what happens" as I try to build out improved tools for using agentic-coding as an accelerator for developers. I do NOT want to become a VTK vti file format expert. Still, I am willing to apply the "research first," "test second," "code last" pattern so that you can hopefully get this to a point where you have very little effort to apply your expert knowledge and use-case knowledge to take it to the last mile. Your comments and suggestions are GREATLY appreciated. |
|
@greptileai review this draft before I make it official |
Let me know when you have finished with the comparisons, and I'll do a final code review. |
|
@greptile re-review |
|
@hjmjohnson This is ready for another pass by you. 700 images in .vti format were read without crashing, and some similarity metric (MMI) produced reasonable, non-zero results. |
dzenanz
left a comment
dzenanz
left a comment
There was a problem hiding this comment.
Only one remark / question.
|
@dzenanz Final triage checklist before this PR ships — items I think are still open, plus housekeeping suggestions. Provenance — most of the recent commitsThe 18 fixup commits I pushed today (everything from This is disclosed up-front so reviewers can give the AI-generated portion the closer scrutiny it deserves. Deferred tasks (not in this PR; tracked for follow-up)
Suggestion: squash to a few logical commits before merge35 commits is a lot for review. The natural groupings: | Bucket | Suggested message | Includes | Two commits, one logical topic each, each independently revertable. Happy to do the squash on a separate branch if you want to review the squashed shape before force-pushing. Housekeeping: convert in-tree data files to ExternalData CIDs
These should migrate to ExternalData
Same time, ITK has historically used |
|
Force-pushed |
hjmjohnson
left a comment
hjmjohnson
left a comment
There was a problem hiding this comment.
@dzenanz I'll let you take this over the finish line. I am happy with the state of this code. It addresses all the issues identified.
I do not plan to revisit this PR unless you explicitly ask me to. Good Luck!
|
I removed the following paragraph from the commit message: as it is unrelated to this PR. I rebased on top of current main. Good to go from me. |
end-of-file-fixer pre-commit hook flagged a trailing empty line. One-character fix to unblock CI on PR InsightSoftwareConsortium#6032.
|
Pushed |
github-actions
Bot
added
the
area:Documentation
ImageIO module for reading and writing VTK XML ImageData (.vti) files.
expat-backed XML header parser; supports inline ASCII, inline base64,
appended raw, appended base64, and zlib-compressed appended encodings.
Handles scalar / Vector(3) / RGB(3) / RGBA(4) / symmetric-tensor(6)
pixel types; the symmetric-tensor on-disk layout is VTK-canonical
[XX,YY,ZZ,XY,YZ,XZ] and is remapped to ITK's [e00,e01,e02,e11,e12,e22]
(upper-triangular row-major) on read.
Writer emits version="1.0" / header_type="UInt64" / 3x3 row-major
Direction attribute; appended-raw writes pair with vtkZLibDataCompressor
when SetUseCompression(true).
Hardening:
* <!DOCTYPE> / <!ENTITY> declarations are rejected up-front to
mitigate billion-laughs and XXE attacks.
* <DataArray> consumption is scoped to <PointData> children only;
arrays inside <CellData>, <FieldData> etc. are not consumed
(F-011 explicit guard).
* Direction attribute parser rejects fewer-than-9 floats and
trailing non-whitespace junk; warns when the matrix is not
orthonormal (D^T * D != I) since ITK geometry pipelines assume
orthonormality.
* TensorRemapGuard remaps only on the successful Read() exit so a
throw mid-decode does not scramble the caller's buffer.
Tests cover the round-trip for all encodings and pixel types, the
F-NNN deferred-feature guards (LZ4/LZMA decompression, multi-Piece,
binary symmetric-tensor write, unknown-compressor catch-all,
CellData-only), malformed-input rejection (truncated AppendedData,
non-numeric NumberOfComponents, Direction trailing junk,
non-orthonormal Direction, DOCTYPE/ENTITY rejection), and
pixel-equality round-trip against MetaIO oracles produced by an
independent Python-stdlib generator. Four no-arg legacy CTests are
delivered as GoogleTest blocks via a new ITKIOVTKGTests driver.
Original scaffold + zlib/base64/appended-data + vtiSupport branch:
@dzenanz (with Sonnet 4.6 draft assistance). Correctness review,
test coverage extension, F-NNN guard system, and migration-guide
documentation: @hjmjohnson (with Claude Opus 4.7 1M context xhigh
review).
Co-Authored-By: Hans J. Johnson <hans-johnson@uiowa.edu>
|
Squashed to a single commit ( |
end-of-file-fixer pre-commit hook flags a trailing empty line on this upstream-tracked file. Same one-character fix as PR InsightSoftwareConsortium#6032; included here so PR InsightSoftwareConsortium#4221's pre-commit CI can pass without waiting on that PR to merge first.
Same upstream-inherited end-of-file-fixer hit blocking PR InsightSoftwareConsortium#6032 / InsightSoftwareConsortium#4221. One-character fix included here so this PR's pre-commit CI can pass.
`Documentation/docs/releases/5.4.6.md` carries a trailing empty line that the `end-of-file-fixer` pre-commit hook flags every time a PR rebases onto current `main`. Affected at least PRs InsightSoftwareConsortium#6032, InsightSoftwareConsortium#4221, and InsightSoftwareConsortium#6186, where each had to carry an identical one-character fix. Apply once on `main` to stop the spurious `pre-commit` failures.
65a487e
into
InsightSoftwareConsortium:main
…port Add support for reading/writing VTK XML ImageData (.vti) format
…port Add support for reading/writing VTK XML ImageData (.vti) format
VTK XML ImageData (
.vti) reader + writer for ITK. Original scaffold from @dzenanz (Sonnet 4.6 draft); correctness + guard-test pass on top by @hjmjohnson. Every @blowekamp and @greptileai P1/P2 concern is addressed or explicitly deferred to a follow-up PR with a tagged guard in the source.Status — 13 new commits added today; CI running. Draft; ready for re-review once CI turns green.
One known test is disabled:
itkVTIImageIOReadWriteTestVHFColorZLib— the ParaView-produced fixture cannot be published while the ExternalData upload tool is down (see below). Equivalent code-path coverage is provided by a syntheticVector<float,3>ZLib-appended fixture, so nothing ships with red CI.Reviewer comments addressed (with commit pointers)
ddf579b15f). Parser is nowexpatwith<!DOCTYPE>/<!ENTITY>rejection added on top.ddf579b15f, hardened in96352cdcf1content-link-upload.itk.orgis down (ITK#4340, storacha→Pinata migration), so 294 KB+ ParaView fixtures cannot be published right now. Replaced with a Python-stdlib fixture generator producing sub-KB synthetic.vti/.mhdpairs covering every encoding combination;itkVTIImageIOGeneratedFixturesTestreads each and pixel-compares to its MetaIO oracle. When upload returns, real ParaView fixtures will be added alongside — both serve as useful cross-impl validation.bda507863f,bd2a11e6db,944bcbdcc6GetIndex()deprecation warning initkVTIImageIOTest.cxx:100b884beed42before this re-review pass.b884beed42itkVTIImageIO.cxx:777]SetNumberOfComponents(6), so only 6 of the 9 stream values are consumed per pixel → every subsequent pixel's components shift.[XX, YY, ZZ, XY, YZ, XZ]. Reader rejectsNumberOfComponents!="6"for tensor arrays and remaps to ITK's[e00, e01, e02, e11, e12, e22]via aTensorRemapGuardscope guard after every encoding path. Writer emits 6 components in canonical order and the test's promised pixel-wise comparison is now real.45b8815de7itkVTIImageIO.cxx:749]SwapRangeFromSystemToBigEndianon a BE host does nothing, leaving LE file bytes un-swapped.SwapBufferIfNeededreplaced with a public staticSwapBufferForByteOrder(buffer, componentSize, numComponents, fileByteOrder, targetByteOrder)that byte-reverses each component viastd::reverseunconditionally when the orders differ. 17 unit-test cases covercomponentSize ∈ {1,2,4,8} × (file × target) ∈ {LE,BE}²on any host — no BE CI runner needed to exercise the fix.24e030c4ecitkVTIImageIO.cxx:245]encodingattribute of<AppendedData>was not captured.ea5add7c78(base64 appended support added); reader now capturesencoding="base64"vsencoding="raw"explicitly and dispatches accordingly.ea5add7c78itkVTIImageIO.cxx:513]contentbuffer is a local inInternalReadImageInformation(bounded peak; goes out of scope beforeRead()). Full streaming read requires inheriting fromStreamingImageIOBase, which is a separate, larger change scoped to the follow-up PR. A code comment at the class docstring now documents the Phase-2 plan for streaming-read via the appended-raw byte offset.itkVTIImageIO.cxx:1013]uint32_tblock-size header silently truncates for images >~4 GB"header_type="UInt64"+ auint64_tblock-size prefix unconditionally.version="1.0"pairs with this to match ParaView 5.7+ defaults. No overflow check needed because the header integer is wide enough for all platformSizeTypevalues.b2590b1db5Additional defects discovered during the audit that did not originate from a reviewer comment but were fixed in this pass:
Directioncosines loss (both read and write). Every ParaView-produced.vticarriesDirection="..."but the reader ignored it and the writer emitted none; every round-trip through this IO silently reset orientation to identity. Unacceptable for ITK's medical-imaging use case. Fixed infb3815f64cwith a hand-crafted oblique-rotation fixture + round-trip test.ITK_TRY_EXPECT_NO_EXCEPTION. Replaced with realImagesEqualassertion in45b8815de7.5257040c63.<!DOCTYPE>/<!ENTITY>attacks (billion-laughs, XXE). Added a pre-parse rejection andXML_SetParamEntityParsing(NEVER)in96352cdcf1.Commit list (13 new on top of @dzenanz's vtiSupport tip)
3698b6196cbda507863ffb3815f64c45b8815de724e030c4ecb2590b1db55257040c63e5879bea7596352cdcf1c24cfea37c8c61028f09bd2a11e6db944bcbdcc6Every commit compiled and its associated test(s) ran locally before the next was staged. Diff footprint: +1662 / −87 across 30 files, all scoped to
Modules/IO/VTK/.Test status: 9 of 9 VTI tests pass locally
The
itkVTIImageIOReadWriteTestVHFColorZLibregistration (which has been red sincec283dfd86dbecause its ParaView-generated input fixture was never committed) is now disabled with a TODO inModules/IO/VTK/test/CMakeLists.txtpointing at ITK#4340 and cmake-w3-externaldata-upload#3. When the upload tool is restored, un-comment the block and publishInput/VHFColorZLib.vti.cidalongside it.Equivalent code-path coverage (3-component Float32 ZLib-compressed appended-raw with UInt64 header, dispatched to
IOPixelEnum::VECTOR) is provided by the newvector3_f32_zlib_appendedcase initkVTIImageIOGeneratedFixturesTest— driven by a 4×4×2Vector<float,3>fixture produced by the Python-stdlib generator (no ITK code participates in data generation, giving genuine cross-impl validation).Scope boundary: features deferred to the follow-up PR (F-NNN guards)
Every deferred feature has a tagged guard exception in the source so
git grep 'F-NNN'locates the guard, its test, and the commit message explaining why. When the follow-up PR lands each feature, it flips the guard test from "expect exception" to "expect success + pixel-compare" in the same commit that removes the guard — a visible red → green transition in git history.itkExceptionMacrooncompressor="vtkLZ4DataCompressor"VTI_guard_lz4.vtivtkLZMADataCompressorVTI_guard_lzma.vtiImageIOBase, notStreamingImageIOBase<Piece>readitkExceptionMacrowhenpieceCount > 1VTI_guard_multipiece.vtiitkExceptionMacroinWrite()itkExceptionMacrofor any non-{zlib,lz4,lzma} compressorVTI_guard_unknown_compressor.vtiNo ZSTD guard is needed — VTK has never shipped a
vtkZSTDDataCompressor. F-006 (UInt32 overflow guard on write) was subsumed by emitting UInt64 unconditionally.Synthetic fixture generator
Modules/IO/VTK/test/generate_vti_fixtures.pyis ~200 lines of Python stdlib (xml,struct,base64,zlib) that produces deterministic sub-KB.vtifixtures plus matching MetaIO.mhd/.raworacles. Runs idempotently:VTI_oblique_direction.{vti,mhd,raw}uint8scalar with 45° Z-rotationDirectionitkVTIImageIODirectionTestVTI_scalar_u8_appended_raw.{vti,mhd,raw}uint8scalaritkVTIImageIOGeneratedFixturesTestVTI_scalar_f32_zlib_appended.{vti,mhd,raw}itkVTIImageIOGeneratedFixturesTestVTI_rgba_u8_appended_raw.{vti,mhd,raw}RGBA<uint8>itkVTIImageIOGeneratedFixturesTestVTI_vector3_f32_zlib_appended.{vti,mhd,raw}Vector<float,3>itkVTIImageIOGeneratedFixturesTest(replaces the disabled VHFColorZLib coverage)VTI_tensor_f32_ascii.{vti,mhd,raw}SymmetricSecondRankTensor<float,3>(VTK canonical[XX,YY,ZZ,XY,YZ,XZ]on disk)itkVTIImageIOGeneratedFixturesTestVTI_guard_{lz4,lzma,unknown_compressor,multipiece}.vtiitkVTIImageIOFutureFeaturesTestRationale for shipping the generator alongside the fixtures: we need sub-100 KB fixtures to pass the
kw-pre-commithook cap; ParaView-produced fixtures exceed that and ExternalData upload is unavailable today; and passing a reader test against a Python-stdlib writer's output is genuinely independent cross-implementation validation that the other round-trip tests do not provide.Implementation highlights worth a targeted review
Directioncosines (fb3815f64c) — expat parser adds adirectionstate field, reader builds a 3×3 row-major matrix and callsSetDirection(axis, v)per image axis, writer composes an identity-padded 3×3 fromGetDirection(axis). ColumnjofDirectionis axis-j's world-space direction, matching both VTK's convention and ITK'sm_Direction[axis][world]storage.Symmetric tensor canonicalization (
45b8815de7) — reader uses a scope guard (TensorRemapGuard) that remaps[XX, YY, ZZ, XY, YZ, XZ]→[e00, e01, e02, e11, e12, e22]in place regardless of which encoding path populated the buffer. One remap site, five exit points, no ifs.Endian fix (
24e030c4ec) —std::reverse-based swap makes the code work on any host and lets the unit test generate both "file=LE, target=BE" and "file=BE, target=LE" combinations from either endianness.Security (
96352cdcf1) — pre-parse scan for<!DOCTYPE/<!ENTITYtokens (billion-laughs + XXE) +XML_SetParamEntityParsing(NEVER)— defence-in-depth with a test that round-trips a malicious payload and asserts refusal.F-NNN discoverability (
e5879bea75) —git grep 'F-001'(etc.) from any clone lands on the guard, the test, and the commit message. Designed so the follow-up PR authors can pick up work items without reading this PR description.