Skip to content

Performance comparison of iterators and ranges to copy vector images #5669

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

Closed
blowekamp wants to merge 2 commits into from
+505 −1
Closed

Performance comparison of iterators and ranges to copy vector images #5669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
903ca57
WIP: Comparing performance in CastImageFilter
blowekamp Dec 1, 2025
a720403
WIP: Add performance test to compare copy methods
blowekamp Dec 1, 2025
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
32 changes: 31 additions & 1 deletion Modules/Filtering/ImageFilterBase/include/itkCastImageFilter.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

#include "itkProgressReporter.h"
#include "itkImageAlgorithm.h"
#include "itkImageRegionRange.h"

namespace itk
{
Expand Down Expand Up @@ -139,18 +140,21 @@ CastImageFilter<TInputImage, TOutputImage>::DynamicThreadedGenerateDataDispatche

this->CallCopyOutputRegionToInputRegion(inputRegionForThread, outputRegionForThread);

const unsigned int componentsPerPixel = outputPtr->GetNumberOfComponentsPerPixel();

#if 0
Copy link
Contributor

@N-Dekker N-Dekker Dec 3, 2025

Choose a reason for hiding this comment

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

I guess the #if 0 code may be removed when this PR is ready, right? Otherwise if constexpr (false) might be nicer... 😺 But I would prefer the code to just be removed, eventually.

Copy link
Member Author

@blowekamp blowekamp Dec 3, 2025

Choose a reason for hiding this comment

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

I made PR #5670 with just the change to the cast filter.

This change below fails the testing due to the aliasing/reference of the first outputPixel, just like #5668. I'll update that issue with the Image Range iterator to hopefully focus the discussion of what the correct behavior should be.

// Define the iterators
ImageScanlineConstIterator inputIt(inputPtr, inputRegionForThread);
ImageScanlineIterator outputIt(outputPtr, outputRegionForThread);


const unsigned int componentsPerPixel = inputPtr->GetNumberOfComponentsPerPixel();
while (!inputIt.IsAtEnd())
{
while (!inputIt.IsAtEndOfLine())
{
const InputPixelType & inputPixel = inputIt.Get();
OutputPixelType value{ outputIt.Get() };

for (unsigned int k = 0; k < componentsPerPixel; ++k)
{
value[k] = static_cast<typename OutputPixelType::ValueType>(inputPixel[k]);
Expand All @@ -163,6 +167,32 @@ CastImageFilter<TInputImage, TOutputImage>::DynamicThreadedGenerateDataDispatche
inputIt.NextLine();
outputIt.NextLine();
}
#else

auto inputRange = ImageRegionRange<const TInputImage>(*inputPtr, inputRegionForThread);
auto outputRange = ImageRegionRange<TOutputImage>(*outputPtr, outputRegionForThread);
Comment on lines +172 to +173
Copy link
Contributor

@N-Dekker N-Dekker Dec 3, 2025

Choose a reason for hiding this comment

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

Nitpick: technically this is OK, of course, but in ITK we don't really do AAA ("auto almost always") everywhere. (We just follow clang-tidy modernize-use-auto, but that's still not AAA everywhere). So I would just do:

    ImageRegionRange<const TInputImage> inputRange(*inputPtr, inputRegionForThread);
    ImageRegionRange<TOutputImage>      outputRange(*outputPtr, outputRegionForThread);


auto inputIt = inputRange.begin();
auto outputIt = outputRange.begin();
const auto inputEnd = inputRange.end();


OutputPixelType outputPixel{ *outputIt };
Copy link
Contributor

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

Choose a reason for hiding this comment

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

At this point, the value of *outputIt may not yet be initialized, right? The output pixel values may still be uninitialized at this point. If so, better just declare OutputPixelType outputPixel; without retrieving the value from *outputIt.

Copy link
Member Author

@blowekamp blowekamp Dec 2, 2025

Choose a reason for hiding this comment

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

You are correct that the values in outputPixel are uninitialized. However, for the VectorImage cast, the outputPixel will point to the data in the output Image's buffer. So this removed the need to allocation memory, reduced the memory locations used ( atleast for variable length vectors. )

Copy link
Member Author

@blowekamp blowekamp Dec 2, 2025

Choose a reason for hiding this comment

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

This is also the "BUG" cast where the reference to the first pixel is maintained and will be assigned all values in the range.

Copy link

@SimonRit SimonRit Dec 2, 2025

Choose a reason for hiding this comment

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

Are you implying that #5661 didn't fix a bug? To be honest, the time it took me to understand the origin of this bug is a clear indication to me that this is not the expected behavior of the Get function of an iterator. And I guess this is not consistent with how it behaves with scalar pixel.

Copy link
Member Author

@blowekamp blowekamp Dec 2, 2025

Choose a reason for hiding this comment

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

The merged patch fixed the issue in the case filter. But this new code block with the ImageRangeIterator exhibits the same behavior as the original CastImageFilter.

Some tricks have been needed to get operations on the VectorImageFilter to perform reasonably. But they have been this way for over 10 years, and crippling filters by making them run 10x slower may not be a reasonable option.

Copy link

@SimonRit SimonRit Dec 2, 2025

Choose a reason for hiding this comment

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

I don't think it's reasonable too and I don't have the solution. But it does not mean it's not a bug. BTW, if you implement the (slow) version, a recursive Gaussian test fails which probably indicates a bug there too.

const unsigned int componentsPerPixel = NumericTraits<OutputPixelType>::GetLength(outputPixel);
while (inputIt != inputEnd)
{
InputPixelType inputPixel = *inputIt;
Copy link
Contributor

@N-Dekker N-Dekker Dec 2, 2025

Choose a reason for hiding this comment

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

Why not const InputPixelType & inputPixel? (The original code also has const InputPixelType & inputPixel.)

Copy link
Member Author

@blowekamp blowekamp Dec 2, 2025

Choose a reason for hiding this comment

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

Yes. You reference code times better on my system.

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

++inputIt;
++outputIt;
}

#endif
}

} // end namespace itk
Expand Down
8 changes: 8 additions & 0 deletions Modules/Filtering/ImageFilterBase/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ set(
itkVectorNeighborhoodOperatorImageFilterTest.cxx
itkMaskNeighborhoodOperatorImageFilterTest.cxx
itkCastImageFilterTest.cxx
itkImageIterationPerformanceTest.cxx
)

