BlueQuartzSoftware / BlueQuartzSoftware/simplnx
Image IO follow-ups deferred from PR #1585
- Dominant language
- C++
- Stars
- 17
- Forks
- 13
- Avg merge
- 3d 4h
- Merged PRs (30d)
- 10
Description
PR #1585 added a new format-agnostic image-IO subsystem (`IImageIO` + `StbImageIO` / `TiffImageIO` backends) and three new SimplnxCore filters (`ReadImageFilter`, `WriteImageFilter`, `ReadImageStackFilter`). Two automated review passes (cross-cutting code quality + design validation) flagged a number of items that were intentionally deferred from that PR to keep the diff focused and to land the merge sooner. This issue tracks them.
Each item links back to the relevant comment thread on #1585 for the original rationale.
### Correctness / API discipline
- [ ] **Replace `throw std::runtime_error` in `GetDataTypeSize` and the `ImageIOUtilities` choice helpers with `Result`.**
`simplnx/Common/TypesUtility.hpp` (`GetDataTypeSize`) and `simplnx/Utilities/ImageIO/ImageIOUtilities.hpp` (`ChoiceToImageDataType`, `ImageDataTypeToChoice`) currently throw on unexpected enum values. The codebase convention is `Result` / `MakeErrorResult`. The throws are unreachable in practice today (callers come from validated `ChoicesParameter` indexes / known `DataType` enum values), but a future caller from an unvalidated path would crash any pipeline runner that does not catch. Should be done as a coordinated pass that also touches the existing `GetNumericTypeSize` for consistency.
- [ ] **`TiffOpenOptions` first-error vs last-error semantics.**
`simplnx/Utilities/ImageIO/TiffImageIO.cpp` — when libtiff produces multiple errors during a single operation, the per-handle error handler currently keeps only the last one. The first error is usually the most diagnostic ("unknown tag 700" → "cannot read scanline N" — the first one is the actual cause). Change to first-write-wins.
- [ ] **Add a focused regression test for the ITK multi-component `ConvertImageToDataStoreAsType` fix.**
PR #1585 fixed a bug in `ITKImageProcessing/Common/ReadImageUtils.hpp` where multi-component pixel images were getting only their first 1/N scalars filled in (`pixelContainer->Size()` returns the pixel count, not the scalar element count). Add a test that reads a 3-channel RGB TIFF with type conversion to `uint16` and asserts every channel of every pixel is populated, not just the first 1/3.
### Performance
- [ ] **In-core `memcpy` fast path on `CopyPixelDataFunctor` / `ExtractSliceFunctor`.**
`Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/ReadImage.cpp` and `WriteImage.cpp` currently walk every pixel through `AbstractDataStore::operator[]` / `setValue` — a virtual call per scalar component. For a 4096×4096 RGBA image that is ~67M virtual calls per slice. When (a) no cropping, (b) `srcType == destType`, and (c) the destination is in-core, the whole row (or the whole image) can be a single `std::memcpy`. Depends on the planned bulk `copyIntoBuffer` / `copyFromBuffer` API on `AbstractDataStore` — file a separate issue for that API if it doesn't already exist.
- [ ] **Tiled-TIFF chunked read.**
`simplnx/Utilities/ImageIO/TiffImageIO.cpp` `readPixelData` falls back to `TIFFReadRGBAImageOriented` for tiled TIFFs and allocates `std::vector(width * height)` (1 GiB for a 16384² image). Switch to `TIFFReadRGBAStrip` / `TIFFReadRGBATile` to read in chunks. At minimum, reject upfront with a clear error if the implied scratch buffer exceeds some threshold rather than failing inside `std::vector::vector` with `std::bad_alloc`.
- [ ] **Per-slice sub-filter construction in `ReadImageStack`.**
`Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/ReadImageStack.cpp` constructs a fresh `ReadImageFilter`, `Arguments`, and scratch `DataStructure` per slice (and another `ResampleImageGeomFilter` / `ConvertColorToGrayScaleFilter` per slice when those modes are on). For a 1000-slice stack that is 1000 sub-filter setups. Hoist the `Arguments` outside the slice loop and only `insertOrAssign` the per-slice file path.
- [ ] **Skip per-slice double-preflight in `ReadImageStack`.**
`ReadImageFilter::execute` re-runs preflight, which itself opens the file to read metadata. Each slice file is therefore opened twice. Consider an algorithm-level entry point (`ReadImage::operator()` directly) the stack code can use without going through the filter wrapper.
- [ ] **`FlipAboutXAxis` row-swap via `std::swap_ranges`.**
`Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/ReadImageStack.cpp` does element-by-element scalar swap with `getValue` / `operator[]`. `FlipAboutYAxis` already uses iterator-based row buffer copies; `FlipAboutXAxis` should swap whole rows via `std::swap_ranges` on the iterators. Same algorithmic complexity, real constant-factor savings.
- [ ] **Throttled progress messaging in slice loops.**
Both `Algorithms/ReadImageStack.cpp` and `Algorithms/WriteImage.cpp` `fmt::format` a per-slice "Importing/Writing X/Y" message every iteration. For thousands of slices this is wasted work behind a probably-no-op handler. Use `ThrottledMessenger` per the `progress-messaging` skill.
- [ ] **Streaming `ReadImage` instead of whole-image scratch buffer.**
`Algorithms/ReadImage.cpp` allocates `std::vector tempBuffer` of *one image's* worth of bytes — bounded for typical inputs but a 65535×65535 RGBA float32 image is 64 GiB. Add a scanline-streaming code path that materializes one row at a time when the source format supports it (libtiff scanline reads do).
- [ ] **`IImageIO::beginStream() / writeRow() / endStream()` extension.**
`Algorithms/WriteImage.cpp` per-slice scratch buffer is correct and bounded today, but a streaming write API would let TIFF and other row-oriented formats skip the intermediate slice buffer entirely. Only worth doing if profiling shows the slice buffer matters.
### Architecture / API
- [ ] **Format registry instead of an `if/else` chain in `CreateImageIO`.**
`simplnx/Utilities/ImageIO/ImageIOFactory.cpp` hard-codes the supported extension list inside an `if/else`. Same list is duplicated in `WriteImageFilter::parameters()` (currently `ExtensionListType{}` — empty). Add a `static const std::map()>>` plus a `GetSupportedReadExtensions()` / `GetSupportedWriteExtensions()` helper that the filter parameter UI can call. Important once the third backend lands.
- [ ] **Populate `WriteImageFilter` extension dropdown from the same source as `CreateImageIO`.**
`Plugins/SimplnxCore/src/SimplnxCore/Filters/WriteImageFilter.cpp` declares `ExtensionListType{}` (empty) for the output `FileSystemPathParameter` — the user can type any extension and only `preflightImpl` validates. Tied to the previous registry item.
- [ ] **Drop `SIMPLNXCORE_EXPORT` from `BuildReadImageFilterArgs`.**
`Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/ReadImageStack.hpp` exports a sub-filter argument helper across the DLL boundary. If only the algorithms call it, drop the export and put it in an anonymous namespace.
- [ ] **Decouple `ReadImageStack` algorithm from `ReadImageFilter` header.**
`Algorithms/ReadImageStack.cpp` currently includes `SimplnxCore/Filters/ReadImageFilter.hpp` to construct a `ReadImageFilter` per slice. If we expose an algorithm-level entry point on `ReadImage` (see the double-preflight item above), the algorithm file can depend only on data + IO and not on a sibling filter header.
- [ ] **`ConvertColorToGrayScaleFilter` in-place mode.**
`Algorithms/ReadImageStack.cpp` does an awkward "convert to grayscale → get array named `gray` → delete the original → rename the gray one back" dance because `ConvertColorToGrayScaleFilter` does not support in-place. Long-term fix is on the grayscale filter.
### Defensive
- [ ] **`WriteImageFilter::preflightImpl` should validate `totalDigits >= 1`.**
Negative or zero `totalDigits` is currently passed straight to `CreateIndexString` which throws an `fmt::format` exception out of preflight. Bounds-check in preflight and `MakeErrorResult` instead.
- [ ] **`fs::create_directories` use `std::error_code` overload.**
`Algorithms/WriteImage.cpp` `fs::create_directories` failure currently produces a generic "Error creating output directory" message. The `std::error_code` overload would let us surface the OS-level reason.
- [ ] **Surface override conflicts in `ReadImageFilter::preflightImpl`.**
When the file specifies an origin/spacing and the user also overrides one in the filter, emit a `PreflightValue` warning ("file specifies origin X but you are overriding to Y") so the conflict is visible up front. Currently silent.
- [ ] **`TIFFGetField(XPOSITION/YPOSITION)` partial-presence.**
`simplnx/Utilities/ImageIO/TiffImageIO.cpp` `readMetadata` sets `hasOrigin = true` if either X or Y position tag is present; if only X is present, `yPosition` stays `0.0f` and is silently written into `metadata.origin[1]`. Either require both, or comment the partial-presence behavior.
### Documentation
- [ ] **Filter docs need a "use this filter for these formats" section.**
Both the new `ReadImageFilter` / `WriteImageFilter` / `ReadImageStackFilter` and the existing ITK-based equivalents are now equally findable in the filter palette (after PR #1585 dropped the per-format tags from the ITK reader). Add a "Use this for PNG/JPEG/BMP/TIFF; use ITK for everything else" note to both filter pairs' documentation so users know which to reach for first.
- [ ] **Replace placeholder doc comments on `ReadImageFilter.hpp` and `ReadImageStackFilter.hpp`.**
Both `/** @brief This filter will .... */` comments are placeholders. Replace with a one-sentence description of what the filter does.
### Source
- [Cross-cutting code review](https://github.com/BlueQuartzSoftware/simplnx/pull/1585#issuecomment-4336175054)
- [Design-validation review](https://github.com/BlueQuartzSoftware/simplnx/pull/1585#issuecomment-4336175823)
- [Round-3 fixes already landed in #1585](https://github.com/BlueQuartzSoftware/simplnx/pull/1585#issuecomment-4336311239)
Contributor guide
Assessment
This issue has not been assessed yet.