diff --git a/Documentation/docs/migration_guides/itk_6_migration_guide.md b/Documentation/docs/migration_guides/itk_6_migration_guide.md index 28da7130ca5..3e418baa721 100644 --- a/Documentation/docs/migration_guides/itk_6_migration_guide.md +++ b/Documentation/docs/migration_guides/itk_6_migration_guide.md @@ -1159,3 +1159,33 @@ approximation) are unchanged and intentionally do not return their own type. `typeid` comparison, or a `static_cast` to a sibling type), update that code to the new concrete type. Such a dependency is uncommon and was not found in a survey of downstream ITK consumers. + +## `GradientImageFilter::OverrideBoundaryCondition` deprecated in favor of `SetBoundaryCondition` + +`GradientImageFilter` is the only ITK class whose `OverrideBoundaryCondition` *takes +ownership* of its argument; every other class of that name stores a non-owning +pointer. Because the two share a signature, passing a stack object to a +`GradientImageFilter` compiled silently and then double-freed. + +The owning API now says so in its name and its type: + +```cpp +// ITKv6 +filter->SetBoundaryCondition(std::make_unique>()); + +// ITKv5 (deprecated, removed when ITK_FUTURE_LEGACY_REMOVE is enabled) +filter->OverrideBoundaryCondition(new itk::PeriodicBoundaryCondition); +``` + +`GetBoundaryCondition()` and `ResetBoundaryCondition()` were added to complete the +interface, matching the rest of the family. + +### What you need to do + +Replace `OverrideBoundaryCondition(new X)` with `SetBoundaryCondition(std::make_unique())` +on `GradientImageFilter`. In Python, pass the boundary condition to +`SetBoundaryCondition`; ownership moves to the filter, and re-using that object +afterwards raises `RuntimeError` rather than crashing. + +Calls to `OverrideBoundaryCondition` on any *other* class are unaffected — those +never took ownership and keep their current behavior. diff --git a/Modules/Core/Common/include/itkConstNeighborhoodIterator.h b/Modules/Core/Common/include/itkConstNeighborhoodIterator.h index f22c789b362..c0cf0115340 100644 --- a/Modules/Core/Common/include/itkConstNeighborhoodIterator.h +++ b/Modules/Core/Common/include/itkConstNeighborhoodIterator.h @@ -483,11 +483,8 @@ class ITK_TEMPLATE_EXPORT ConstNeighborhoodIterator bool IndexInBounds(const NeighborIndexType n) const; - /** Allows a user to override the internal boundary condition. Care should - * be taken to ensure that the overriding boundary condition is a persistent - * object during the time it is referenced. The overriding condition - * can be of a different type than the default type as long as it is - * a subclass of ImageBoundaryCondition. */ + /** Overrides the internal boundary condition. Does not take ownership; the + * caller must keep the object alive while it is referenced. */ void OverrideBoundaryCondition(const ImageBoundaryConditionPointerType i) { diff --git a/Modules/Core/Common/wrapping/itkOptimizerParameters.wrap b/Modules/Core/Common/wrapping/itkOptimizerParameters.wrap index 30c9c13e06c..efaac348613 100644 --- a/Modules/Core/Common/wrapping/itkOptimizerParameters.wrap +++ b/Modules/Core/Common/wrapping/itkOptimizerParameters.wrap @@ -7,6 +7,9 @@ itk_end_wrap_class() itk_wrap_class("itk::OptimizerParameters") foreach(t ${types}) + # SetHelper stores the argument in a unique_ptr member. + string(APPEND ITK_WRAP_PYTHON_SWIG_EXT "%apply SWIGTYPE *DISOWN { itkOptimizerParametersHelper${ITKM_${t}} * helper };\n") + itk_wrap_template("${ITKM_${t}}" "${ITKT_${t}}") endforeach() itk_end_wrap_class() diff --git a/Modules/Core/Common/wrapping/test/CMakeLists.txt b/Modules/Core/Common/wrapping/test/CMakeLists.txt index 2f948e1d43c..6594cac3cc8 100644 --- a/Modules/Core/Common/wrapping/test/CMakeLists.txt +++ b/Modules/Core/Common/wrapping/test/CMakeLists.txt @@ -66,4 +66,9 @@ if(ITK_WRAP_PYTHON) COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/itkImageLifetimeTest.py ) + itk_python_add_test( + NAME itkOptimizerParametersOwnershipPythonTest + COMMAND + ${CMAKE_CURRENT_SOURCE_DIR}/itkOptimizerParametersOwnershipTest.py + ) endif() diff --git a/Modules/Core/Common/wrapping/test/itkOptimizerParametersOwnershipTest.py b/Modules/Core/Common/wrapping/test/itkOptimizerParametersOwnershipTest.py new file mode 100644 index 00000000000..740c991563a --- /dev/null +++ b/Modules/Core/Common/wrapping/test/itkOptimizerParametersOwnershipTest.py @@ -0,0 +1,36 @@ +# ========================================================================== +# +# 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. +# +# ========================================================================== +"""OptimizerParameters.SetHelper takes ownership of its argument. + +Without a DISOWN typemap, Python and the m_Helper unique_ptr both free the helper, +and the interpreter aborts at shutdown. A non-zero exit code from this script is +itself part of the regression check. +""" + +import itk + +parameters = itk.OptimizerParameters[itk.D](3) +helper = itk.OptimizerParametersHelper[itk.D]() + +assert helper.thisown, "Python should own a freshly constructed helper" + +parameters.SetHelper(helper) + +assert not helper.thisown, "SetHelper must transfer ownership away from Python" + +print("Test finished.") diff --git a/Modules/Filtering/BinaryMathematicalMorphology/include/itkObjectMorphologyImageFilter.h b/Modules/Filtering/BinaryMathematicalMorphology/include/itkObjectMorphologyImageFilter.h index 72cf89d6384..7716a0b863b 100644 --- a/Modules/Filtering/BinaryMathematicalMorphology/include/itkObjectMorphologyImageFilter.h +++ b/Modules/Filtering/BinaryMathematicalMorphology/include/itkObjectMorphologyImageFilter.h @@ -137,11 +137,8 @@ class ITK_TEMPLATE_EXPORT ObjectMorphologyImageFilter : public ImageToImageFilte void GenerateInputRequestedRegion() override; - /** Allows a user to override the internal boundary condition. Care should be - * be taken to ensure that the overriding boundary condition is a persistent - * object during the time it is referenced. The overriding condition - * can be of a different type than the default type as long as it is - * a subclass of ImageBoundaryCondition. + /** Overrides the internal boundary condition. Does not take ownership; the + * caller must keep the object alive while it is referenced. * NOTE: Don't forget to set UseBoundaryCondition to true! */ void OverrideBoundaryCondition(const ImageBoundaryConditionPointerType i) diff --git a/Modules/Filtering/ImageFilterBase/include/itkNeighborhoodOperatorImageFilter.h b/Modules/Filtering/ImageFilterBase/include/itkNeighborhoodOperatorImageFilter.h index 30c18253e48..bae34cb85b8 100644 --- a/Modules/Filtering/ImageFilterBase/include/itkNeighborhoodOperatorImageFilter.h +++ b/Modules/Filtering/ImageFilterBase/include/itkNeighborhoodOperatorImageFilter.h @@ -115,11 +115,8 @@ class ITK_TEMPLATE_EXPORT NeighborhoodOperatorImageFilter : public ImageToImageF return m_Operator; } - /** Allows a user to override the internal boundary condition. Care should be - * be taken to ensure that the overriding boundary condition is a persistent - * object during the time it is referenced. The overriding condition - * can be of a different type than the default type as long as it is - * a subclass of ImageBoundaryCondition. */ + /** Overrides the internal boundary condition. Does not take ownership; the + * caller must keep the object alive while it is referenced. */ void OverrideBoundaryCondition(const ImageBoundaryConditionPointerType i) { diff --git a/Modules/Filtering/ImageFilterBase/include/itkVectorNeighborhoodOperatorImageFilter.h b/Modules/Filtering/ImageFilterBase/include/itkVectorNeighborhoodOperatorImageFilter.h index 7d76aaae965..b960fee8520 100644 --- a/Modules/Filtering/ImageFilterBase/include/itkVectorNeighborhoodOperatorImageFilter.h +++ b/Modules/Filtering/ImageFilterBase/include/itkVectorNeighborhoodOperatorImageFilter.h @@ -105,11 +105,8 @@ class ITK_TEMPLATE_EXPORT VectorNeighborhoodOperatorImageFilter : public ImageTo this->Modified(); } - /** Allows a user to override the internal boundary condition. Care should be - * be taken to ensure that the overriding boundary condition is a persistent - * object during the time it is referenced. The overriding condition - * can be of a different type than the default type as long as it is - * a subclass of ImageBoundaryCondition. */ + /** Overrides the internal boundary condition. Does not take ownership; the + * caller must keep the object alive while it is referenced. */ void OverrideBoundaryCondition(const ImageBoundaryConditionPointerType i) { diff --git a/Modules/Filtering/ImageGradient/include/itkGradientImageFilter.h b/Modules/Filtering/ImageGradient/include/itkGradientImageFilter.h index cebcd083025..9fb034a7470 100644 --- a/Modules/Filtering/ImageGradient/include/itkGradientImageFilter.h +++ b/Modules/Filtering/ImageGradient/include/itkGradientImageFilter.h @@ -139,9 +139,36 @@ class ITK_TEMPLATE_EXPORT GradientImageFilter : public ImageToImageFilter; + + /** Replaces the default boundary condition. The filter takes ownership. + * Throws when the argument is empty, as the boundary condition is dereferenced + * unconditionally while processing boundary faces. */ + void + SetBoundaryCondition(std::unique_ptr boundaryCondition); + + /** Returns the boundary condition in use. The filter retains ownership. */ + [[nodiscard]] const BoundaryConditionType * + GetBoundaryCondition() const + { + return m_BoundaryCondition.get(); + } + + /** Restores the default ZeroFluxNeumann boundary condition. */ + void + ResetBoundaryCondition() + { + m_BoundaryCondition = std::make_unique>(); + this->Modified(); + } + +#if !defined(ITK_FUTURE_LEGACY_REMOVE) + /** Allows to change the default boundary condition. The filter takes ownership. + \deprecated Use GradientImageFilter::SetBoundaryCondition instead, whose + `unique_ptr` parameter states the ownership transfer. */ void OverrideBoundaryCondition(ImageBoundaryCondition * boundaryCondition); +#endif itkConceptMacro(InputConvertibleToOutputCheck, (Concept::Convertible)); itkConceptMacro(OutputHasNumericTraitsCheck, (Concept::HasNumericTraits)); diff --git a/Modules/Filtering/ImageGradient/include/itkGradientImageFilter.hxx b/Modules/Filtering/ImageGradient/include/itkGradientImageFilter.hxx index 22af83d5b8c..1b838971167 100644 --- a/Modules/Filtering/ImageGradient/include/itkGradientImageFilter.hxx +++ b/Modules/Filtering/ImageGradient/include/itkGradientImageFilter.hxx @@ -37,13 +37,29 @@ GradientImageFilterThreaderUpdateProgressOff(); } +template +void +GradientImageFilter::SetBoundaryCondition( + std::unique_ptr boundaryCondition) +{ + if (boundaryCondition == nullptr) + { + itkExceptionMacro("The boundary condition should not be null!"); + } + m_BoundaryCondition = std::move(boundaryCondition); + this->Modified(); +} + +#if !defined(ITK_FUTURE_LEGACY_REMOVE) template void GradientImageFilter::OverrideBoundaryCondition( ImageBoundaryCondition * boundaryCondition) { m_BoundaryCondition.reset(boundaryCondition); + this->Modified(); } +#endif template void diff --git a/Modules/Filtering/ImageGradient/test/itkGradientImageFilterGTest.cxx b/Modules/Filtering/ImageGradient/test/itkGradientImageFilterGTest.cxx index 758175ae77d..072858ca25c 100644 --- a/Modules/Filtering/ImageGradient/test/itkGradientImageFilterGTest.cxx +++ b/Modules/Filtering/ImageGradient/test/itkGradientImageFilterGTest.cxx @@ -19,12 +19,16 @@ // First include the header file to be tested: #include "itkGradientImageFilter.h" +#include "itkConstNeighborhoodIterator.h" #include "itkDeref.h" #include "itkImage.h" #include "itkImageBufferRange.h" #include "itkIndexRange.h" +#include "itkNeighborhoodOperatorImageFilter.h" +#include "itkZeroFluxNeumannBoundaryCondition.h" #include +#include // Tests the output for a uniform input image. @@ -115,3 +119,133 @@ TEST(GradientImageFilter, ConstantGradientInputImage) } } } + + +namespace +{ +// Counts its own destructions, so tests can observe who owns it. +template +class DestructionCountingBoundaryCondition : public itk::ZeroFluxNeumannBoundaryCondition +{ +public: + explicit DestructionCountingBoundaryCondition(unsigned int & counter) + : m_Counter(&counter) + {} + + ~DestructionCountingBoundaryCondition() override { ++(*m_Counter); } + +private: + unsigned int * m_Counter{}; +}; +} // namespace + + +// The unique_ptr overload takes ownership: the filter destroys the boundary condition. +TEST(GradientImageFilter, SetBoundaryConditionTakesOwnership) +{ + using ImageType = itk::Image; + using BoundaryConditionType = DestructionCountingBoundaryCondition; + + unsigned int destructionCount{ 0 }; + { + const auto filter = itk::GradientImageFilter::New(); + filter->SetBoundaryCondition(std::make_unique(destructionCount)); + EXPECT_EQ(destructionCount, 0u); + } + EXPECT_EQ(destructionCount, 1u); +} + + +// Get returns what was set; Reset restores the default and destroys the previous one. +TEST(GradientImageFilter, GetAndResetBoundaryCondition) +{ + using ImageType = itk::Image; + using BoundaryConditionType = DestructionCountingBoundaryCondition; + + unsigned int destructionCount{ 0 }; + const auto filter = itk::GradientImageFilter::New(); + + auto boundaryCondition = std::make_unique(destructionCount); + const auto rawPointer = boundaryCondition.get(); + filter->SetBoundaryCondition(std::move(boundaryCondition)); + EXPECT_EQ(filter->GetBoundaryCondition(), rawPointer); + + filter->ResetBoundaryCondition(); + EXPECT_EQ(destructionCount, 1u); + EXPECT_NE(filter->GetBoundaryCondition(), nullptr); + EXPECT_NE(filter->GetBoundaryCondition(), rawPointer); +} + + +// Changing the boundary condition must invalidate an already computed output. +TEST(GradientImageFilter, SetBoundaryConditionModifiesFilter) +{ + using ImageType = itk::Image; + + const auto inputImage = ImageType::New(); + inputImage->SetRegions(itk::Size<2>::Filled(4)); + inputImage->Allocate(true); + + const auto filter = itk::GradientImageFilter::New(); + filter->SetInput(inputImage); + filter->Update(); + const auto modifiedTimeAfterUpdate = filter->GetMTime(); + + filter->SetBoundaryCondition(std::make_unique>()); + EXPECT_GT(filter->GetMTime(), modifiedTimeAfterUpdate); + + filter->ResetBoundaryCondition(); + EXPECT_GT(filter->GetMTime(), modifiedTimeAfterUpdate); +} + + +// A null boundary condition would be dereferenced while processing boundary faces. +TEST(GradientImageFilter, SetBoundaryConditionRejectsNull) +{ + using ImageType = itk::Image; + + const auto filter = itk::GradientImageFilter::New(); + EXPECT_THROW(filter->SetBoundaryCondition(nullptr), itk::ExceptionObject); + EXPECT_NE(filter->GetBoundaryCondition(), nullptr); +} + + +// The rest of the OverrideBoundaryCondition family does not take ownership. +TEST(GradientImageFilter, OverrideBoundaryConditionDoesNotTakeOwnership) +{ + using ImageType = itk::Image; + using BoundaryConditionType = DestructionCountingBoundaryCondition; + + unsigned int destructionCount{ 0 }; + { + BoundaryConditionType boundaryCondition(destructionCount); + + itk::ConstNeighborhoodIterator iterator; + iterator.OverrideBoundaryCondition(&boundaryCondition); + + const auto neighborhoodFilter = itk::NeighborhoodOperatorImageFilter::New(); + neighborhoodFilter->OverrideBoundaryCondition(&boundaryCondition); + + EXPECT_EQ(destructionCount, 0u); + } + // Destroyed exactly once, by leaving scope -- not by either consumer. + EXPECT_EQ(destructionCount, 1u); +} + + +#if !defined(ITK_FUTURE_LEGACY_REMOVE) +// The deprecated overload keeps its original ownership semantics. +TEST(GradientImageFilter, DeprecatedOverrideBoundaryConditionStillTakesOwnership) +{ + using ImageType = itk::Image; + using BoundaryConditionType = DestructionCountingBoundaryCondition; + + unsigned int destructionCount{ 0 }; + { + const auto filter = itk::GradientImageFilter::New(); + filter->OverrideBoundaryCondition(new BoundaryConditionType(destructionCount)); + EXPECT_EQ(destructionCount, 0u); + } + EXPECT_EQ(destructionCount, 1u); +} +#endif diff --git a/Modules/Filtering/ImageGradient/test/itkGradientImageFilterTest.cxx b/Modules/Filtering/ImageGradient/test/itkGradientImageFilterTest.cxx index 9020375b366..54386db3353 100644 --- a/Modules/Filtering/ImageGradient/test/itkGradientImageFilterTest.cxx +++ b/Modules/Filtering/ImageGradient/test/itkGradientImageFilterTest.cxx @@ -24,6 +24,8 @@ #include "itkPeriodicBoundaryCondition.h" +#include + inline std::ostream & operator<<(std::ostream & o, const itk::CovariantVector & v) { @@ -88,8 +90,8 @@ itkGradientImageFilterTest(int argc, char * argv[]) auto filter2 = FilterType2::New(); using PeriodicBoundaryType = itk::PeriodicBoundaryCondition; - // Test the OverrideBoundaryCondition setting; - filter2->OverrideBoundaryCondition(new PeriodicBoundaryType); + // Test the SetBoundaryCondition setting; + filter2->SetBoundaryCondition(std::make_unique()); ITK_EXERCISE_BASIC_OBJECT_METHODS(filter2, GradientImageFilter, ImageToImageFilter); diff --git a/Modules/Filtering/ImageGradient/wrapping/itkGradientImageFilter.wrap b/Modules/Filtering/ImageGradient/wrapping/itkGradientImageFilter.wrap index be96554cf79..54f26e74c1b 100644 --- a/Modules/Filtering/ImageGradient/wrapping/itkGradientImageFilter.wrap +++ b/Modules/Filtering/ImageGradient/wrapping/itkGradientImageFilter.wrap @@ -1,9 +1,20 @@ +# Defines the %unique_ptr macro used per instantiation below. +string(APPEND ITK_WRAP_PYTHON_SWIG_EXT "%include \n") + itk_wrap_class("itk::GradientImageFilter" POINTER) foreach(d ${ITK_WRAP_IMAGE_DIMS}) set(vector_dim ${d}) # Wrap only vector dimensions which are the same as image dimensions foreach(t ${WRAP_ITK_SCALAR}) + # SetBoundaryCondition and OverrideBoundaryCondition both adopt their argument. + string( + APPEND + ITK_WRAP_PYTHON_SWIG_EXT + "%unique_ptr(itkImageBoundaryCondition${ITKM_I${t}${vector_dim}})\n" + "%apply SWIGTYPE *DISOWN { itkImageBoundaryCondition${ITKM_I${t}${vector_dim}} * boundaryCondition };\n" + ) + if(ITK_WRAP_covariant_vector_float) itk_wrap_template("${ITKM_I${t}${vector_dim}}${ITKM_F}${ITKM_F}" "${ITKT_I${t}${vector_dim}},${ITKT_F},${ITKT_F}") endif() diff --git a/Modules/Filtering/ImageGradient/wrapping/test/CMakeLists.txt b/Modules/Filtering/ImageGradient/wrapping/test/CMakeLists.txt index bad48378d6f..442b8c87fb0 100644 --- a/Modules/Filtering/ImageGradient/wrapping/test/CMakeLists.txt +++ b/Modules/Filtering/ImageGradient/wrapping/test/CMakeLists.txt @@ -14,4 +14,10 @@ if(ITK_WRAP_PYTHON AND ITK_WRAP_float AND wrap_2_index GREATER -1) ${ITK_TEST_OUTPUT_DIR}/GradientMagnitudeRecursiveGaussianImageFilterTest.png 5 ) + + itk_python_add_test( + NAME itkGradientImageFilterOwnershipPythonTest + COMMAND + ${CMAKE_CURRENT_SOURCE_DIR}/itkGradientImageFilterOwnershipTest.py + ) endif() diff --git a/Modules/Filtering/ImageGradient/wrapping/test/itkGradientImageFilterOwnershipTest.py b/Modules/Filtering/ImageGradient/wrapping/test/itkGradientImageFilterOwnershipTest.py new file mode 100644 index 00000000000..d9e9ee37b13 --- /dev/null +++ b/Modules/Filtering/ImageGradient/wrapping/test/itkGradientImageFilterOwnershipTest.py @@ -0,0 +1,74 @@ +# ========================================================================== +# +# 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. +# +# ========================================================================== +"""GradientImageFilter.OverrideBoundaryCondition takes ownership of its argument. + +Without a DISOWN typemap, Python and the filter's unique_ptr member both free the +boundary condition, and the interpreter aborts at shutdown. A non-zero exit code +from this script is itself part of the regression check. +""" + +import itk + +itk.auto_progress(2) + +ImageType = itk.Image[itk.F, 2] + +filt = itk.GradientImageFilter[ImageType, itk.F, itk.F].New() +boundaryCondition = itk.PeriodicBoundaryCondition[ImageType]() + +assert ( + boundaryCondition.thisown +), "Python should own a freshly constructed boundary condition" + +filt.SetBoundaryCondition(boundaryCondition) + +assert ( + not boundaryCondition.thisown +), "SetBoundaryCondition must transfer ownership away from Python" + +# Re-offering an already-adopted object must be refused, not silently double-freed. +try: + filt.SetBoundaryCondition(boundaryCondition) +except RuntimeError: + pass +else: + raise AssertionError("SetBoundaryCondition must reject a non-owned object") + +# The deprecated overload keeps its ownership semantics while it still exists. +legacyFilter = itk.GradientImageFilter[ImageType, itk.F, itk.F].New() +legacyBoundaryCondition = itk.PeriodicBoundaryCondition[ImageType]() +legacyFilter.OverrideBoundaryCondition(legacyBoundaryCondition) + +assert ( + not legacyBoundaryCondition.thisown +), "OverrideBoundaryCondition must transfer ownership away from Python" + +# The filter must remain usable with the adopted boundary condition. +image = ImageType.New() +region = itk.ImageRegion[2]() +region.SetSize([8, 8]) +image.SetRegions(region) +image.Allocate() +image.FillBuffer(1.0) + +filt.SetInput(image) +filt.Update() + +assert filt.GetOutput().GetLargestPossibleRegion().GetSize()[0] == 8 + +print("Test finished.") diff --git a/Modules/Filtering/MathematicalMorphology/include/itkMorphologyImageFilter.h b/Modules/Filtering/MathematicalMorphology/include/itkMorphologyImageFilter.h index a61a6c20d09..c727c892d30 100644 --- a/Modules/Filtering/MathematicalMorphology/include/itkMorphologyImageFilter.h +++ b/Modules/Filtering/MathematicalMorphology/include/itkMorphologyImageFilter.h @@ -112,11 +112,8 @@ class ITK_TEMPLATE_EXPORT MorphologyImageFilter : public KernelImageFilter