ENH: Add Image Reader/Writer that depend on Tiff and Stb libraries.

[imikejackson]

Depends on BlueQuartzSoftware/simplnx-registry#30

Introduces three new SimplnxCore image I/O filters (ReadImageFilter, WriteImageFilter, ReadImageStackFilter) backed by a new format-agnostic IImageIO
abstraction layer, fixes a long-standing multi-component conversion bug in the legacy ITK reader, and consolidates the test data archives for both filter
families into two versioned _v3 archives.

New SimplnxCore filters

• ReadImageFilter, WriteImageFilter, ReadImageStackFilter — full feature parity with their ITKImageReaderFilter / ITKImageWriterFilter /
  ITKImportImageStackFilter counterparts (origin/spacing overrides with pre/post-processed timing, voxel and physical cropping, data type conversion, XY/XZ/YZ
  plane slicing on write, flip transforms, grayscale conversion, resampling).

New IImageIO abstraction layer (simplnx/Utilities/ImageIO/)

• Strategy pattern with StbImageIO (PNG/JPEG/BMP via stb) and TiffImageIO (TIFF via libtiff) backends selected by a CreateImageIO() extension-based factory.
• Out-of-core safe: data is copied tuple-by-tuple into AbstractDataStore via operator[] — no raw pointers to DataArray internals. Write temp buffers are
  never larger than a single 2D slice.
• Error messages from the underlying libraries are captured and returned through Result. libtiff errors are captured via a thread-local handler.

Upstream ITK reader bug fix

• ITKImageProcessing/Common/ReadImageUtils.hpp — ConvertImageToDataStoreAsType() was iterating only pixelContainer->Size() elements, which is the pixel
  count, not the scalar element count. For multi-component images (e.g. RGB itk::Vector<uint8, 3>) this left two-thirds of the destination uninitialized. Fixed
  by multiplying by itk::NumericTraits::GetLength().

[JDuffeyBQ]

The filter documentation needs to be added.

[imikejackson]

@JDuffeyBQ thanks for the review. Here's what changed:

IImageIO layer — readPixelData/writePixelData now take std::span<uint8> / std::span<const uint8> (StbImageIO + TiffImageIO updated to match). Destructor was already noexcept in the latest push.

All three new filter headers — removed FromSIMPLJson declarations and the stray /* LEGACY UUID ... */ comment, since these are new filters not ported from SIMPL.

All three filter .cpp files — removed the now-empty namespace SIMPL {} block and the FromSIMPLJson stub function bodies.

Algorithm constructors — signatures changed from *InputValues to const InputValues& (ReadImage, WriteImage, ReadImageStack); internal m_InputValues-> became m_InputValues..

ReadImageFilter.cpp:

• Renamed backendResult/backend → imageIOResult/imageIO.
• Replaced the FilterHandle/FilterList lookup for CropImageGeometryFilter with a direct header include and CropImageGeometryFilter::k_*_Key constants.
• Extracted the applyOriginSpacingOverrides lambda into a free ApplyOriginSpacingOverrides function in the anonymous namespace.

WriteImageFilter.cpp:

• GetScalarPixelAllowedTypes() function replaced with a k_ScalarPixelAllowedTypes constant.
• Preflight's stringstream replaced with an fmt::runtime formatted string.
• Removed <sstream>, <iomanip>, <StringUtilities>, <SIMPLConversion> includes.

WriteImage.cpp:

• Fixed the reinterpret_cast<T*>(buffer.data()) aliasing hazard — added WriteElementAs<T> / ReadElementAs<T> helpers that go through std::memcpy. Same fix applied to ReadImage.cpp's Copy/Convert functors.
• Hoisted the per-slice ImageMetadata construction out of the loop (all fields are slice-invariant).
• Added a CreateIndexString<T> helper using fmt::runtime and removed the while-loop manual zero-padding.

ReadImageStack.cpp:

• FlipAboutXAxis / FlipAboutYAxis now take const Vec3<usize>&.
• Inverted the scalingFactor == 100.0f / else branch to scalingFactor != 100.0f with a comment.
• Collapsed the scoped IdType fetch into a single line.

ReadImageStackFilter.cpp — the two dynamic_cast<const Create*Action*> now return a preflight error (-23530, -23531) if the cast fails instead of silently skipping.

Tests:

• ReadImageStackTest.cpp — the k_* local mutable variables renamed to camelCase (fileListInfo, origin, spacing).
• WriteImageTest.cpp — added a ScopedTempDir RAII class so the temp directory is cleaned up even if a REQUIRE() fails mid-test. validateOutputFiles no longer does the cleanup.

SimplnxCore/CMakeLists.txt — dropped the commented-out txm_reader block (not related to this PR).

Documentation — wrote the three docs pages (ReadImageFilter.md, WriteImageFilter.md, ReadImageStackFilter.md), mirroring the ITK filter doc structure with adjustments for stb/libtiff.

Deferred / pushed back on two items — replies on the inline threads:

• Scoped enums for the ChoicesParameter indexes (originSpacingProcessing, imageTransformChoice, resampleImagesChoice).
• Switching origin/spacing parameters from VectorFloat64Parameter to VectorFloat32Parameter.

All 42 image-IO unit tests pass locally on the Rel build. Happy to revisit the two deferred items if you'd rather have them in this PR.

[imikejackson]

Did the scoped-enum cleanup and consolidate across both plugins. Pushed the following:

New shared header — simplnx/Utilities/ImageIO/ImageIOEnums.hpp declares two scoped enums that both SimplnxCore and ITKImageProcessing now use:

enum class OriginSpacingProcessing : uint64 { Preprocessed = 0, Postprocessed = 1 };
enum class ImageFlipTransform     : uint64 { None = 0, FlipAboutXAxis = 1, FlipAboutYAxis = 2 };

The underlying type is uint64 to match ChoicesParameter::ValueType exactly (static_cast/to_underlying round-trips through the parameter system without a signed/unsigned-long mismatch).

SimplnxCore filters

• ReadImageInputValues::originSpacingProcessing → OriginSpacingProcessing
• ReadImageStackInputValues::originSpacingProcessing → OriginSpacingProcessing
• ReadImageStackInputValues::imageTransformChoice → imageTransform : ImageFlipTransform
• ReadImageStackInputValues::imageDataTypeChoice (usize) → imageDataType : DataType (converted once at the filter boundary via ConvertChoiceToDataType)
• Dropped the per-file k_NoImageTransform / k_FlipAboutXAxis / k_FlipAboutYAxis constants (both the filter and the algorithm file) — the enum replaces them.
• All == 0 / == 1 comparisons in both filters and algorithms now use the named enumerators.

ITKImageProcessing — migrated to the same shared enum so there's only one vocabulary across both plugins:

• Removed cxItkImageReaderFilter::OriginSpacingProcessingTiming from ReadImageUtils.hpp.
• ITKImageReaderFilter, ITKImportImageStackFilter, and ReadImageUtils.cpp all now take and compare OriginSpacingProcessing directly.
• ITKImportImageStackFilter's template ReadImageStack<T> takes ImageFlipTransform transformType instead of a ChoicesParameter::ValueType, and its file-scope k_NoImageTransform / k_FlipAboutXAxis / k_FlipAboutYAxis constants are gone.
• ITKImageReaderTest.cpp now passes to_underlying(OriginSpacingProcessing::Preprocessed) — the switch from uint64_t to uint64 above is what makes this actually work.

