Bruker 2dseq reader: ParaVision on-disk format conformance

[gdevenyi]

Parse the JCAMP-DX forms ParaVision writes (PV5.1 headers, PV360 RLE and enum arrays, strings with commas), fix frame-scaling cardinality, and derive slice count and direction from frame groups. Adds synthetic PV360 GTests, no new external test data.

Defects fixed, per commit

• Parser: the fixed five-##/three-$$ header assumption went out of step on PV5.1 files, which write two $$ lines, so the reader consumed VisuVersion as a header line. It could not read ParaVision 360 files at all: @N*(value) run-length encoded arrays, $$ @vis= comments inside wrapped value blocks, commas inside <> strings such as <Parameter maps T2 relaxation, bg: Otsu.>, enum values stored as sized arrays (( 1 ) then disk_normal_slice_order), and scalar struct values on the parameter line.
• Scaling: VisuCoreDataSlope and VisuCoreDataOffs hold either one value for all frames or one value per frame. Per-frame indexing read out of bounds when the file stored a single value. Neither parameter can depend on a frame group, so the reader now rejects any other count instead of reusing element 0.
• Geometry: 2D datasets whose frame groups lack FG_SLICE, such as FG_ISA parameter maps, hold a single slice. Taking the slice count from identical per-frame positions gave a zero slice spacing. The slice axis now follows the sign of the slice-position step along the orientation's third row, which generalizes the earlier coronal-only Y-component heuristic to oblique stacks. A step below half the frame thickness lies inside one slice and no longer flips the axis.
• RLE bound: the file controls the repetition count, so the reader bounds the expansion before it appends anything.

Why the reader measures the slice axis instead of reading it from the orientation

The format notes linked in the first comment reconstruct VisuCoreOrientation from ACQ_grad_matrix and the ACQ_*_offset values as [ −d_r ; −d_p ; ±d_s ]. The third row is the right-handed completion of the first two, not the direction in which the slice offsets grow. It therefore carries either sign, and the slice order has to come from VisuCorePosition.

VisuCoreDiskSliceOrder cannot supply it either. ParaVision writes that parameter for 3D frames, not for the 2D multi-slice case this code path handles.

Test results

• The existing itkBruker2dseq_PV5.1_FSE_INT16 and PV6.0_FLASH_* regression tests pass against unchanged baselines.
• The Bruker2dseqImageIO GTests cover RLE arrays, wrapped strings with embedded commas, mid-value comments, broadcast scaling, frame-group reordering, a reversed slice axis, an out-of-range scaling count, an oversized and an over-long RLE count, and a slice step too small to carry a direction.
• The reader loads every dataset in a local ParaVision collection spanning PV5.1, PV6.0.1, PV7, and PV360 3.4-3.7, except zero-byte placeholder files, which it rejects with a clean exception.

AI assistance

• Tool: Claude Code
• Role: wrote the parser rewrite and the geometry fixes against the Bruker ParaVision file-format specification, which derives from Bruker's D01/D12 File Formats manuals and from ParaVision headers.
• I reviewed, built, and tested all code locally before committing.

[gdevenyi]

This work is built on https://github.com/gdevenyi/brkraw-legacy/blob/main/FILE_FORMAT.md which was constructed using an extensive AI deep dive into publicly available Bruker datasets, the Bruker Paravision manuals over multiple versions.

[dzenanz]

@greptileai review this.

[greptile-apps]

Greptile Summary

The Bruker reader now supports ParaVision 360 parameter forms, frame scaling, and frame-derived geometry. The previously reported resource-exhaustion path was disproved: the exact oversized run-length input is rejected with an ITK exception before expansion.

Confidence Score: 5/5

No blocking failure remains.

Focused public-reader tests passed for ParaVision 360 parsing, scaling, frame ordering, geometry, and the exact oversized run-length input.

T-Rex Logs

What T-Rex did

• An earlier run of the same harness recorded the precise Bruker JCAMP-DX RLE exception text and showed no bug evidenced by execution, occurring before the expansion loop.
• Rebuilt the focused Bruker reader test target and ran an authored public-reader test that writes @2147483647*(2) into VisuCoreDataSlope before calling ImageFileReader::Update() with Bruker2dseqImageIO; the test passed after catching the expected Bruker JCAMP-DX RLE ITK exception Bruker JCAMPDX RLE count out of range: @2147483647*(2).
• Ran the focused ParaVision 360 reader test, which passed while covering parsing, scaling, frame ordering, and geometry.

