Skip to content

BUG: Avoid copying uninitialized pixels in CastImageFilter #5680

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thewtex merged 1 commit into from
Dec 11, 2025
Merged

BUG: Avoid copying uninitialized pixels in CastImageFilter #5680

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions Modules/Filtering/ImageFilterBase/include/itkCastImageFilter.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "itkProgressReporter.h"
#include "itkImageAlgorithm.h"
#include "itkImageRegionRange.h"
#include "itkVariableLengthVector.h"

namespace itk
{
Expand Down Expand Up @@ -156,12 +157,19 @@ CastImageFilter<TInputImage, TOutputImage>::DynamicThreadedGenerateDataDispatche
while (inputIt != inputEnd)
{
const InputPixelType & inputPixel = *inputIt;
OutputPixelType outputPixel{ *outputIt };

using OutputPixelValueType = typename OutputPixelType::ValueType;

constexpr bool isVariableLengthVector = std::is_same_v<OutputPixelType, VariableLengthVector<OutputPixelValueType>>;
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@N-Dekker N-Dekker Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks, that makes sense!


// If the output pixel type is a VariableLengthVector, it behaves as a "reference" to the internal data. Otherwise
// declare outputPixel as a reference, `OutputPixelType &`, to allow it to access the internal buffer directly.
std::conditional_t<isVariableLengthVector, OutputPixelType, OutputPixelType &> outputPixel{ *outputIt };
Comment on lines +163 to +167
Copy link
Contributor Author

@N-Dekker N-Dekker Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@N-Dekker N-Dekker Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@N-Dekker N-Dekker Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@blowekamp blowekamp Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@N-Dekker N-Dekker Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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." 🤷

Copy link
Contributor Author

@N-Dekker N-Dekker Dec 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷

Copy link
Member

@blowekamp blowekamp Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@N-Dekker N-Dekker Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷

Copy link
Contributor Author

@N-Dekker N-Dekker Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷

Copy link
Member

@blowekamp blowekamp Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


for (unsigned int k = 0; k < componentsPerPixel; ++k)
{
outputPixel[k] = static_cast<typename OutputPixelType::ValueType>(inputPixel[k]);
outputPixel[k] = static_cast<OutputPixelValueType>(inputPixel[k]);
}
*outputIt = outputPixel;
++inputIt;
++outputIt;
}
Expand Down
Loading