Tests — all 84 image-IO tests (SimplnxCore::ReadImage*, SimplnxCore::WriteImage*, ITKImageProcessing::ITKImageReaderFilter*, ITKImportImageStack*) pass when run sequentially (ctest -j 1) on a Rel build.

I did NOT convert ResampleImagesChoice — its values are wired into multiple other filters/pipelines and the scope felt like a separate PR. Left a comment in ReadImageStack.hpp noting that if we later share the ResampleImageGeomFilter enum, resampleImagesChoice should migrate to it too.

[JDuffeyBQ]

Also unresolved some comments that weren't addressed.

[imikejackson]

@JDuffeyBQ round 3 pushed as 0a01e83. Summary of changes since 032babf:

TiffImageIO — full rewrite of the error-handling path

• Removed the thread_local s_TiffErrorMessage + process-global TIFFSetErrorHandler / TIFFSetWarningHandler pattern. Every call now uses TIFFOpenExt with per-handle handlers set via TIFFOpenOptionsSetErrorHandlerExtR / TIFFOpenOptionsSetWarningHandlerExtR (libtiff >= 4.5). Each call site owns a stack-local std::string errorMessage and passes its address as user_data. Thread-safe by construction.
• TiffErrorGuard deleted. New TiffOpenOptionsGuard RAII. TiffHandleGuard destructor marked noexcept.
• determineTiffDataType now returns Result<DataType> and rejects unsupported (bits-per-sample, sample-format) combinations instead of coercing them into DataType::uint8. Only (8, UINT), (16, UINT), (32, IEEEFP) pass; everything else returns k_ErrorUnsupportedFormat with the actual claimed values in the message.
• numPages now uses TIFFNumberOfDirectories (side-effect-free) instead of the do { ... } while(TIFFReadDirectory) loop.
• Added TIFFGetField / TIFFSetField return-value checks for all required tags.

ReadImageStack algorithm

• Dropped the FilterList + FilterHandle + Uuid lookup dance. ReadImageFilter, ConvertColorToGrayScaleFilter, and ResampleImageGeomFilter are now constructed directly on the stack (their headers are already included). Removed the dead Application::Instance() / containsPlugin(k_SimplnxCorePluginId) guard and the FilterHandle.hpp include.
• rowLCV renamed to rowSwapCount with a comment; dropped the redundant odd/even branch.

Origin/spacing parameters switched to VectorFloat32

• Both ReadImageFilter and ReadImageStackFilter now expose k_Origin_Key / k_Spacing_Key as VectorFloat32Parameter. Eliminates the ranges::transform narrowing and every static_cast<float32>(originValues[i]) at the filter boundary. ImageGeom stores origin/spacing as float32, so this aligns parameter type with backing storage.
• Tests updated to pass std::vector<float32>.

WriteImage

• Fill-character validator now requires exactly one character (!= 1 => error) instead of the previous "empty or one".
• CreateIndexString<T> helper moved from WriteImage.cpp's anonymous namespace into simplnx/Utilities/ImageIO/ImageIOUtilities.hpp. WriteImageFilter::preflightImpl now calls it for the example-filename generation instead of assembling an fmt::runtime format string inline.

ScopedTempDir (test helper)

• Destructor is noexcept, uses std::error_code, and logs one line to std::cerr if fs::remove_all leaves files behind.

Verified

• 42/42 SimplnxCore image-IO tests pass.
• 23/23 ITKImageProcessing image-IO tests pass.
• Individual thread replies posted on each of the 13 unresolved review comments.

[imikejackson]

Summary

Overall this is a solid first iteration of the new image IO stack. The IImageIO abstraction is clean, the TiffOpenOptions / TiffHandleGuard RAII split after round-2 review is well executed, and the libtiff return-value audit looks thorough. The biggest unfinished work is in the ReadImage algorithm: it does pixel-by-pixel virtual operator[] writes through AbstractDataStore (a real CPU cost), the ConvertPixelData path produces nonsense for floating-point sources, and several parts of ReadImageStack still carry forward awkward patterns from the pre-refactor code (suspicious cropping fallback when getIndex returns nullopt, misleading tempDataStore naming, per-slice grayscale cost). Nothing here is a merge blocker except the multi-component fix in ReadImageUtils.hpp, which is correct and worth landing on its own.

C++ best practices

• [Praise] std::span<uint8> / std::span<const uint8> on the IImageIO interface (src/simplnx/Utilities/ImageIO/IImageIO.hpp:57,71) is the right modern choice. Cleanly decouples the interface from std::vector and lets callers pass any contiguous buffer.

• [Praise] std::optional<FloatVec3> in ImageMetadata (src/simplnx/Utilities/ImageIO/ImageMetadata.hpp:28-29) — the doc comment correctly motivates this as "file had it" vs "file did not", which is exactly what optional is for.

• [Praise] RAII used consistently for the libtiff handle and options, and for the ScopedTempDir in WriteImageTest.cpp. All = delete for copy/move on the guard types is correct because m_ErrorMessage's address is captured by libtiff.

• [High] GetDataTypeSize and the two ImageIOUtilities.hpp choice helpers throw std::runtime_error (src/simplnx/Common/TypesUtility.hpp:228, src/simplnx/Utilities/ImageIO/ImageIOUtilities.hpp:43,63). The rest of this codebase uses Result<T> / MakeErrorResult everywhere and reserves exceptions for genuinely unrecoverable conditions. These three helpers are called from preflightImpl and executeImpl paths (e.g. ReadImageStackFilter.cpp:209,469, ReadImageFilter.cpp:331) where an unsupported choice index is a user-input error, not a logic invariant. A pipeline runner that does not catch these will crash. Convert to Result<T> returns and have callers propagate via MakeErrorResult. If you want to keep the throw for GetDataTypeSize::default because all live DataType values are covered, at minimum mark it [[noreturn]]-friendly with a comment that the default is genuinely unreachable, and prefer a static_assert-like trap (e.g. assert false, then return 0) over an exception in a constexpr inline header.

• [Medium] int32 vs int32_t mixed across the two backends — StbImageIO.cpp:22-27 uses int32, TiffImageIO.cpp:17-22 uses int32_t. Pick one. The codebase convention is the simplnx alias int32.