# Disable optimization on the tests below to avoid possible
Expand Down Expand Up @@ -59,6 +60,13 @@ itk_add_test(
ITKImageFilterBaseTestDriver
itkCastImageFilterTest
)
itk_add_test(
NAME itkImageIterationPerformanceTest
COMMAND
ITKImageFilterBaseTestDriver
itkImageIterationPerformanceTest
256
)

set(ITKImageFilterBaseGTests itkGeneratorImageFilterGTest.cxx)
creategoogletestdriver(ITKImageFilterBase "${ITKImageFilterBase-Test_LIBRARIES}" "${ITKImageFilterBaseGTests}")
50 changes: 50 additions & 0 deletions Modules/Filtering/ImageFilterBase/test/itkCastImageFilterTest.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include "itkFloatingPointExceptions.h"
#include "itkRGBPixel.h"
#include "itkRGBAPixel.h"
#include "itkTimeProbe.h"

namespace itk
{}
Expand Down Expand Up @@ -444,6 +445,47 @@ TestVectorImageCast3()
}


template <typename TInputImage, typename TOutputImage>
void
TimeImageCast()
{
std::cout << "Timing cast from " << GetCastTypeName<typename TInputImage::PixelType>() << " to "
<< GetCastTypeName<typename TOutputImage::PixelType>() << " ... ";

// Create a 512^3 image
auto image = TInputImage::New();

typename TInputImage::SizeType size;
size.Fill(512);
typename TInputImage::RegionType region{ size };
image->SetRegions(region);
image->SetNumberOfComponentsPerPixel(3);
image->AllocateInitialized();


using CastImageFilterType = itk::CastImageFilter<TInputImage, TOutputImage>;
auto castImageFilter = CastImageFilterType::New();
castImageFilter->SetInput(image);
castImageFilter->SetNumberOfWorkUnits(1); // Single-threaded for consistent timing

// Warm-up run
castImageFilter->Update();

// Timed run
itk::TimeProbe timer;
timer.Start();
castImageFilter->Modified();
castImageFilter->Update();
timer.Stop();

const double totalPixels = size.CalculateProductOfElements();
const double timeInSeconds = timer.GetMean();
const double megapixelsPerSecond = (totalPixels / 1e6) / timeInSeconds;

std::cout << megapixelsPerSecond << " Mpixels/sec (" << timeInSeconds << " seconds)" << std::endl;
}


int
itkCastImageFilterTest(int, char *[])
{
Expand Down Expand Up @@ -477,6 +519,14 @@ itkCastImageFilterTest(int, char *[])
success &= TestVectorImageCast2();
success &= TestVectorImageCast3();


std::cout << std::endl;
std::cout << "Performance timing tests (512^3 images):" << std::endl;
TimeImageCast<itk::Image<itk::Vector<float, 3>, 3>, itk::Image<itk::RGBPixel<double>, 3>>();
TimeImageCast<itk::Image<itk::Vector<float, 3>, 3>, itk::VectorImage<double, 3>>();

TimeImageCast<itk::VectorImage<float, 3>, itk::Image<itk::RGBPixel<double>, 3>>();

std::cout << std::endl;
if (!success)
{
Expand Down
Loading
Loading