ImageRange<TImage> supports iteration over all pixels of an image buffer by using a C++11 range-based for-loop (for(auto&& pixel: imageRange) ...) or by using a Standard C++ Library (STL) algorithm like std::for_each. Moreover, it also supports random access (pixel = imageRange[i]).
Possible next steps (not necessarily in that order):
Use ImageRange in existing or new ITK filters, whenever it could yield a significant performance improvement. I’m now looking at for-loops like this one in N4BiasFieldCorrectionImageFilter, which iterates over the entire buffered region, and appears to do a lot of (avoidable?) index computation, by calling itk::Image::GetPixel(const itk::Index<N>&) every time:
I think the performance of the code above here could be improved by using ImageRange.
Add begin(), end(), and other range member functions to itk::Image and itk::VectorImage, which could just call the corresponding ImageRange member functions. This would allow having a range-based for-loop directly on an ITK image, which seems more convenient to me: for(auto&& pixel: image) ...
itk::ImageRegion support: allow iteration over the pixels of a certain region within the image. @matt.mccormick asked for this feature already. Something like this (I guess):
ImageRegionRange imageRegionRange{ image, region };
for (auto&& pixel: imageRegionRange )
// Process each pixel in the image region here!
I have to admit I find it a bit challenging to add such region support to the current ImageRange<TImage> class, without loosing any performance on “whole image buffer iteration”. To be continued…
In the mean time, I had a look at the performance, using ImageRange to copy the pixel data of an itk::Image, and compared it with the good old itk::ImageAlgorithm::Copy (using VS2017 Release):
The ImageAlgorithm::Copy is a good implementation. It was implemented in ITKv4, and it made several filters go nearly 100x faster than the prior implementation. It implements copying arbitrary regions in the images, and takes care to compute the maximum continuous regions between the image, and then calls std::transform or std::copy in a the appropriate chunks. This method was implemented based on the needs and features required for working with itk::Images and their multiple regions (BufferedRegions, RequestedRegion, LargestPossibleRegions). It is used in filters like the PasteImageFilter, SliceBySliceImageFilter, PadImageFilter etc… These filters are in the Grid group, so they move chunks around. Removing most of the features and then making a comparison biases the evaluation.
Are there any problems with the ImageAlgorithm::Copy that motivate an update?
Honestly I’m impressed about itk::ImageAlgorithm::Copy. I think it’s a very clever piece of code! Having seen how well it performs, I’m certainly not suggesting to generally replace existing ImageAlgorithm::Copy calls by std::copy calls on ImageRange iterators.
In general, I would only suggest to convert existing ITK filters to start using ImageRange, when that would yield a significant performance gain. Or when it would yield an obvious style improvement. But only if it would perform at least as good as the original code, of course!
I like the new interface of the ImageRange iterator as it behaves as expected like a std C++ iterator. This is a very good thing and hope to see more of it. However, it became apparent yesterday, during the N4 review, the implications of only iterating over the BufferedRegion and how that interacts with the ITK pipeline.
The fact that the pipeline provides no guarantee that the BufferedRegion(s) is equal to the RequestedRegion(s) for a filter’s inputs is quite significant and needs to be fully considered. The implications are that the iterator is not suitable for operating on the input images, in general, because of the lack of guarantee we don’t know what the BufferedRegion is after a parent filter is executed. There are some cases where these implicit assumptions might be appropirate… but that requires further thought.
It would be prudent to add a required argument to the ImageRange, the Region which it iterates on. This would make the current implicit assumption explicit. The exemplar use case would be to just pass the image’s BufferedRegion. Logic should be added so that if the Region parameter does not match the BufferedRegion than an exception would be thrown. This would simplify the N4 filter, up for review, by removing the checks ( they would be now part for the ImageRange constructor), and make the region operated on explicit. For old school ITK developers this is the expected usage, and it makes sense to know the region we are iterating over.
Thanks for your ideas on ImageRange, @blowekamp. However, I’m not yet convinced that there should be such region checking and exception throwing within the ImageRange(Image&) constructor. It’s an essential property of ImageRange that it iterates over the entire image buffer. (If necessary, we could rename the class to something like “ImageBufferPixelRange” or “BufferedImageRange”.) Note that ImageRange may also be used outside of the context of an ITK pipeline. For example, when initializing an itk::Image, before its first use.
I’m considering to also add an image region specific class to iterate over pixels. However, such an “ImageRegionRange” would not be as fast as ImageRange.
It seems alright to me that a filter itself explicitly checks the regions or sizes of its input images. It does not seem necessary to me to have these precondition checks “hidden” within the ImageRange objects used by the filter. Right?
The use / assumptions / contract of itk::Image's RequestedRegion and BufferedRegion are that
Once is comes time to process the RequestedRegion, it may be smaller than the BufferedRegion. This is common when an input image cannot stream or the filter producing an input image needed to extend the input’s BufferedRegion.
The BufferedRegion fully contains the RequestedRegion.
As a consequence, the ImageRange class or an alternative class should support operating on a RequestedRegion that is smaller than the BufferedRegion. Also, it is not necessary to check that the RequestedRegion is inside the BufferedRegion when iterating over it. The filters make sure that this is true, and if an image is constructed outside of a filter, the author is required to ensure this is true, just like they are required to call Allocate() if necessary. Excessive checks that hurt performance or make the API more confusing should be avoided.
BufferedImageRange<TImage>: the fastest way to iterate over the entire buffer of an itk::Image (as currently implemented by Experimental::ImageRange<TImage>)
RequestedImageRange<TImage>: iterating within the RequestedRegion of the specified image.
They could be constructed the same way, just passing an image as parameter:
Clearly, the BufferedImageRange would use the BufferedRegion, and the RequestedImageRange would use the RequestedRegion of the image. Do you agree? Or do you think it would still be useful to add a region parameter? Is it useful to support specifying an arbitrary region, that is not one of these two?
There is a check in the base ImageConstIterator class that ensures the region to iterate on is contained in the BufferedRegion. This is a simple check that is usually done once during construction, and not per-pixel, so it has negligible impact on performance. This checks has been very useful which saves time and effort in writing, running and debugging pipelines or filters. The ImageRange or ImageBufferedRegionRange needs a similar check for the expected region being equal to the buffered.
Unexpected issues happened when there are multiple input and/or image grid and region manipulation explicitly stating the region being iterated on and checking that will occur will benefit both developers of filters and pipelines as well as when the ImageRange is use directly. It will help the class be easy to use and have wider adoption, while improving the reliability of the toolkit.
Currently, you specify the region an iterator works on with the conventional ImageRegionIterators. This works quite well both in the pipeline and outside. Only being able to iterate on requested or buffered regions will not generally work inside the pipelines, and would miss the flexibility needed to be widely used.
I understand that the current ImageRange was implemented to be very efficient and it has achieved that goal inpart by only being able to iterated on the BufferedRegion. This class needs to be:
renamed to something like ImageBufferedRegionRange
an argument added to the constructor to ensure the expected region is actually the buffered region being iterated on so that it is safe and clear what is being iterated over.
There is also a need for a general ImageRegionRange iterator to operate on an arbitrary region.
Thank you guys, for discussing this issue so extensively.
In my opinion, this specific range should just provide an elegant and generic interface to the pixel buffer as a whole:
Offering access though the external PixelType of the image, rather than the InternalPixelType
Supporting the modern C++ range interface
I think there should be another image range type that specifically iterates within a specified region (working title “ImageRegionRange”). I think I can submit a WIP version quite soon.
And yes, I would like rename this ImageRange, to ImageBufferRange. Would you like that too?
When using multiple ImageRange is make since to specify what region we are intending to iterate on, and check that they are consistent with the BufferedRegion.
Thanks, @blowekamp I understand your intention. Although personally I still think this region checking could be done more explicitly within the filter itself, before creating the ranges. Anyway, it’s clear how you would like to have it.
But then, I would like to support other use cases as well, with just a single image. For example, like this:
Internally, ImageBufferRange would then just check if image->GetBufferedRegion() == image->GetBufferedRegion(), for one and the same image. That would not make any sense to me.
As a compromise, could it be acceptable to you to have an optional rather than an obligatoryImageRegion parameter?
auto itkImage = this->GetImage();
for( auto && pixel: MakeImageBufferRange(itkImage, itkImage->GetBufferedRegion()))
...
That looks like the right amount of verbosity to me. ITK has traditionally been over verbose or long in the required typedefs, variable names etc. The burden of writing is only once, but it is read likely hundreds of time so a little verbosity to make what the region being operated on clear ( to those who may not have seen this new iterative before) is well worth the effort.
I’m sorry Bradley, but I’m not convinced. Internally, MakeImageBufferRange(itkImage, itkImage->GetBufferedRegion()) would just check if itkImage->GetBufferedRegion() is equal to itself (!), which looks completely unnecessary to me.
Moreover, it would be a pity to me if MakeImageBufferRange would no longer support passing a nullptr as Image-pointer argument of MakeImageBufferRange. Currently MakeImageBufferRange returns an empty range, in that case. For example at itkN4BiasFieldCorrectionImageFilter.hxx#L278:
const auto maskImageBufferRange = MakeImageBufferRange(this->GetMaskImage());
In this case, this->GetMaskImage() may return nullptr, and if that’s the case, you cannot easily pass maskImage->GetBufferedRegion() as second argument.
Would you like it if I rename the current MakeImageBufferRange(TImage*) to something like “MakeImageBufferRangeForBufferedRegion(TImage*)”? Would that make things clearer to you (or anyone else)?
Of course you may still add another (an extra) Make function that does perform BufferedRegion checking… what do you think?
piece of code! Having seen how well it performs, I’m certainly not suggesting to generally replace existing