• [Medium] The ChoicesParameter value 0/1/2 → OriginSpacingProcessing enum cast is done with static_cast<OriginSpacingProcessing>(filterArgs.value<ChoicesParameter::ValueType>(...)) in three places (ReadImageFilter.cpp:175,329, ReadImageStackFilter.cpp:165,461, WriteImageFilter.cpp doesn't have it). The header comment claims reinterpret_cast is also valid, which is wrong and dangerous to suggest — only static_cast from the integral underlying type is well-defined. Update ImageIOEnums.hpp:13-14 to drop the reinterpret_cast<> mention.

• [Medium] Missing noexcept on IImageIO::~IImageIO() is fine because = default is implicitly noexcept for trivial dtor, but the explicit noexcept on derived dtors in StbImageIO/TiffImageIO (StbImageIO.hpp:19, TiffImageIO.hpp:22) is good. Be consistent.

• [Low] std::transform(ext.begin(), ext.end(), ext.begin(), ::tolower) (ImageIOFactory.cpp:15, StbImageIO.cpp:142) is undefined behavior on negative char values on platforms where char is signed. Use [](unsigned char c){ return std::tolower(c); }.

• [Low] WriteImageFilter.cpp:163 builds an example filename with a hard-coded / separator and fs::absolute(filePath).parent_path().string() — should use fs::path operator/ everywhere for consistency.

• [Low] int strideBytes = w * comp; (StbImageIO.cpp:153) does signed int multiplication. With w*comp > INT_MAX (huge images) this overflows. Compute in usize then narrow with a check.

• [Nit] ReadImageFilter.hpp:12 doc comment is /** @brief This filter will .... */ — placeholder text. Replace with one sentence that says what the filter does. Same for ReadImageStackFilter.hpp:12.

• [Nit] WriteImageFilter.cpp:11/WriteImage.cpp:8 includes Utilities/FilterUtilities.hpp but WriteImage.cpp actually does need it (for ExecuteDataFunction). WriteImageFilter.cpp does not — drop.

• [Nit] executionContext parameter is unused in preflightImpl/executeImpl of all three filters. Standard noise across the codebase, but [[maybe_unused]] would silence the warning if -Wunused-parameter is ever enabled.

Object-oriented design

• [Praise] IImageIO is a clean small interface — three pure virtual methods, virtual dtor, deleted copy/move on the base, no protected state, no leaked backend types in the signature. Adding a new format (e.g. a JpegXLImageIO) requires no changes outside the new file plus one branch in CreateImageIO. Good separation.

• [Praise] TiffOpenOptions correctly bundles the TIFFOpenOptions* and the captured std::string error buffer. = delete on copy/move is the right call because libtiff stores the string's address as user_data. Comment at TiffImageIO.cpp:29-32 clearly explains why.

• [Praise] TiffHandleGuard is a focused single-responsibility RAII. Good split from TiffOpenOptions.

• [Medium] The factory CreateImageIO(filePath) (ImageIOFactory.cpp:12) keys off file extension and hard-codes the supported extension list inside an if/else. Two scaling problems coming: (1) when a third backend lands the function will become an extension chain; (2) the same extension list is duplicated in ReadImageFilter::parameters() (ReadImageFilter.cpp:103). Consider adding static const std::map<std::string, std::function<std::unique_ptr<IImageIO>()>> g_Registry; plus a GetSupportedReadExtensions() helper that the filter parameter setup can call. Not a blocker; flag for the next backend.

• [Medium] WriteImageFilter::parameters() declares ExtensionListType{} (empty) for the output FileSystemPathParameter (WriteImageFilter.cpp:79). The user can type any extension and only preflightImpl validates via CreateImageIO. Better UX is to populate the dropdown with {".tif"},{".tiff"},{".png"},{".bmp"},{".jpg"},{".jpeg"} from the same source as CreateImageIO. Tied to the previous item.

• [Medium] ReadImageStackFilter::preflightImpl re-runs sub-filter preflight by dynamic_cast-ing outputActions.actions[0] to CreateImageGeometryAction and outputActions.actions[1] to CreateArrayAction (ReadImageStackFilter.cpp:219-224,300-305,343-365). This is fragile — any future change to ReadImageFilter::preflightImpl that reorders or adds actions silently breaks the stack filter at runtime. The error messages at -23530/-23531/-23532/-23533 say "Internal error" which is honest but the user is not the one who can fix it. Either (a) provide a small helper API on the sub-filter that returns the geometry/array actions explicitly, or (b) iterate actions and pick by dynamic_cast rather than by index. Add a comment in ReadImageFilter::preflightImpl warning that the action ordering is depended on by ReadImageStackFilter.

• [Low] Filter-level code knows almost nothing about libtiff/stb — that part is well isolated. The ImageMetadata / IImageIO boundary is honored. Good.

• [Low] BuildReadImageFilterArgs is exported (SIMPLNXCORE_EXPORT) which leaks an internal coupling helper across the DLL boundary. If only the algorithms call it, drop the export and put it in an anonymous namespace.

Defensive programming

• [Praise] Round-2 audit looks complete: every TIFFGetField/TIFFSetField for required tags is checked at TiffImageIO.cpp:179, 188, 261, 268, 386-388, 398-409, 417. Optional tags (TIFFTAG_XPOSITION, TIFFTAG_YPOSITION, TIFFTAG_XRESOLUTION, TIFFTAG_YRESOLUTION) are intentionally allowed to fail (lines 210-241, 427-439) with a comment explaining why. TIFFReadScanline < 0 and TIFFWriteScanline < 0 are both checked (lines 346, 448).

• [Praise] Buffer-size contract is explicit and enforced on both sides — StbImageIO.cpp:90, TiffImageIO.cpp:282, 366.

• [Praise] No raw pointers escape AbstractDataStore. All .data() calls in the diff are on std::vector<uint8> / std::span<uint8> local buffers.

• [High] ReadImageStack.cpp:166-176 (the physical Z-cropping path) calls initialImageGeom.getIndex(...) which returns std::optional<usize>. If the optional is empty, the code silently leaves startSlice = 0 / endSlice = files.size() - 1, producing the wrong slice range without warning. Either return an error or fall back to the voxel-cropping math the preflight already validated. Right now this is dead-reckoning into corruption.

• [High] ReadImage.cpp:228-235 does int64 voxelX = (xMin - srcOrigin) / srcSpacing and clamps negative to zero, but does not check the upper bound against srcWidth. If xMin > image extent, voxelX is huge, xStart becomes garbage, and the only protection is the xStart + dstWidth > srcWidth check at line 240, which fires with an unhelpful "crop window does not fit" error. Validate voxelX < srcWidth here and produce a meaningful "physical X minimum is outside source image" message. Same for Y.

• [High] TiffImageIO.cpp:337 tsize_t scanlineSize = TIFFScanlineSize(tiff); — tsize_t is signed, and libtiff returns 0 or -1 on error. The std::max(static_cast<usize>(scanlineSize), rowBytes) on line 341 will treat -1 as a huge usize and allocate ~16 EiB. Check scanlineSize <= 0 first and return an error.

• [High] ReadImageStackFilter.cpp:251-252 casts a bool * float32 pattern: originZ = (shouldChangeOrigin && originSpacingProcessing == Preprocessed) ? origin[2] : 0; — but origin[2] is float32 and the 0 is int. Fine for ?: here, but on the next line spacingZ defaults to 1 (also int). If spacing[2] happens to be 0.0f, the divide at line 277 is checked, but only when shouldChangeSpacing && Preprocessed. The default spacingZ = 1 masks the case where the user has Z spacing of 0 in Postprocessed mode. This isn't terribly likely but it crosses paths weirdly. Use 1.0 literals.

• [Medium] DispatchConversionFunctor on ReadImage.cpp:113-120 calls ExecuteDataFunction with srcType which can be float32, int8, etc. — see CPU performance section for the resulting nonsense conversion. A guard at the dispatch site rejecting unsupported srcType would catch this defensively.

• [Medium] ReadImageFilter::preflightImpl line 199-200 uses metadata.origin.value_or(...) directly, which is fine, but if(metadata.origin.has_value() && shouldChangeOrigin) is never used as a sanity warning. Worth telling the user "the file specifies origin X but you are overriding to Y" via a PreflightValue.

• [Medium] TIFFGetField for optional XPOSITION/YPOSITION (TiffImageIO.cpp:210-217) sets hasOrigin = true if either present, but if only X is present, yPosition stays 0.0f and is silently written into metadata.origin[1]. That's probably acceptable, but worth a one-line comment noting the partial-presence behavior.

• [Medium] WriteImageFilter::preflightImpl does not validate that totalDigits >= 1. Negative or zero totalDigits is passed to CreateIndexString which gives an fmt::format exception (then thrown out of preflight). Add a bounds check and return MakeErrorResult.

• [Low] WriteImage.cpp:159-166 does fs::create_directories if parentDir does not exist; a std::error_code overload would let you report the OS-level reason on failure rather than a generic "Error creating output directory".

• [Nit] Numeric overflow on width * height * components * bpe is not currently a real issue (everything is usize), but the cast chain static_cast<usize>(width) * static_cast<usize>(height) * static_cast<usize>(samplesPerPixel) * bpe (TiffImageIO.cpp:281) is verbose because width/height are uint32_t. A helper usize TotalBufferBytes(width, height, comps, bpe) would dedupe the four near-identical computations across the two backends and the two algorithm files.

Memory allocation

• [Praise] TIFF scanline buffer is allocated once outside the read loop (TiffImageIO.cpp:342). Good.

• [Praise] WriteImage slice buffer allocated once outside the slice loop (WriteImage.cpp:173); ImageMetadata hoisted out of the loop (lines 176-181) — exactly right.

• [Praise] No allocation per scanline.

• [High] TiffImageIO.cpp:297 allocates std::vector<uint32_t> raster(width * height) on the tiled-TIFF path. For a 16384x16384 tiled TIFF that is 1 GiB. That is the libtiff TIFFReadRGBAImage API constraint, not your fault, but it should be noted in a comment, and you should consider using TIFFReadRGBAStrip / TIFFReadRGBATile to read in chunks. At minimum, reject upfront if width * height * 4 > some_threshold rather than failing inside std::vector::vector with std::bad_alloc propagating out.

• [Medium] ReadImage.cpp:159 allocates std::vector<uint8> tempBuffer(bufferSize) for the entire image. A 65535x65535 RGBA float32 image is 64 GiB. Acceptable for typical inputs but document the upper bound and consider scanline-by-scanline streaming for the next iteration.

• [Medium] ReadImageStack's per-slice grayscale conversion (ReadImageStack.cpp:255-281) constructs a fresh ConvertColorToGrayScaleFilter, builds a fresh Arguments, and runs preflight+execute on a fresh DataStructure — every single slice. The grayscale filter is presumably cheap, but the Arguments map and DataStructure setup is not free per slice. Same for the per-slice ResampleImageGeomFilter invocation (lines 211-245). For a 1000-slice stack this is 1000 sub-filter setups. Hoist the Arguments outside the slice loop and only rewrite the file path; pre-create a single ResampleImageGeomFilter and ConvertColorToGrayScaleFilter instance.

• [Medium] FlipAboutYAxis (ReadImageStack.cpp:62-91) allocates std::vector<T> currentRowBuffer(dims[0] * numComp) — fine — but the variable name tempDataStore (line 65) is not a temp, it's a reference to the live data store. Misleading name. Rename to dataStore or dataStoreRef per the project convention.

• [Low] WriteImage.cpp:192 std::fill(sliceBuffer.begin(), sliceBuffer.end(), 0) zeros the slice buffer every iteration. The extract loop overwrites every byte of it (the dst index covers the full slice). The fill is dead work — drop it. If you keep it as a defensive measure, add a comment.

CPU performance

• [High] The pixel copy loops in ReadImage.cpp (CopyPixelDataFunctor::operator(), lines 43-72) and WriteImage.cpp (ExtractSliceFunctor, lines 37-96) call dataStore[index] = value / dataStore.getValue(index) once per scalar component. AbstractDataStore::operator[] and getValue are virtual. For a 4096x4096 RGBA image that is 67M virtual calls per image. For an in-core DataStore<T>, the underlying memory is contiguous; you could memcpy whole rows when srcType == destType and nComps is uniform. The user's prompt mentioned copyIntoBuffer/copyFromBuffer — those APIs do not appear to exist in this branch (only tuple-to-tuple copyFrom exists at AbstractDataStore.hpp:654). If a bulk-copy API is genuinely planned for OOC, file a follow-up issue and add a TODO referencing it. Either way, a fast in-core memcpy fallback when (a) no cropping, (b) srcType == destType, and (c) the destination is a flat DataStore would dramatically cut read/write time.

• [Blocker for HDR images] ConvertPixelDataFunctor<float32> (ReadImage.cpp:75-111) uses constexpr double srcMax = std::numeric_limits<float32>::max() ≈ 3.4e38 to normalize, then multiplies by destMax. For any sane HDR pixel value (say 0.5), normalized = 0.5 / 3.4e38 ≈ 0, so all output is zero. The conversion math only makes sense for fixed-range integer types where max() is the saturation value. For float32 source, you either need a range probe (min/max scan) or you should reject the conversion. Today this is reachable: stb produces float32 for HDR, the user can request Set Image Data Type = uint8, and the result is a black image. Either reject the combination in preflightImpl or detect float source in DispatchConversionFunctor and dispatch a float32 -> integer branch that uses std::clamp(value * 255.0, 0.0, 255.0) semantics.

• [Medium] ReadImageStack.cpp:179 per-slice messageHandler(IFilter::Message::Type::Info, fmt::format("Importing: {}", filePath)) formats a message every iteration. For thousands of slices this is wasted work behind a probably-no-op handler. Use a ThrottledMessenger (the codebase has one — see CLAUDE.md progress-messaging skill).

• [Medium] Same point in WriteImage.cpp:190 — Writing slice X/Y per slice. Throttle.

• [Medium] inputFilePath.string() is called multiple times per readMetadata / readPixelData in both backends (e.g. TiffImageIO.cpp:167, then again on every error path). Hoist to const std::string pathStr = filePath.string(); once at the top — already done in places, just make it consistent.

• [Medium] FlipAboutXAxis (ReadImageStack.cpp:93-120) does element-by-element scalar swap with getValue/operator[]. For numComp components per pixel and dims[0]*dims[1] pixels, that is 3 virtual calls per scalar. Swap full row buffers via std::swap_ranges on the Iterator returned by begin(), like FlipAboutYAxis does for the row copy. Same algorithmic complexity (O(n)), but real constant-factor savings.

• [Low] FlipAboutYAxis::currentRowBuffer could be allocated once outside the per-row loop if this code is ever lifted into a slice-scope context. Not currently necessary because the function is invoked per slice.

• [Low] ReadImageStack orchestration constructs a fresh ReadImageFilter, ReadImageSubFilterConfig, and Arguments per slice (ReadImageStack.cpp:185-202). The Arguments map setup alone is non-trivial. Build the Arguments once with the per-slice mutable fields (filePath, anything else slice-dependent — looks like only filePath) updated per iteration via insertOrAssign.

• [Low] Tile/scanline read loops are sequential. The TIFF scanline read API is intrinsically serial because TIFFReadScanline advances state, but you could read N scanlines into one buffer using TIFFReadEncodedStrip for stripped TIFFs. Defer until benchmarks justify it.

Top 3 things to fix before merge

1. Float32 source conversion produces black images — ConvertPixelDataFunctor<float32> divides by numeric_limits<float32>::max(). Either reject float32 → integer conversions in ReadImageFilter::preflightImpl, or implement a proper float→integer normalization (clamp(value * 255.0, 0, 255) or a min/max scan). See src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/ReadImage.cpp:75-111.
2. Replace throw std::runtime_error in user-input paths with Result<T> — GetDataTypeSize (src/simplnx/Common/TypesUtility.hpp:228) and the two ImageIOUtilities.hpp choice helpers (src/simplnx/Utilities/ImageIO/ImageIOUtilities.hpp:43,63) throw on unexpected enum values reachable from executeImpl/preflightImpl. Convert to Result<T> and propagate via MakeErrorResult. The current behavior crashes any pipeline runner that does not catch.
3. Validate TIFFScanlineSize and the optional-from-getIndex fallback paths — TiffImageIO.cpp:337-341 will allocate ~16 EiB if libtiff returns -1 from TIFFScanlineSize; check <= 0 first. ReadImageStack.cpp:166-176 silently uses defaults when getIndex(...) returns nullopt, masking malformed Z bounds — return an error or fall back to the validated voxel path.

Honorable mention follow-ups (not blockers but worth filing): the index-based dynamic_cast<CreateImageGeometryAction*>(actions[0]) coupling between ReadImageStackFilter and ReadImageFilter is fragile and will eventually break; the per-slice sub-filter construction in ReadImageStack is wasteful for large stacks; rename the misleading tempDataStore variables in FlipAbout{X,Y}Axis; and stop std::fill-ing the slice buffer in WriteImage since the extract loop overwrites every byte.

---

Posted by an automated reviewer agent (Opus 4.7) — use as input, not gospel.

[imikejackson]

Design Review: PR #1585 (topic/image_io_rewrite)

This branch carves a format-agnostic IImageIO layer out of the monolithic ITK image-IO stack and ships three new SimplnxCore filters (ReadImageFilter, WriteImageFilter, ReadImageStackFilter) that consume it. The shape is two strategy backends (StbImageIO for PNG/JPEG/BMP, TiffImageIO for TIFF) sitting behind a byte-buffer interface, fronted by a small CreateImageIO(extension) factory. The new filters follow the established Filter / Algorithm split, and the stack filter is implemented by orchestrating other simplnx filters per slice rather than reinventing them. A latent multi-component bug in the existing ITK reader is fixed in passing.

This review focuses only on the design choices. Code-quality nits live in a separate review.

1. Why a new IImageIO abstraction at all?

The branch adds:

• src/simplnx/Utilities/ImageIO/{IImageIO,ImageIOFactory,StbImageIO,TiffImageIO,...}.hpp/cpp
• src/Plugins/SimplnxCore/src/SimplnxCore/Filters/{Read,Write,ReadStack}*Filter.{hpp,cpp}

These parallel the pre-existing ITK-based readers (ITKImageReaderFilter, ITKImportImageStackFilter, ITKImageWriterFilter) which still exist and still ship. So the question is: why have two of these?

Inferred rationale:

• Move common-case image IO out of the heavy ITK plugin. Today the only way a user reads a PNG/TIFF in simplnx is to enable ITKImageProcessing, which pulls in all of ITK's image-IO factories, format readers, and template instantiations. ITK is heavyweight at compile and link time, and adds non-trivial transitive dependencies. Hosting PNG/JPEG/BMP/TIFF in SimplnxCore via stb (header-only) and libtiff (single small lib) means the bulk of users never have to build or install ITK to load a TIFF stack. Build-system evidence: vcpkg.json adds only stb and tiff; CMakeLists.txt:189-194 adds find_package(Stb REQUIRED) and find_package(TIFF REQUIRED); the executable links only TIFF::TIFF (stb is header-only).
• Plug a real correctness bug surface in the ITK path. This same PR fixes a multi-component-pixel bug in ITKImageProcessing/Common/ReadImageUtils.hpp:43 (see Section 8). That bug was masked behind an opaque std::transform(rawBufferPtr, rawBufferPtr + pixelContainer->Size(), ...) deep inside template machinery. The new path is much shallower: IImageIO::readPixelData(path, span<uint8>) lands the bytes, and ReadImage walks the buffer with explicit offsets it computed itself. The number of templates between "file on disk" and "byte in DataStore" is far smaller; bugs of this class become far harder to write.
• Leave a clean OOC-friendly path. ITK insists on materializing an itk::Image<T,N> object as the intermediate. For an OOC DataStore<T>, that means an entire-volume scratch buffer in addition to the destination. The new IO contract intentionally exposes only one slice's worth of bytes at a time (see Section 4 / 6) so the scratch buffer is bounded.

The trade-off is that simplnx now carries two parallel image-IO ladders. The ITK one is broader (signed types, 3D MHA, more legacy formats) and is preserved for those use cases; the new one is the recommended path for the common 2D scientific imaging cases.

[Validated]. Splitting the common path off ITK is the right call given how heavyweight that dependency is and how shallow the new path is. The decision to keep the ITK filters rather than deprecate them is also correct — they're still load-bearing for formats stb/libtiff don't cover. The one thing this PR should make explicit (in the WriteImageFilter / ReadImageFilter docs) is which filter a user should reach for first; right now both pairs are equally discoverable via tags (ITKImageReaderFilter.cpp even adds "image","jpg","tiff","bmp","png" tags in this PR, making them indistinguishable in search).

2. Strategy + factory vs alternatives

The chosen design is IImageIO (src/simplnx/Utilities/ImageIO/IImageIO.hpp:27) — a 3-method abstract base — implemented twice (StbImageIO, TiffImageIO), constructed by a free function that switches on extension:

// src/simplnx/Utilities/ImageIO/ImageIOFactory.cpp:12
Result<std::unique_ptr<IImageIO>> nx::core::CreateImageIO(const std::filesystem::path& filePath)
{
  std::string ext = filePath.extension().string();
  std::transform(ext.begin(), ext.end(), ext.begin(), ::tolower);
  if(ext == ".png" || ext == ".jpg" || ext == ".jpeg" || ext == ".bmp")
    return {std::make_unique<StbImageIO>()};
  if(ext == ".tif" || ext == ".tiff")
    return {std::make_unique<TiffImageIO>()};
  return MakeErrorResult<std::unique_ptr<IImageIO>>(-20200, ...);
}

Inferred rationale:

• The interface is small (3 methods, ~60 lines), and there are only two backends. A std::variant<StbImageIO, TiffImageIO> would also work, but it would force every caller to std::visit and would couple every consumer to the full set of backend headers (one of which transitively includes tiffio.h, the other stb_image.h with implementation macros). The virtual-call cost is irrelevant — it happens once per file.
• The factory is a free function, not a class with a registration table. A self-registration registry would let downstream plugins add formats without modifying ImageIOFactory.cpp. That doesn't apply here: simplnx itself owns image IO, and adding a format means adding a vcpkg dependency, so the central switch is the right level of friction.
• Compile-time selection (e.g. tag-dispatched read<".tif">(path)) was a non-starter because the extension is a runtime input.

Alternatives considered and (correctly) not taken:

• std::variant: forces header pollution, makes adding a backend an ABI break.
• Self-registering registry: more machinery than the problem deserves at N=2.
• Returning a function-pointer struct rather than an object: would have worked but loses the option of stateful backends; TiffImageIO is currently stateless but the handle abstraction (TiffOpenOptions per-call) shows that any future caching of e.g. an open MHA file across slices would want object state.

One small wart: CreateImageIO lives at namespace scope as CreateImageIO(...), not IImageIO::create(...). That's a minor inconsistency with the Result-returning factory pattern used elsewhere — but not worth fixing.

[Validated]. Strategy + factory is the obvious right call at this scope. Nothing in the design closes the door on adding .dcm, .nrrd, .exr later — each is a new subclass + two lines in ImageIOFactory.cpp. The interface deliberately has no concept of DataStore, ImageGeom, or Result<OutputActions> (IImageIO.hpp:27 lives in simplnx/Utilities/, not in any Filter/ subdirectory), so the layer is reusable from anywhere — algorithms, tests, future Python bindings.

3. Filter / Algorithm split

The CLAUDE.md mandate is NameFilter.{hpp,cpp} for parameter wiring + preflight; Algorithms/Name.{hpp,cpp} for execution. The PR follows this:

• ReadImageFilter.cpp:309 — executeImpl packs everything into a ReadImageInputValues struct (Algorithms/ReadImage.hpp:19) and calls ReadImage(dataStructure, messageHandler, shouldCancel, inputValues)().
• WriteImageFilter.cpp:173 — same shape, dispatches to WriteImage.
• ReadImageStackFilter.cpp:447 — same, dispatches to ReadImageStack.

Things done well:

• The *InputValues structs in Algorithms/*.hpp are pure data — no IFilter::* types, no Arguments, no parameter keys. The algorithm file is buildable in isolation against the input struct.
• Algorithm files only depend on the IO subsystem and DataStructure — they #include "simplnx/Utilities/ImageIO/IImageIO.hpp" and DataStructure headers, not any filter headers.

Things that leak:

• Algorithms/ReadImage.hpp:9 includes simplnx/Filter/IFilter.hpp for the MessageHandler type used by the constructor signature. This is consistent with the rest of the codebase's algorithm-class convention but is worth noting: the algorithm is not fully filter-agnostic — it knows about IFilter::Message::Type::Info (ReadImage.cpp:140). This is a project-wide convention, not new to this PR.
• Algorithms/ReadImageStack.cpp:3-5 includes three sibling filter headers (ReadImageFilter.hpp, ConvertColorToGrayScaleFilter.hpp, ResampleImageGeomFilter.hpp) and constructs / executes them from inside the algorithm. This breaks the "algorithm depends only on data + IO" pattern — see Section 7 below.

The ReadImageSubFilterConfig struct (Algorithms/ReadImageStack.hpp:30) and the BuildReadImageFilterArgs(config) helper (Algorithms/ReadImageStack.cpp:30-52) are a nice touch: the same builder is used by ReadImageStackFilter::preflightImpl (first-slice preflight) and ReadImageStack::operator() (per-slice execute), so the two paths can never drift in what arguments they hand the sub-reader. That symmetry is a real maintenance win.

[Reasonable, with caveats]. The split is clean for ReadImage and WriteImage. For ReadImageStack, the "algorithm" is doing filter orchestration, which is a different shape than the convention assumes. The convention probably needs to grow a third role ("Workflow" or "Composite"), but that's beyond this PR.

4. OOC-safety in the IO layer

The IO contract is intentionally byte-oriented:

// src/simplnx/Utilities/ImageIO/IImageIO.hpp:57
virtual Result<> readPixelData(const std::filesystem::path& filePath, std::span<uint8> buffer) const = 0;
virtual Result<> writePixelData(const std::filesystem::path& filePath, std::span<const uint8> buffer, const ImageMetadata& metadata) const = 0;

Inferred rationale:

• uint8 instead of T: avoids template explosion at the IO boundary. The backends switch internally on metadata.dataType (StbImageIO.cpp:95-127, TiffImageIO.cpp:142-160) but expose one ABI to callers. New types added inside the IO layer don't ripple through every caller.
• std::span<uint8> instead of std::vector<uint8>&: lets the algorithm own the buffer and pass either a stack-allocated, heap-allocated, or even sub-range view. The IO layer can't accidentally resize() the buffer (it can only verify size and memcpy). The size validation actually happens inside both backends (StbImageIO.cpp:90, TiffImageIO.cpp:282), which gives a useful error rather than a silent overflow.
• OOC preservation in the algorithm layer: ReadImage::operator() (ReadImage.cpp:159) allocates std::vector<uint8> tempBuffer of one image's worth of bytes — not the whole volume — then writes into the destination via dataStore[dstIndex + c] = ... (ReadImage.cpp:67, 105), one element at a time. Crucially, no dataStore.data() raw pointer is ever taken; every store goes through operator[] (AbstractDataStore<T>::operator[]), which is the OOC-safe access path. Per the project's OOC docs, raw pointers into a DataStore are unsafe because some stores are out-of-core; this code respects that.

What this design deliberately gives up:

• Type safety at the IO boundary: readPixelData cannot enforce that the buffer's element type matches metadata.dataType at compile time. The runtime size check catches mismatches but not e.g. a uint16 buffer presented as uint8. The mitigation is ReadElementAs<T> (ReadImage.cpp:36) which does type-safe memcpy reads from the byte buffer at the correct offsets — strict aliasing is preserved, byte order is whatever the platform produces, which matches what stb/libtiff write into the buffer.
• Endianness handling at the IO layer: nobody addresses byte order. libtiff handles its own endianness; stb stores in native uint16, so all memcpy-roundtrips are native. This is fine in practice but worth a comment somewhere.

[Validated]. Bytes-and-spans is the right shape for an IO boundary. It deliberately keeps the IO layer dumb (no DataStructure, no DataType-templated readPixelData), which in turn lets the algorithm decide how much to materialize and how to land it in the store. The OOC discipline (per-tuple operator[], no raw pointers, bounded scratch buffer) is consistent throughout.

5. Per-handle libtiff error capture

The PR introduces TiffOpenOptions (TiffImageIO.cpp:33-95):

class TiffOpenOptions
{
public:
  TiffOpenOptions() : m_Opts(TIFFOpenOptionsAlloc())
  {
    if(m_Opts != nullptr)
    {
      TIFFOpenOptionsSetErrorHandlerExtR(m_Opts, errorHandler, &m_ErrorMessage);
      TIFFOpenOptionsSetWarningHandlerExtR(m_Opts, warningHandler, nullptr);
    }
  }
  // non-copyable/non-movable; pinned because libtiff stores &m_ErrorMessage as opaque user_data
  ...
private:
  static int errorHandler(TIFF*, void* user_data, const char*, const char* fmt, va_list args) { /* writes to *user_data */ }
  TIFFOpenOptions* m_Opts = nullptr;
  std::string m_ErrorMessage;
};