_{Ran code and verified through T-Rex}

_{Reviews (2): Last reviewed commit: "BUG: Bound Bruker JCAMP-DX run-length ex..." | Re-trigger Greptile}

[hjmjohnson]

Reviewed the six commits against the merge base. The parser work is solid — the RLE bound added in bc6224d3f3 is arithmetically sound (the count > remaining / (value.size() + 1) form avoids forming the product, runs before any append, and is cumulative across tokens in a record). Below are seven hardening items found while tracing file-controlled values through to the geometry and rescale paths, plus one configure-time blocker.

Please rebase on current upstream/main first. The branch is 58 commits behind and CI here last ran 2026-08-11. Since then, 969e685938c added a FATAL_ERROR to CreateGoogleTestDriver:

if(NOT "ITKGoogleTest" IN_LIST ITK_MODULE_${itk-module}-Test_DEPENDS)
  message(FATAL_ERROR "${itk-module} builds a GoogleTest driver: add ITKGoogleTest to its TEST_DEPENDS")
endif()

This PR adds creategoogletestdriver(ITKIOBruker …) without declaring the dependency, so it will fail to configure once rebased. The green checks above predate the guard and do not cover it. Fix in Modules/IO/Bruker/itk-module.cmake:

   TEST_DEPENDS
     ITKTestKernel
     ITKIOMeta
+    ITKGoogleTest

---

Hardening checklist

Every item below is driven by a value read from visu_pars, i.e. attacker-controllable input. Suggested patches are illustrative, not prescriptive.

• S1 — Array cardinality is never validated before indexing. (pre-existing — see note below, but worth fixing here)
• S2 — VisuFGOrderDesc tuple indexed at [1] without a size check
• S3 — StringToInt32 returns a signed value that is multiplied into a SizeType
• S4 — VisuCoreFrameCount is never reconciled with the allocated buffer
• S5 — Zero VisuCoreSize yields inf spacing and zero-length dimensions
• S6 — Slope/offset arrays of unexpected cardinality silently reuse element 0
• S7 — Slice-axis sign is decided from a single sampled position step

S1 — unvalidated array cardinality (pre-existing; please fix here)

GetParameter checks only that the key exists; ExposeMetaData succeeds for a vector of any length. VisuCoreOrientation is then consumed as nine doubles unconditionally:

const vnl_matrix<double> dirMatrix(&orient[0], 3, 3);

##$VisuCoreOrientation=( 1 ) 1 gives a one-element vector and reads 64 bytes past the allocation; ( 0 ) indexes an empty vector. The same applies to VisuCorePosition (needs ≥3), VisuCoreSize / VisuCoreExtent (indexed to [2] when VisuCoreDim==3), and [0] on VisuCoreFrameThickness.

To be clear: this is not introduced by this PR — dirMatrix(&orient[0], 3, 3) is byte-identical in the pre-PR file. I am raising it here anyway because this PR takes the readable-dataset count from 677 to 1631, which means roughly 950 files that previously threw during header parsing now reach this code. The PR does not create the bug, but it substantially widens the input surface that reaches it, so this is the right moment to close it. Happy for it to be a separate PR if you would rather keep this one scoped.

A checked accessor keeps it to one line per call site:

// Cardinality comes from the file; verify before indexing.
std::vector<double>
GetParameterArray(const MetaDataDictionary & dict, const std::string & name, const SizeType minimumSize)
{
  auto values = GetParameter<std::vector<double>>(dict, name);
  if (values.size() < minimumSize)
  {
    itkGenericExceptionMacro("Bruker parameter " << name << " has " << values.size() << " values, need "
                                                 << minimumSize);
  }
  return values;
}

Then GetParameterArray(dict, "VisuCoreOrientation", 9), …, "VisuCorePosition", 3), …, "VisuCoreSize", brukerDim), and likewise for VisuCoreExtent and VisuCoreFrameThickness.

S2 — VisuFGOrderDesc tuple indexed without a size check

Both loops index field [1]:

for (auto & i : GetParameter<std::vector<std::vector<std::string>>>(dict, "VisuFGOrderDesc"))
{
  const auto length = static_cast<SizeType>(StringToInt32(i[0], "Bruker 2dseq VisuFGOrderDesc size"));
  if (i[1] == "<FG_SLICE>")

##$VisuFGOrderDesc=( 1 ) (2) produces a single-field tuple, so i[1] is out of bounds. Reached from both ReadImageInformation and Read (the second loop at the sizeToSwap site has the same shape).

  if (i.size() < 2)
  {
    itkGenericExceptionMacro("Bruker 2dseq VisuFGOrderDesc tuple has " << i.size() << " fields, need at least 2");
  }

S3 — signed length multiplied into an unsigned accumulator

StringToInt32 is correctly ITK-throwing, but its result is signed and is multiplied into SizeType/size_t:

sizeToSwap *= itk::StringToInt32(i[0], "Bruker 2dseq VisuFGOrderDesc size");

( 1 ) (-4, <FG_ECHO>, ...) wraps sizeToSwap to ~1.8e19. It passes the sizeToSwap > 1 test and reaches

std::vector<T> tempBuffer(szSlice * sizeZ * sizeToSwap * sizeNoSwap);

which either throws a non-ITK std::bad_alloc/std::length_error out of Read, or — if the product wraps to something small — lets the copy loop walk fromPixel far past the end of buffer. The ReadImageInformation loop has the same issue via sizeT *= length.

  const auto rawLength = StringToInt32(i[0], "Bruker 2dseq VisuFGOrderDesc size");
  if (rawLength <= 0)
  {
    itkGenericExceptionMacro("Bruker 2dseq VisuFGOrderDesc size must be positive, got " << rawLength);
  }
  const auto length = static_cast<SizeType>(rawLength);

S4 — VisuCoreFrameCount not reconciled with the buffer

buffer is sized from GetImageSizeInComponents(), but Rescale's outer loop is driven by a value taken straight from the dictionary:

const SizeType frameCount = static_cast<SizeType>(GetParameter<double>(dict, "VisuCoreFrameCount"));
…
Rescale(static_cast<float *>(buffer), slopes, offsets, frameSize, frameCount);

Nothing checks frameCount * frameSize against the allocation, so a large VisuCoreFrameCount with a small VisuCoreSize writes past the buffer. A negative value is worse: static_cast<SizeType> of a negative double is undefined behavior.

  if (frameCount == 0 || frameSize == 0 || frameCount > numberOfComponents / frameSize ||
      frameCount * frameSize != numberOfComponents)
  {
    itkExceptionMacro("Bruker VisuCoreFrameCount " << frameCount << " is inconsistent with " << numberOfComponents
                                                   << " components in " << frameSize << "-voxel frames");
  }

Guarding the GetParameter<double> result for negativity/non-finiteness before the cast would also be worth doing.

S5 — zero extent produces inf spacing silently

this->SetDimensions(0, size[0]);
this->SetSpacing(0, FoV[0] / size[0]);

##$VisuCoreSize=( 2 ) 0 0 sets a zero-length dimension and an infinite spacing, and the image is handed back with no diagnostic. halfStep[0] = FoV[0] / (2 * size[0]) then propagates inf/NaN into the origin. Validate alongside the S1 cardinality check:

  for (SizeType i = 0; i < brukerDim; ++i)
  {
    if (!(size[i] >= 1.0) || !std::isfinite(FoV[i]))
    {
      itkGenericExceptionMacro("Bruker VisuCoreSize/VisuCoreExtent invalid on axis " << i);
    }
  }

(The !(x >= 1.0) form rejects NaN as well.)

S6 — unexpected slope cardinality silently reuses element 0

const double slope = (f < static_cast<SizeType>(slopes.size())) ? slopes[f] : slopes.front();

This correctly fixes the out-of-bounds read, and the broadcast case is exactly right per the spec: "may hold one value for all frames or one per frame." But it accepts every other cardinality too — 3 slopes across 12 frames silently applies slope 0 to the last 9 frames, producing wrong pixel values with no error. Since the format admits only two cardinalities, the third case is a corrupt file:

  if (slopes.size() != 1 && slopes.size() != frameCount)
  {
    itkExceptionMacro("Bruker VisuCoreDataSlope has " << slopes.size() << " values, expected 1 or " << frameCount);
  }

…and the same for offsets, after which the ternary can become slopes[slopes.size() == 1 ? 0 : f], which states the rule directly.

S7 — slice-axis sign from a single sampled step

This is the item I would most like your view on, because it governs the 67 changed outputs the PR reports.

const SizeType positionStride = (positionCount > sizeZ) ? framesPerSlice : 1;
if (3 * (positionStride + 1) <= static_cast<SizeType>(position.size()))
{
  const vnl_vector<double> slice1(&position[0], 3);
  const vnl_vector<double> slice2(&position[3 * positionStride], 3);
  sliceDiff = slice2 - slice1;
  spacingZ = sliceDiff.magnitude();
}
…
const double reverseZ = (dot_product(sliceDiff, dirMatrix.get_row(2)) < 0) ? -1 : 1;

The stride is selected solely by positionCount > sizeZ. When positionCount == sizeZ but a frame group varying faster than FG_SLICE is present, the stride is 1 and position[0] / position[3] can be two frames of the same slice. sliceDiff is then floating-point noise, its sign is arbitrary, and the Z axis flips on a dataset that was previously correct — while spacingZ becomes near-zero and gets silently replaced by VisuCoreFrameThickness a few lines later, erasing the evidence that the measurement was bad.

That near-zero magnitude is a usable reliability signal: a genuine slice step is on the order of the frame thickness, so require it before trusting the sign, and fall back to the un-flipped axis otherwise.

  // A step much smaller than the frame thickness means the two sampled
  // positions are within one slice, so its sign carries no orientation.
  const double frameThickness = GetParameterArray(dict, "VisuCoreFrameThickness", 1)[0];
  if (spacingZ < 0.5 * frameThickness)
  {
    sliceDiff.fill(0.0);
    spacingZ = 0;
  }

With sliceDiff zeroed the dot product is 0, the < 0 test is false, and reverseZ stays +1 — the pre-PR behavior — so this narrows the flip to cases where a real inter-slice step was measured.

More generally: could you say how the 67 changed datasets were confirmed? The PR notes they were "verified against the stored VisuCorePosition progression", but since that progression is the same data the new rule derives the sign from, it would agree with the rule by construction. An independent check — a known-orientation phantom, or agreement with brkraw/bruker2nifti on the same files — would separate a genuine correction from a new regression. Some of those 67 may well have been correct before.

Smaller notes (non-blocking)

• sizeZ = dict.HasKey("VisuFGOrderDesc") ? 1 : positionCount; replaces a itkGenericExceptionMacro("Could not find order description field"). Inferring a slice count where the reader used to refuse is presumably deliberate given the readability goal, but it is worth a one-line comment saying so, since it is the one place the change trades an error for a guess.
• The ( N ) dimension indicator is parsed and then discarded without being checked against the element count. ParseDoubles stops at the first non-numeric token, so ( 2 ) 0 spatial silently yields one element rather than an error.
• The >9-digit RLE guard is not exercised: the test's @999999999*(2) is exactly nine digits and trips the byte cap instead. A ten-digit count would cover the other branch.
• MaskStrings is recomputed in ExpandRLE, SplitTuples, SplitFields, and inline in ParseJCAMPDXRecord — four O(n) passes over the same record that could be built once.
• The GTest writes to testing::TempDir(); other ITK IO GTests use TOSTRING(ITK_TEST_OUTPUT_DIR) (see itkJPEGImageIOGTest.cxx, itkHDF5ImageIOGTest.cxx), which CTest cleans up.
• A few comment blocks run to 5 and 8 lines; ITK's guidance is one short WHY-only line where a comment is needed at all.

Thanks for this — the format work is the valuable part, and 677 → 1631 readable datasets is a big improvement for Bruker users. The items above are about what happens on malformed input, which matters more than usual here because these are files the reader accepts from wherever the user got them.

[hjmjohnson]

Filed S1 (and the related S2-S5 input-validation gaps, all of which predate this PR) as #6813 against ITKIOBruker, so none of them need to block this branch. The rebase and the ITKGoogleTest TEST_DEPENDS line are still needed here, and S6/S7 remain specific to this changeset.

[hjmjohnson]

Please see the checklist of requested changes.

[gdevenyi]

Rebased on current main (was 58 commits behind) and force-pushed. The configure blocker and both
remaining items are done. Nothing from S1–S5 is in this branch.

• Configure blocker — ITKGoogleTest added to TEST_DEPENDS (COMP: commit). The
  FATAL_ERROR postdates the last CI run here, so the green checks above do not cover it.
• S1–S5 — agreed on all five. Thanks for filing Bruker 2dseq reader does not validate file-controlled sizes before indexing and allocating #6813.
• S6 — fixed as you wrote it.
• S7 — guard added in the form you proposed. More on the sign rule below.

S6 — why the strict check is safe

VisuCoreDataSlope and VisuCoreDataOffs cannot depend on a frame group. PV5.1 D13 lists ten
parameters that can; PV6 D02, PV7 and PV360 1.0–3.7 extend the list to fourteen. Neither slope nor
offset is in either list.

A parameter that does not depend on a frame group carries no value, one value, or
VisuCoreFrameCount values. Any other count is therefore a corrupt file rather than a broadcast,
so the check rejects only corrupt files.

The check sits in Read(), where itkExceptionMacro has member context. The ternary now reads
slopes[slopes.size() == 1 ? 0 : f]. A new GTest covers a 3-slope, 12-frame file.

S7 — the guard, and why the sign is measured at all

The guard is yours, with one change. The reader takes the thickness through the existing
array-or-scalar accessor, which returns 0 when the parameter is absent. A file that has a good
measured step but no VisuCoreFrameThickness therefore still reads, instead of throwing where it
did not throw before.

On your wider point: you are right that the position progression cannot confirm a rule derived from
the position progression. The reason the reader derives the sign at all is in the format itself.
The format notes linked in the first comment on this PR reconstruct VisuCoreOrientation from
ACQ_grad_matrix and the ACQ_*_offset values as [ −d_r ; −d_p ; ±d_s ]. The third row is the
right-handed completion of the first two. It is not the direction in which the slice offsets grow,
so it carries either sign, and a reader that treats it as the slice order is wrong on the files
where it does not. VisuCoreDiskSliceOrder cannot fill the gap either: ParaVision writes it for
3-D frames, not for the 2-D multi-slice case this code path handles. That leaves VisuCorePosition
as the only source, which is what the branch measures.

Your guard narrows that measurement to steps large enough to mean something, which is the right
constraint, and I have taken it as written.

I also checked the affected datasets against a separate reader. Sixty-seven datasets in a local
ParaVision collection come out of this branch with a different TransformMatrix than at the merge
base, and nothing else about their output changes. pvraw reads those files through brukerapi and
applies a subject-position correction, so its affine sits in a rotated frame. Comparing its slice
column against ITK's direction column therefore compares two different spaces, which is the trap I
fell into first. Estimating that rotation from the two in-plane axes and then testing the slice
axis under it gives:

datasets whose direction changed	67
slice axis agrees with pvraw	46
slice axis opposite	0
outside a study pvraw can load	16
pvraw returns several images, for a slice package or an echo train	5

Please weigh that for what it is. The code is independent of ITK's, but it shares lineage with the
format notes this branch was written against, so it could agree by having learned the same rule. It
is firmer evidence than the position progression you flagged, and softer than agreement with
brkraw or bruker2nifti would be.

Smaller notes

Done:

• GTest output moves from testing::TempDir() to TOSTRING(ITK_TEST_OUTPUT_DIR), wired through
  target_compile_definitions as itkJPEGImageIOGTest.cxx does it.
• A ten-digit RLE count now exercises the >9-digit branch, next to the byte cap.
• The comment blocks this PR adds are one line each. The longer blocks left in the file are older.
• The sizeZ inference already carried its one-line reason, so it is unchanged.

Not done:

• ( N ) count enforcement. New strictness would reject files that read correctly today. It is
  the same class of check as S1–S5, so it belongs with Bruker 2dseq reader does not validate file-controlled sizes before indexing and allocating #6813.
• MaskStrings single pass. It changes four functions to save one pass over a parameter
  record. That is not worth the diff here.
• Bracketed scalar enums (##$VisuCoreByteOrder=<littleEndian>). PV6 and later document this
  form, but ParaVision writes the bare token in practice. The reader handles the bare token and the
  ( 1 ) sized enum-array form that PV360 writes. I record this as a documented but unobserved
  gap, and added no branch that nothing exercises.

One note on the test fixture, because the file looks self-contradictory. It declares
disk_normal_slice_order and its slice positions run against the third row. That combination is
deliberate, and it is what real reversed 2-D data looks like: per the format relation above, the
third row carries either sign, and disk_normal_slice_order says nothing about which.
disk_reverse_slice_order would instead make the reader reverse the pixel data, which is a
different code path.