From eef74fc7ec2553321fbb605b242bc6117ad30237 Mon Sep 17 00:00:00 2001 From: "Hans J. Johnson" Date: Wed, 4 Nov 2026 12:40:14 -0600 Subject: [PATCH] ENH: Step iterator positions with integer arithmetic at buffer markers ImageSliceConstIteratorWithIndex and ImageLinearConstIteratorWithIndex hold a raw pointer to the current pixel. At the end-of-line and end-of-slice markers of a traversal that pointer lies outside the pixel buffer, where the C++ standard does not define pointer arithmetic. The markers are never dereferenced or compared. MovePosition() steps the pointer through its integer value instead, so no pointer arithmetic crosses the buffer boundary. The loop code generated by Clang is identical and GCC adds one instruction per pixel. GoogleTests check that both iterators visit every pixel of a region once in every direction and both orders. --- .../include/itkImageConstIteratorWithIndex.h | 11 ++ .../itkImageLinearConstIteratorWithIndex.h | 20 +-- .../itkImageLinearConstIteratorWithIndex.hxx | 6 +- .../itkImageSliceConstIteratorWithIndex.hxx | 31 +++-- Modules/Core/Common/test/CMakeLists.txt | 2 + .../itkImageLinearIteratorWithIndexGTest.cxx | 110 +++++++++++++++ .../itkImageSliceIteratorWithIndexGTest.cxx | 129 ++++++++++++++++++ 7 files changed, 283 insertions(+), 26 deletions(-) create mode 100644 Modules/Core/Common/test/itkImageLinearIteratorWithIndexGTest.cxx create mode 100644 Modules/Core/Common/test/itkImageSliceIteratorWithIndexGTest.cxx diff --git a/Modules/Core/Common/include/itkImageConstIteratorWithIndex.h b/Modules/Core/Common/include/itkImageConstIteratorWithIndex.h index 0375004e1a3..8889b4ed2d9 100644 --- a/Modules/Core/Common/include/itkImageConstIteratorWithIndex.h +++ b/Modules/Core/Common/include/itkImageConstIteratorWithIndex.h @@ -20,6 +20,8 @@ #include "itkIndex.h" #include "itkImage.h" + +#include #include #include // For remove_const_t. @@ -286,6 +288,15 @@ class ITK_TEMPLATE_EXPORT ImageConstIteratorWithIndex } protected: // made protected so other iterators can access + /** Moves the position by `n` pixels with integer arithmetic, so that a marker past the buffer is not formed by + * pointer arithmetic; the position is dereferenced only at a pixel of the buffer. */ + void + MovePosition(const OffsetValueType n) + { + m_Position = reinterpret_cast( + reinterpret_cast(m_Position) + static_cast(n) * sizeof(InternalPixelType)); + } + typename TImage::ConstWeakPointer m_Image{}; IndexType m_PositionIndex{ { 0 } }; // Index where we currently are diff --git a/Modules/Core/Common/include/itkImageLinearConstIteratorWithIndex.h b/Modules/Core/Common/include/itkImageLinearConstIteratorWithIndex.h index dfe049b12d6..5722542a80f 100644 --- a/Modules/Core/Common/include/itkImageLinearConstIteratorWithIndex.h +++ b/Modules/Core/Common/include/itkImageLinearConstIteratorWithIndex.h @@ -216,7 +216,7 @@ class ITK_TEMPLATE_EXPORT ImageLinearConstIteratorWithIndex : public ImageConstI operator++() { this->m_PositionIndex[m_Direction]++; - this->m_Position += m_Jump; + this->MovePosition(m_Jump); return *this; } @@ -226,7 +226,7 @@ class ITK_TEMPLATE_EXPORT ImageLinearConstIteratorWithIndex : public ImageConstI operator--() { this->m_PositionIndex[m_Direction]--; - this->m_Position -= m_Jump; + this->MovePosition(-m_Jump); return *this; } @@ -248,8 +248,8 @@ template inline void ImageLinearConstIteratorWithIndex::NextLine() { - this->m_Position -= - this->m_OffsetTable[m_Direction] * (this->m_PositionIndex[m_Direction] - this->m_BeginIndex[m_Direction]); + this->MovePosition(-this->m_OffsetTable[m_Direction] * + (this->m_PositionIndex[m_Direction] - this->m_BeginIndex[m_Direction])); this->m_PositionIndex[m_Direction] = this->m_BeginIndex[m_Direction]; @@ -265,12 +265,12 @@ ImageLinearConstIteratorWithIndex::NextLine() this->m_PositionIndex[n]++; if (this->m_PositionIndex[n] < this->m_EndIndex[n]) { - this->m_Position += this->m_OffsetTable[n]; + this->MovePosition(this->m_OffsetTable[n]); this->m_Remaining = true; break; } - this->m_Position -= this->m_OffsetTable[n] * (this->m_Region.GetSize()[n] - 1); + this->MovePosition(-this->m_OffsetTable[n] * static_cast(this->m_Region.GetSize()[n] - 1)); this->m_PositionIndex[n] = this->m_BeginIndex[n]; } } @@ -282,8 +282,8 @@ template inline void ImageLinearConstIteratorWithIndex::PreviousLine() { - this->m_Position += - this->m_OffsetTable[m_Direction] * (this->m_EndIndex[m_Direction] - 1 - this->m_PositionIndex[m_Direction]); + this->MovePosition(this->m_OffsetTable[m_Direction] * + (this->m_EndIndex[m_Direction] - 1 - this->m_PositionIndex[m_Direction])); this->m_PositionIndex[m_Direction] = this->m_EndIndex[m_Direction] - 1; @@ -299,12 +299,12 @@ ImageLinearConstIteratorWithIndex::PreviousLine() this->m_PositionIndex[n]--; if (this->m_PositionIndex[n] >= this->m_BeginIndex[n]) { - this->m_Position -= this->m_OffsetTable[n]; + this->MovePosition(-this->m_OffsetTable[n]); this->m_Remaining = true; break; } - this->m_Position += this->m_OffsetTable[n] * (this->m_Region.GetSize()[n] - 1); + this->MovePosition(this->m_OffsetTable[n] * static_cast(this->m_Region.GetSize()[n] - 1)); this->m_PositionIndex[n] = this->m_EndIndex[n] - 1; } } diff --git a/Modules/Core/Common/include/itkImageLinearConstIteratorWithIndex.hxx b/Modules/Core/Common/include/itkImageLinearConstIteratorWithIndex.hxx index e52da3dc538..b194586d4ae 100644 --- a/Modules/Core/Common/include/itkImageLinearConstIteratorWithIndex.hxx +++ b/Modules/Core/Common/include/itkImageLinearConstIteratorWithIndex.hxx @@ -41,7 +41,7 @@ ImageLinearConstIteratorWithIndex::GoToReverseBeginOfLine() { const OffsetValueType distanceToEnd = this->m_EndIndex[m_Direction] - this->m_PositionIndex[m_Direction] - 1; - this->m_Position += m_Jump * distanceToEnd; + this->MovePosition(m_Jump * distanceToEnd); this->m_PositionIndex[m_Direction] = this->m_EndIndex[m_Direction] - 1; } @@ -54,7 +54,7 @@ ImageLinearConstIteratorWithIndex::GoToBeginOfLine() { const OffsetValueType distanceToBegin = this->m_PositionIndex[m_Direction] - this->m_BeginIndex[m_Direction]; - this->m_Position -= m_Jump * distanceToBegin; + this->MovePosition(-m_Jump * distanceToBegin); this->m_PositionIndex[m_Direction] = this->m_BeginIndex[m_Direction]; } @@ -68,7 +68,7 @@ ImageLinearConstIteratorWithIndex::GoToEndOfLine() { const OffsetValueType distanceToEnd = this->m_EndIndex[m_Direction] - this->m_PositionIndex[m_Direction]; - this->m_Position += m_Jump * distanceToEnd; + this->MovePosition(m_Jump * distanceToEnd); this->m_PositionIndex[m_Direction] = this->m_EndIndex[m_Direction]; } diff --git a/Modules/Core/Common/include/itkImageSliceConstIteratorWithIndex.hxx b/Modules/Core/Common/include/itkImageSliceConstIteratorWithIndex.hxx index 2e6cfd4442b..96b534b1f00 100644 --- a/Modules/Core/Common/include/itkImageSliceConstIteratorWithIndex.hxx +++ b/Modules/Core/Common/include/itkImageSliceConstIteratorWithIndex.hxx @@ -30,11 +30,12 @@ ImageSliceConstIteratorWithIndex::NextLine() { // Move to next line this->m_PositionIndex[m_Direction_B]++; - this->m_Position += m_LineJump; + this->MovePosition(static_cast(m_LineJump)); // Move to beginning of line this->m_PositionIndex[m_Direction_A] = this->m_BeginIndex[m_Direction_A]; - this->m_Position -= m_PixelJump * (this->m_EndIndex[m_Direction_A] - this->m_BeginIndex[m_Direction_A]); + this->MovePosition( + -static_cast(m_PixelJump * (this->m_EndIndex[m_Direction_A] - this->m_BeginIndex[m_Direction_A]))); } //---------------------------------------------------------------------- @@ -46,11 +47,12 @@ ImageSliceConstIteratorWithIndex::PreviousLine() { // Move to previous line this->m_PositionIndex[m_Direction_B]--; - this->m_Position -= m_LineJump; + this->MovePosition(-static_cast(m_LineJump)); // Move to end of line this->m_PositionIndex[m_Direction_A] = this->m_EndIndex[m_Direction_A] - 1; - this->m_Position += m_PixelJump * (this->m_EndIndex[m_Direction_A] - this->m_BeginIndex[m_Direction_A]); + this->MovePosition( + static_cast(m_PixelJump * (this->m_EndIndex[m_Direction_A] - this->m_BeginIndex[m_Direction_A]))); } //---------------------------------------------------------------------- @@ -62,7 +64,8 @@ ImageSliceConstIteratorWithIndex::GoToBeginOfSlice() { // Move to beginning of Slice this->m_PositionIndex[m_Direction_B] = this->m_BeginIndex[m_Direction_B]; - this->m_Position -= m_LineJump * (this->m_EndIndex[m_Direction_B] - this->m_BeginIndex[m_Direction_B]); + this->MovePosition( + -static_cast(m_LineJump * (this->m_EndIndex[m_Direction_B] - this->m_BeginIndex[m_Direction_B]))); } //---------------------------------------------------------------------- @@ -73,7 +76,8 @@ void ImageSliceConstIteratorWithIndex::NextSlice() { // Move to beginning of Slice - this->m_Position -= m_LineJump * (this->m_PositionIndex[m_Direction_B] - this->m_BeginIndex[m_Direction_B]); + this->MovePosition(-static_cast( + m_LineJump * (this->m_PositionIndex[m_Direction_B] - this->m_BeginIndex[m_Direction_B]))); this->m_PositionIndex[m_Direction_B] = this->m_BeginIndex[m_Direction_B]; for (unsigned int n = 0; n < TImage::ImageDimension; ++n) @@ -88,12 +92,12 @@ ImageSliceConstIteratorWithIndex::NextSlice() this->m_PositionIndex[n]++; if (this->m_PositionIndex[n] < this->m_EndIndex[n]) { - this->m_Position += this->m_OffsetTable[n]; + this->MovePosition(this->m_OffsetTable[n]); this->m_Remaining = true; break; } - this->m_Position -= this->m_OffsetTable[n + 1] - this->m_OffsetTable[n]; + this->MovePosition(this->m_OffsetTable[n] - this->m_OffsetTable[n + 1]); this->m_PositionIndex[n] = this->m_BeginIndex[n]; } } @@ -107,7 +111,8 @@ ImageSliceConstIteratorWithIndex::PreviousSlice() { // Move to end of Slice this->m_PositionIndex[m_Direction_B] = this->m_EndIndex[m_Direction_B] - 1; - this->m_Position += m_LineJump * (this->m_EndIndex[m_Direction_B] - this->m_BeginIndex[m_Direction_B]); + this->MovePosition( + static_cast(m_LineJump * (this->m_EndIndex[m_Direction_B] - this->m_BeginIndex[m_Direction_B]))); for (unsigned int n = 0; n < TImage::ImageDimension; ++n) { @@ -121,12 +126,12 @@ ImageSliceConstIteratorWithIndex::PreviousSlice() this->m_PositionIndex[n]--; if (this->m_PositionIndex[n] >= this->m_BeginIndex[n]) { - this->m_Position -= this->m_OffsetTable[n]; + this->MovePosition(-this->m_OffsetTable[n]); this->m_Remaining = true; break; } - this->m_Position += this->m_OffsetTable[n + 1] - this->m_OffsetTable[n]; + this->MovePosition(this->m_OffsetTable[n + 1] - this->m_OffsetTable[n]); this->m_PositionIndex[n] = this->m_EndIndex[n] - 1; } } @@ -211,7 +216,7 @@ ImageSliceConstIteratorWithIndex & ImageSliceConstIteratorWithIndex::operator++() { this->m_PositionIndex[m_Direction_A]++; - this->m_Position += m_PixelJump; + this->MovePosition(static_cast(m_PixelJump)); return *this; } @@ -223,7 +228,7 @@ ImageSliceConstIteratorWithIndex & ImageSliceConstIteratorWithIndex::operator--() { this->m_PositionIndex[m_Direction_A]--; - this->m_Position -= m_PixelJump; + this->MovePosition(-static_cast(m_PixelJump)); return *this; } } // end namespace itk diff --git a/Modules/Core/Common/test/CMakeLists.txt b/Modules/Core/Common/test/CMakeLists.txt index d58d007c042..02a3a3e09e6 100644 --- a/Modules/Core/Common/test/CMakeLists.txt +++ b/Modules/Core/Common/test/CMakeLists.txt @@ -1518,9 +1518,11 @@ set( itkImageGTest.cxx itkImageIORegionGTest.cxx itkImageIteratorsGTest.cxx + itkImageLinearIteratorWithIndexGTest.cxx itkImageNeighborhoodOffsetsGTest.cxx itkImageRandomConstIteratorWithIndexGTest.cxx itkImageRandomNonRepeatingIteratorWithIndexGTest.cxx + itkImageSliceIteratorWithIndexGTest.cxx itkImageRegionGTest.cxx itkImageRegionRangeGTest.cxx itkImageTransformGTest.cxx diff --git a/Modules/Core/Common/test/itkImageLinearIteratorWithIndexGTest.cxx b/Modules/Core/Common/test/itkImageLinearIteratorWithIndexGTest.cxx new file mode 100644 index 00000000000..bd221eb245a --- /dev/null +++ b/Modules/Core/Common/test/itkImageLinearIteratorWithIndexGTest.cxx @@ -0,0 +1,110 @@ +/*========================================================================= + * + * Copyright NumFOCUS + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0.txt + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + *=========================================================================*/ + +#include "itkImage.h" +#include "itkImageLinearConstIteratorWithIndex.h" +#include "itkImageLinearIteratorWithIndex.h" +#include "itkImageBufferRange.h" +#include "itkGTest.h" + +#include +#include + +namespace +{ +using ImageType = itk::Image; +using RegionType = ImageType::RegionType; + +ImageType::Pointer +MakeImage() +{ + auto image = ImageType::New(); + image->SetRegions(RegionType({ { 10, 20, 30 } }, { { 6, 5, 4 } })); + image->Allocate(); + float value = 1.0F; + for (float & pixel : itk::ImageBufferRange{ *image }) + { + pixel = value++; + } + return image; +} + +// Visits every pixel of `region` once, in both orders, checking index, value and position agreement. +template +void +ExpectTraversal(ImageType * image, const RegionType & region, unsigned int direction) +{ + TIterator it(image, region); + it.SetDirection(direction); + for (const bool reverse : { false, true }) + { + SCOPED_TRACE(reverse ? "reverse" : "forward"); + std::vector visits(image->GetBufferedRegion().GetNumberOfPixels(), 0); + reverse ? it.GoToReverseBegin() : it.GoToBegin(); + while (!(reverse ? it.IsAtReverseEnd() : it.IsAtEnd())) + { + while (!(reverse ? it.IsAtReverseEndOfLine() : it.IsAtEndOfLine())) + { + const auto index = it.GetIndex(); + ASSERT_TRUE(region.IsInside(index)); + EXPECT_EQ(it.Get(), image->GetPixel(index)); + EXPECT_EQ(visits[image->ComputeOffset(index)]++, 0U); + reverse ? --it : ++it; + } + reverse ? it.PreviousLine() : it.NextLine(); + } + EXPECT_EQ(std::count(visits.cbegin(), visits.cend(), 1U), region.GetNumberOfPixels()); + } +} + +const RegionType regions[] = { RegionType({ { 10, 20, 30 } }, { { 6, 5, 4 } }), + RegionType({ { 11, 21, 31 } }, { { 4, 3, 2 } }), + RegionType({ { 10, 20, 30 } }, { { 3, 2, 2 } }), + RegionType({ { 13, 23, 32 } }, { { 3, 2, 2 } }) }; +} // namespace + +TEST(ImageLinearIteratorWithIndex, VisitsEveryPixelOnceInEveryDirection) +{ + const auto image = MakeImage(); + for (const auto & region : regions) + { + for (unsigned int direction = 0; direction < 3; ++direction) + { + SCOPED_TRACE(testing::Message() << "region " << region << " direction " << direction); + ExpectTraversal>(image, region, direction); + ExpectTraversal>(image, region, direction); + } + } +} + +TEST(ImageLinearIteratorWithIndex, GoToBeginAndEndOfLineLandOnTheLine) +{ + const auto image = MakeImage(); + const auto region = regions[1]; + itk::ImageLinearConstIteratorWithIndex it(image, region); + it.SetDirection(2); + it.GoToBegin(); + it.NextLine(); + it.GoToEndOfLine(); + EXPECT_EQ(it.GetIndex()[2], region.GetIndex(2) + static_cast(region.GetSize(2))); + it.GoToReverseBeginOfLine(); + EXPECT_EQ(it.Get(), image->GetPixel(it.GetIndex())); + it.GoToBeginOfLine(); + EXPECT_EQ(it.GetIndex()[2], region.GetIndex(2)); + EXPECT_EQ(it.Get(), image->GetPixel(it.GetIndex())); +} diff --git a/Modules/Core/Common/test/itkImageSliceIteratorWithIndexGTest.cxx b/Modules/Core/Common/test/itkImageSliceIteratorWithIndexGTest.cxx new file mode 100644 index 00000000000..28e16e4691c --- /dev/null +++ b/Modules/Core/Common/test/itkImageSliceIteratorWithIndexGTest.cxx @@ -0,0 +1,129 @@ +/*========================================================================= + * + * Copyright NumFOCUS + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0.txt + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + *=========================================================================*/ + +#include "itkImage.h" +#include "itkImageSliceConstIteratorWithIndex.h" +#include "itkImageSliceIteratorWithIndex.h" +#include "itkImageBufferRange.h" +#include "itkImageRegionConstIteratorWithIndex.h" +#include "itkGTest.h" + +#include +#include + +namespace +{ +using ImageType = itk::Image; +using RegionType = ImageType::RegionType; + +ImageType::Pointer +MakeImage() +{ + auto image = ImageType::New(); + image->SetRegions(RegionType({ { 10, 20, 30 } }, { { 6, 5, 4 } })); + image->Allocate(); + float value = 1.0F; + for (float & pixel : itk::ImageBufferRange{ *image }) + { + pixel = value++; + } + return image; +} + +// Visits every pixel of `region` once, in both orders, checking index, value and position agreement. +template +void +ExpectTraversal(ImageType * image, const RegionType & region, unsigned int first, unsigned int second) +{ + TIterator it(image, region); + it.SetFirstDirection(first); + it.SetSecondDirection(second); + for (const bool reverse : { false, true }) + { + SCOPED_TRACE(reverse ? "reverse" : "forward"); + std::vector visits(image->GetBufferedRegion().GetNumberOfPixels(), 0); + reverse ? it.GoToReverseBegin() : it.GoToBegin(); + while (!(reverse ? it.IsAtReverseEnd() : it.IsAtEnd())) + { + while (!(reverse ? it.IsAtReverseEndOfSlice() : it.IsAtEndOfSlice())) + { + while (!(reverse ? it.IsAtReverseEndOfLine() : it.IsAtEndOfLine())) + { + const auto index = it.GetIndex(); + ASSERT_TRUE(region.IsInside(index)); + EXPECT_EQ(it.Get(), image->GetPixel(index)); + EXPECT_EQ(visits[image->ComputeOffset(index)]++, 0U); + reverse ? --it : ++it; + } + reverse ? it.PreviousLine() : it.NextLine(); + } + reverse ? it.PreviousSlice() : it.NextSlice(); + } + EXPECT_EQ(std::count(visits.cbegin(), visits.cend(), 1U), region.GetNumberOfPixels()); + } +} + +const RegionType regions[] = { RegionType({ { 10, 20, 30 } }, { { 6, 5, 4 } }), + RegionType({ { 11, 21, 31 } }, { { 4, 3, 2 } }), + RegionType({ { 10, 20, 30 } }, { { 3, 2, 2 } }), + RegionType({ { 13, 23, 32 } }, { { 3, 2, 2 } }) }; +} // namespace + +TEST(ImageSliceIteratorWithIndex, VisitsEveryPixelOnceInEveryDirectionPair) +{ + const auto image = MakeImage(); + for (const auto & region : regions) + { + for (unsigned int first = 0; first < 3; ++first) + { + for (unsigned int second = 0; second < 3; ++second) + { + if (first != second) + { + SCOPED_TRACE(testing::Message() << "region " << region << " directions " << first << ',' << second); + ExpectTraversal>(image, region, first, second); + ExpectTraversal>(image, region, first, second); + } + } + } + } +} + +TEST(ImageSliceIteratorWithIndex, SetWritesTheIndexedPixel) +{ + const auto image = MakeImage(); + const auto region = regions[3]; + itk::ImageSliceIteratorWithIndex it(image, region); + it.SetFirstDirection(2); + it.SetSecondDirection(0); + for (it.GoToBegin(); !it.IsAtEnd(); it.NextSlice()) + { + for (; !it.IsAtEndOfSlice(); it.NextLine()) + { + for (; !it.IsAtEndOfLine(); ++it) + { + it.Set(-it.Get()); + } + } + } + for (itk::ImageRegionConstIteratorWithIndex check(image, image->GetBufferedRegion()); !check.IsAtEnd(); + ++check) + { + EXPECT_EQ(check.Get() < 0.0F, region.IsInside(check.GetIndex())) << check.GetIndex(); + } +}