Inferred rationale: the prior approach in much C++ libtiff usage is TIFFSetErrorHandler (process-global). That has two problems:

1. Thread safety. Two threads opening TIFFs concurrently race on the global handler and the global error string buffer.
2. Non-locality. The handler must store errors somewhere callable code can read them. Process-globals leak across call boundaries; thread-local fixes thread safety but still leaves "whose error is this?" ambiguity when libtiff calls the handler more than once per operation.

Per-handle (via TIFFOpenOptionsSetErrorHandlerExtR, libtiff 4.5+) cleanly fixes both: each TIFFOpenExt call gets its own handler bound to its own error-string slot. The RAII object's correctness depends on &m_ErrorMessage staying valid until TIFFClose runs; that's why TiffOpenOptions is non-copyable AND non-movable (TiffImageIO.cpp:54-57) — moving would invalidate the captured pointer libtiff is holding. Good attention to that detail.

The design correctly treats warnings differently — the warning handler is a no-op (TiffImageIO.cpp:88) because libtiff's warnings are noisy and benign for valid files (e.g. unknown private tags). That's a deliberate UX choice and the right one for a library consumer.

One small concern: m_ErrorMessage is captured by vsnprintf into a 1024-byte stack buffer and then assigned (TiffImageIO.cpp:81-83). If libtiff produces multiple errors during one open (which it can — e.g. "unknown tag 700" followed by "cannot read scanline N"), only the last error is preserved. For diagnostic purposes the first is usually more useful. Worth keeping the first-error semantics for a follow-up.

