BUG: Avoid copying uninitialized pixels in CastImageFilter

[N-Dekker]

For a regular output pixel type like RGBPixel<T> and itk::Vector<T>, the expression OutputPixelType outputPixel{ *outputIt } had undefined behavior, within CastImageFilter::DynamicThreadedGenerateDataDispatched, because at that point, the pixel pointed to by outputIt was typically still uninitialized. For such output pixel types, outputPixel should be declared as a reference (OutputPixelType &) instead.

The original OutputPixelType outputPixel{ *outputIt } initialization is only OK when the output pixel type is a VariableLengthVector<T>.

[blowekamp]

Please add this "optimization" to the benchmark test to determine if it's worth the code.

[N-Dekker]

Please add this "optimization" to the benchmark test to determine if it's worth the code.

I can add this to the benchmark, but it's not an "optimization". It's really a bug fix! When v1 is an uninitialized itk::Vector, itk::Vector v2{ v1 } really is a bug. It has undefined behavior.

---

• For the record, the corresponding benchmark fix that I'm proposing is now at pull request BUG: Avoid copying uninitialized pixels in VectorIterationBenchmark ITKPerformanceBenchmarking#109

[N-Dekker]

Please check if this is the best criterion for choosing between:

1. OutputPixelType outputPixel{ *outputIt }; // Without C++ reference.
2. OutputPixelType & outputPixel{ *outputIt }; // With C++ reference.

Another possibility might be: check if TOutputImage is a VectorImage.

[N-Dekker]

I assume that CastImageFilter supports itk::VectorImage<T> and itk::Image<itk::Vector<T>>, but not itk::Image<itk::VariableLengthVector<T>>, right? A regular itk::Image whose (internal) pixel type is itk::VariableLengthVector seems strange to me, and it certainly isn't tested, for CastImageFilter.

[N-Dekker]

Not to mention itk::VectorImage<itk::VariableLengthVector<T>>... that would be very strange to me 😸

[blowekamp]

Given that point, it's certainly safer to check for VectorImage than the pixel type.

The case for the Vector pixel type having "undefined" behavior for this situation, I have been struggling with. I suppose if the output pixel type was something like a std::vector ( or a variable length vector), there would be a major problem with the pixel being a non-trivial object.

However, for these trivial object I am not sure/convinced that assigning ( and not observing) the indeterminate value is really undefined behavior.
https://en.cppreference.com/w/cpp/language/default_initialization.html#Read_from_an_indeterminate_byte

[N-Dekker]

Thanks for the link, @blowekamp Looks like itk::Vector<float> is not an "uninitialized-friendly type", so I guess the following is applicable here:

"If an evaluation produces an indeterminate value, the behavior is undefined." 🤷

[N-Dekker]

Sorry, I prefer the currently proposed:

std::conditional_t<isVariableLengthVector, OutputPixelType, OutputPixelType &> outputPixel{ *outputIt };

Rather than:

if constexpr (IsOutputVectorImage)
{
  OutputPixelType outputPixel{ *outputIt };
}
else
{
  OutputPixelType & outputPixel{ *outputIt };
}

I don't like too much code duplication. So I just stick with the current PR 🤷

[blowekamp]

Here at line 103 you proposed two differ loops:
https://github.com/InsightSoftwareConsortium/ITKPerformanceBenchmarking/pull/109/files

A similar things can be done with the Ranged iterator. The needs for the two loops and setting them are different, by as well just make two. We are conviently using the range for loop so it's rather small the duplication. Each loop can do it's one thing well.

[N-Dekker]

I am wondering if we could explicit enable the ImageIterator::Value method to work like the reference we are trying to get the following to work:

 (*outputIt)[k] = static_cast<OutputPixelValueType>(inputPixel[k]);

For VectorImage, *outputIt returns an instance of PixelProxy. For the end-user, PixelProxy only has an assignment operator to assign a pixel value to the proxy, and a conversion operator to retrieve the pixel value. Technically, we could manually still add other member functions from the pixel type to the proxy type, but I'm afraid it would cause feature creep. Moreover, it would be inefficient to call such member functions via the proxy class, instead of directly on the pixel type. Sorry 🤷

[N-Dekker]

Here at line 103 you proposed two differ loops:
https://github.com/InsightSoftwareConsortium/ITKPerformanceBenchmarking/pull/109/files

You know I felt very bad 😞 when I did so!

You see, the "if" in InsightSoftwareConsortium/ITKPerformanceBenchmarking#109 has a outputIt.Get() call at the beginning, whereas the "else" has outputIt.Set(value) at the end:

      if constexpr (isVariableLengthVector<OutputPixelType>)
      {
        OutputPixelType value(outputIt.Get());
        for (unsigned int k = 0; k < componentsPerPixel; ++k)
        {
          value[k] = static_cast<typename OutputPixelType::ValueType>(inputPixel[k]);
        }
      }
      else
      {
        OutputPixelType value;
        for (unsigned int k = 0; k < componentsPerPixel; ++k)
        {
          value[k] = static_cast<typename OutputPixelType::ValueType>(inputPixel[k]);
        }
        outputIt.Set(value);
      }

I did not know how to replace this if constexpr with a simple std::conditional_t 🤷

[blowekamp]

This does not contain this discussed ( and I thought agreed upon ) change to detect VectorImage instead of VariableLengthVector. This theoretical case of Image of VariableLengthVetor would not work. The optimization where the VariableLengthVector reference the vector image buffers is done here:

ITK/Modules/Core/Common/include/itkDefaultVectorPixelAccessor.h

Line 78 in 27244a0

Get(const InternalType & input, const SizeValueType offset) const

This would not apply with other Image types, and the check should be based on the image type not the pixel type.

[dzenanz]

Looks good on a glance.

[SimonRit]

Looks good to me if we do not change the (counter-intuitive) behavior of the VariableLengthVector copy constructor.

[Copilot]

Pull request overview

This PR fixes undefined behavior in CastImageFilter::DynamicThreadedGenerateDataDispatched where uninitialized output pixels were being copied. The fix uses template metaprogramming to conditionally create either a value (for VariableLengthVector) or a reference (for other pixel types) to directly modify the output buffer.

Key changes:

• Introduces compile-time detection of VariableLengthVector pixel types
• Uses std::conditional_t to create either a proxy value or a reference to output pixels
• Removes unnecessary assignment back to the output iterator since modifications are now done in-place
• Refactors the static cast to use a type alias for cleaner code

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The type VariableLengthVector is referenced in the template code but is not included or forward declared in this header file. While it may work when VectorImage is used (which transitively includes VariableLengthVector), it would be more robust to add a forward declaration at the top of the file or include the header directly. Consider adding template <typename TValue> class VariableLengthVector; in the itk namespace before the CastImageFilter class, or add #include "itkVariableLengthVector.h" to the includes.

[N-Dekker]

OK, thanks, that makes sense!