[Validated]. Per-handle is unambiguously correct for any code that may run on a worker thread. Project-wide, ReadImageStack is currently single-threaded for slices (one slice file at a time, see Algorithms/ReadImageStack.cpp:179), but anything that decides to parallelize stack reading later is now thread-safe by construction. The pinned-address constraint is correctly enforced by =delete on copy/move.

6. WriteImage temp-buffer strategy

Algorithms/WriteImage.cpp:172-220 allocates one slice worth of bytes once and reuses it across all slices:

usize sliceBufferSize = sliceW * sliceH * nComp * bytesPerComponent;
std::vector<uint8> sliceBuffer(sliceBufferSize);
// ImageMetadata is invariant across slices for a single volume; hoist out of the loop.
ImageMetadata metadata; ...
for(usize slice = 0; slice < sliceCount; ++slice)
{
  std::fill(sliceBuffer.begin(), sliceBuffer.end(), uint8(0));
  ExtractSliceFunctor()(...);                      // typed walk into sliceBuffer
  imageIO->writePixelData(atomicFile.tempFilePath(), sliceBuffer, metadata);
  atomicFile.commit();
}

Inferred rationale:

• Bounded peak memory regardless of volume size. Writing a 1000-slice 4096x4096x3-channel uint16 volume holds at most ~96 MB of scratch (one slice), not 96 GB (whole volume). For an OOC volume this is essential; you cannot pull all slices into RAM by definition.
• Allocate-once, refill-many. The buffer is sized for the largest slice the loop will produce (which is constant since slice dimensions are fixed by plane choice); we just std::fill and refill each iteration. This avoids per-slice allocator churn.
• Hoist invariant ImageMetadata. Width/height/components/dtype are constant across slices for a single volume; the metadata object is built once outside the loop.
• Atomic per-slice write. Each slice goes through AtomicFile::Create -> writePixelData -> commit (WriteImage.cpp:203-220), so a partial write of slice N doesn't corrupt slice N's filename — it leaves either the previous file or nothing.

What this gives up: streaming writes. For TIFF, libtiff supports TIFFWriteScanline (TiffImageIO.cpp:444-452 already uses this internally); in principle the WriteImage algorithm could open a TIFF once and stream scanlines directly out of the DataStore without an intermediate buffer at all. That would shave the slice buffer entirely. However:

• stb_image_write for PNG/JPEG/BMP requires the whole image in memory anyway — those formats are not streamable.
• A "streaming for TIFF, buffered for stb" branch in the IO layer would leak format details up. The current uniform-buffer design trades a bounded scratch buffer for a clean ABI.

[Validated]. Per-slice scratch is the right correctness baseline. A future "streaming write" mode could be added as a IImageIO::beginStream() / writeRow() / endStream() extension if profiling shows the slice buffer matters, but for now the simpler design is correct and bounded.

7. Subfilter composition in ReadImageStack

This is the most interesting design choice in the PR. ReadImageStack does not implement per-slice reading itself; for each slice it constructs and execute()s three other simplnx filters into a scratch DataStructure, then copyFroms the result into the destination volume:

// src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/ReadImageStack.cpp:184-210
DataStructure importedDataStructure;
{
  ReadImageFilter imageReader;
  ReadImageSubFilterConfig subConfig{}; // ...
  IFilter::ExecuteResult executeResult = imageReader.execute(importedDataStructure, BuildReadImageFilterArgs(subConfig));
  ...
}
if(resample != k_NoResampleModeIndex) { ResampleImageGeomFilter::execute(...); }
if(convertToGrayscale && validInputForGrayScaleConversion) { ConvertColorToGrayScaleFilter::execute(...); }
// then copyFrom(...) into the destination DataStore

Inferred rationale:

• Don't reimplement. ReadImage already knows how to read+crop+convert-type a single slice through the IO layer. ResampleImageGeomFilter already knows how to bilinear-resample a 2D geometry. ConvertColorToGrayScaleFilter already knows the luminosity coefficients. Reimplementing these inline would mean three new code paths to maintain in lockstep with three existing ones; bug fixes would have to be ported.
• Symmetry with the preflight. ReadImageStackFilter::preflightImpl (ReadImageStackFilter.cpp:212-217, 336-341, 396-402) does the same dance — it preflight()s the same three sub-filters into a scratch DataStructure to learn the output shape, then copies the resulting CreateImageGeometryAction and CreateArrayAction into its own action list with the Z dimension expanded. If execute reproduced the geometry and resample logic inline, the preflight and execute could disagree on geometry shape; the current design uses the same code paths in both.

What this costs:

• Per-slice allocation churn. A fresh DataStructure is built per slice (Algorithms/ReadImageStack.cpp:184), ReadImageFilter::executeImpl allocates a std::vector<uint8> tempBuffer per slice (ReadImage.cpp:159), the resample filter allocates intermediate arrays, etc. For a 1000-slice stack you do 1000 of these. The dominant cost is the per-slice file IO (which has to happen anyway), so this likely doesn't show up in profiles, but it's not free.
• Some preflight cost runs N times. Each ReadImageFilter::execute call internally re-runs preflight, which itself opens the file to read metadata. So each slice file is opened twice: once for preflight, once for read. For TIFF this is two TIFFOpenExt calls per slice. Again, unlikely to matter unless someone profiles a tiny-slice case.
• Sub-filter parameter coupling. Whenever someone changes ReadImageFilter's parameter keys, ReadImageStack's sub-filter argument builder must change in lockstep. The PR mitigates this with ReadImageSubFilterConfig + BuildReadImageFilterArgs as a single coupling point, but the coupling is real.

The grayscale-then-rename dance (Algorithms/ReadImageStack.cpp:255-281) is genuinely awkward: convert to grayscale, get an array named gray<original>, delete the original, rename the gray one back. That's an artefact of ConvertColorToGrayScaleFilter not having an in-place mode. The right long-term fix is on the grayscale filter, not here.

[Reasonable, with caveats]. Composition over inlining is the correct long-term call — the "don't reimplement" argument dominates. The acknowledged costs (per-slice scratch DS, double preflight, rename dance) are real but each has a known mitigation if profiling demands it. The one improvement worth making in this PR: the algorithm already calls ReadImageFilter::execute() directly, which re-runs preflight every time; consider a ReadImage algorithm-level entry point the stack code can use to skip the per-slice preflight. That would also remove Algorithms/ReadImageStack.cpp's dependency on the filter header (currently it includes SimplnxCore/Filters/ReadImageFilter.hpp).

8. The ITK multi-component bug fix

Before:

// previous src/.../ReadImageUtils.hpp ConvertImageToDataStoreAsType()
std::transform(rawBufferPtr, rawBufferPtr + pixelContainer->Size(), dataStore.data(), ...);

After (ReadImageUtils.hpp:38-46):

const std::size_t componentsPerPixel = itk::NumericTraits<PixelT>::GetLength();
const std::size_t totalElements = pixelContainer->Size() * componentsPerPixel;
std::transform(rawBufferPtr, rawBufferPtr + totalElements, dataStore.data(), ...);

The bug: ITK's PixelContainer::Size() returns the pixel count, not the scalar element count. For a multi-component pixel type (e.g. itk::Vector<uint8, 3> for RGB), the underlying buffer holds pixels * 3 scalars, but the old code only iterated over the first pixels of them. Every multi-component image read with type-conversion was getting only its first 1/N of scalars filled in; the rest of the destination DataStore was uninitialized.

The fix: multiply by itk::NumericTraits<PixelT>::GetLength(), which is ITK's portable way to ask "how many scalar components in one pixel of this type?" This trait is defined for all of ITK's pixel types, so it's correct for:

• scalar types (uint8, float, …) — GetLength() == 1, multiply by 1, behavior unchanged.
• itk::Vector<T, N> — GetLength() == N. This is the type the ReadImageByPixelType ladder dispatches to (ReadImageUtils.hpp:407-422 switches numComponents into itk::Vector<T, 1|2|3|4|36>). The bug actually manifested for N>1 cases here.
• itk::RGBPixel<T> — GetLength() == 3. Not used by this dispatch ladder in the current code (the ladder uses itk::Vector for components 3 and 4), but the fix is correct if someone wires it up.
• itk::CovariantVector<T, N> — GetLength() == N. Same.

Verified by grep: ReadImageUtils.hpp:409-421 uses itk::Vector<T, 1|2|3|4|36> exclusively in the dispatch path, which is exactly the case the bug fix targets. The ITKArrayHelper.hpp paths that do mention RGBPixel/RGBAPixel/CovariantVector go through a different code path (ITK::ConvertImageToDataStore rather than the type-converting ConvertImageToDataStoreAsType), and that path is unchanged here.

The accompanying enum consolidation in the same file — replacing the local cxItkImageReaderFilter::OriginSpacingProcessingTiming with nx::core::OriginSpacingProcessing (ReadImageUtils.hpp:8, ReadImageUtils.cpp:83,159, ITKImageReaderFilter.cpp:147,182,199-201, ITKImportImageStackFilter.cpp various) — is a parallel cleanup so both ITK and non-ITK readers share the same enum. Good incidental dedup.

[Validated]. This is the correct fix, in the right place, and the comment explaining why (ReadImageUtils.hpp:34-37) is exactly the kind of comment that prevents the bug from being reintroduced. The fix is only effective for the type-converting code path (ConvertImageToDataStoreAsType); the same-type path uses ITK's ITK::ConvertImageToDataStore which already handles multi-component correctly. Worth a small follow-up to verify with a focused unit test (e.g. read a 3-channel RGB TIFF with type conversion to uint16 and assert that all 3 channels of every pixel are populated, not just the first 1/3).

Architecture verdict

Ship it. The architectural shape is right:

• The new IImageIO layer is correctly scoped (small interface, lives in simplnx/Utilities, knows nothing about DataStructure), uses a sensible factory dispatch, and supports the OOC discipline through byte-buffer + std::span.
• The Filter / Algorithm split is correctly applied for the simple filters and explicitly bent (with documentation in the ReadImageStackInputValues/ReadImageSubFilterConfig design) for the composite filter, which is the right call.
• The TiffOpenOptions per-handle error capture is the only thread-safe way to consume libtiff and the non-movable RAII enforcement of pointer stability is the textbook implementation.
• The drive-by ITK fix closes a real correctness bug with a minimal, targeted change and leaves the surrounding code measurably cleaner via enum consolidation.

The caveats — sub-filter coupling in ReadImageStack, double preflight per slice, the grayscale rename dance, "first error vs last error" in TiffOpenOptions, and discoverability of new vs ITK filters via tags — are all minor follow-ups, not redesigns. None of them changes the shape of the code that should land. They are fix-as-you-touch items, not blockers.

What I would not redesign:

• Don't switch the IO interface to templates / variants — the byte-buffer ABI is the entire point.
• Don't fold the new filters into ITKImageProcessing — the dependency-elimination is half the value of this PR.
• Don't inline the resample/grayscale logic into ReadImageStack — composition is correct.

What I'd add as a follow-up issue (not blocking):

• Either deprecate the ITK readers for the formats the new path covers, or add a "Use this for PNG/JPEG/BMP/TIFF; use ITK for everything else" note to both filters' documentation. Right now both pairs are equally findable by tag and a user has no signal which one to pick.
• A focused regression test for the ITK multi-component fix (3-channel RGB TIFF with type conversion).
• An IReadImageOptions / scanline-streaming extension to IImageIO for very large TIFFs, only if profiling shows the slice buffer matters.

---

Posted by an automated reviewer agent (Opus 4.7) — use as input, not gospel.

[imikejackson]

Acted on review-agent feedback in 3d9ae56

Going through the items the two review agents flagged. Fixed in this commit:

Correctness (top-3-blocker items from the cross-cutting review):

• ReadImage.cpp ConvertPixelDataFunctor<float32> was dividing by numeric_limits<float32>::max() (~3.4e38) — every HDR float input normalized to ~0 and the converted image was black. Switched to a [0, 1] saturation range for floating-point types (matches stb's HDR convention) with explicit std::clamp so HDR floats > 1.0 saturate cleanly instead of wrapping the destination's range.
• ReadImage.cpp physical-cropping now rejects out-of-bounds X/Y minima up front with a meaningful error rather than producing a garbage xStart and falling into the generic "crop window does not fit" message.
• ReadImageStack.cpp physical Z-crop: ImageGeom::getIndex returning nullopt is now a hard error instead of silently leaving the full slice range. Added a sanity check on the computed [startSlice, endSlice] range.
• TiffImageIO.cpp now treats TIFFScanlineSize <= 0 as an error before the cast-to-usize would have allocated ~16 EiB.

Robustness:

• ReadImageStackFilter.cpp: the four dynamic_cast<…>(actions.at(0|1).get()) lookups against sub-filter outputActions are replaced with a FindFirstActionOfType<T> helper. Sub-filters can now reorder actions (or insert new ones) without silently breaking the stack filter at runtime.
• ImageIOFactory.cpp / StbImageIO.cpp: ::tolower replaced with an unsigned-char lambda — the original is UB on platforms with signed char whenever the high bit is set.

Cosmetic / consistency:

• TiffImageIO.cpp error-code constants now use the simplnx int32 alias to match StbImageIO.cpp.
• WriteImage.cpp dropped the dead std::fill on sliceBuffer — the extract loop overwrites every byte of the slice plane.
• ReadImageStack.cpp tempDataStore variables in FlipAbout{X,Y}Axis renamed to dataStoreRef (the variable was a live store reference, not a temp).
• ImageIOEnums.hpp doc comment no longer claims reinterpret_cast<> from ChoicesParameter::ValueType is valid — only static_cast<> from the underlying integral type is well-defined.

Discoverability (from the design review):

• ITKImageReaderFilter.cpp dropped the per-format tags ("jpg", "tiff", "bmp", "png") so the new SimplnxCore ReadImageFilter wins format-specific searches in the filter palette. The ITK reader is still findable via "io" / "image" for the formats stb/libtiff don't cover.

Deferred to follow-up issues (intentionally not in this PR):

• GetDataTypeSize and the ImageIOUtilities choice helpers throwing std::runtime_error rather than returning Result<T>. GetDataTypeSize mirrors the established GetNumericTypeSize codebase convention; the choice helpers are reachable only via programming bugs in practice (the ChoicesParameter validates the index range upstream). Worth a separate audit-pass that touches both old and new helpers consistently.
• Per-slice sub-filter construction in ReadImageStack (perf, not correctness).
• Tiled-TIFF std::vector<uint32_t>(width*height) upfront-allocation cap (TIFFReadRGBAStrip would chunk it).
• In-core memcpy fast path on the CopyPixelDataFunctor / ExtractSliceFunctor virtual operator[] loops — depends on the planned copyIntoBuffer/copyFromBuffer API on AbstractDataStore.
• WriteImageFilter ExtensionListType{} populated from the same source as CreateImageIO.
• "First error vs last error" semantics for TiffOpenOptions.

42/42 image-IO tests pass on NX-Com-Qt69-Vtk95-Rel.