Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 30 additions & 0 deletions Documentation/docs/migration_guides/itk_6_migration_guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -1184,3 +1184,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<itk::PeriodicBoundaryCondition<ImageType>>());

// ITKv5 (deprecated, removed when ITK_FUTURE_LEGACY_REMOVE is enabled)
filter->OverrideBoundaryCondition(new itk::PeriodicBoundaryCondition<ImageType>);
```

`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<X>())`
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.
7 changes: 2 additions & 5 deletions Modules/Core/Common/include/itkConstNeighborhoodIterator.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
29 changes: 28 additions & 1 deletion Modules/Filtering/ImageGradient/include/itkGradientImageFilter.h
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,36 @@ class ITK_TEMPLATE_EXPORT GradientImageFilter : public ImageToImageFilter<TInput
}
#endif

/** Allows to change the default boundary condition */
using BoundaryConditionType = ImageBoundaryCondition<TInputImage, TInputImage>;

/** 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<BoundaryConditionType> 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<ZeroFluxNeumannBoundaryCondition<TInputImage>>();
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<TInputImage> * boundaryCondition);
#endif

itkConceptMacro(InputConvertibleToOutputCheck, (Concept::Convertible<InputPixelType, OutputValueType>));
itkConceptMacro(OutputHasNumericTraitsCheck, (Concept::HasNumericTraits<OutputValueType>));
Expand Down
16 changes: 16 additions & 0 deletions Modules/Filtering/ImageGradient/include/itkGradientImageFilter.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,29 @@ GradientImageFilter<TInputImage, TOperatorValueType, TOutputValueType, TOutputIm
this->ThreaderUpdateProgressOff();
}

template <typename TInputImage, typename TOperatorValueType, typename TOutputValue, typename TOutputImage>
void
GradientImageFilter<TInputImage, TOperatorValueType, TOutputValue, TOutputImage>::SetBoundaryCondition(
std::unique_ptr<BoundaryConditionType> boundaryCondition)
{
if (boundaryCondition == nullptr)
{
itkExceptionMacro("The boundary condition should not be null!");
}
m_BoundaryCondition = std::move(boundaryCondition);
Comment thread
hjmjohnson marked this conversation as resolved.
Comment thread
hjmjohnson marked this conversation as resolved.
this->Modified();
}

#if !defined(ITK_FUTURE_LEGACY_REMOVE)
template <typename TInputImage, typename TOperatorValueType, typename TOutputValue, typename TOutputImage>
void
GradientImageFilter<TInputImage, TOperatorValueType, TOutputValue, TOutputImage>::OverrideBoundaryCondition(
ImageBoundaryCondition<TInputImage> * boundaryCondition)
{
m_BoundaryCondition.reset(boundaryCondition);
this->Modified();
}
#endif

template <typename TInputImage, typename TOperatorValueType, typename TOutputValueType, typename TOutputImageType>
void
Expand Down
134 changes: 134 additions & 0 deletions Modules/Filtering/ImageGradient/test/itkGradientImageFilterGTest.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -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 <gtest/gtest.h>
#include <memory>


// Tests the output for a uniform input image.
Expand Down Expand Up @@ -115,3 +119,133 @@ TEST(GradientImageFilter, ConstantGradientInputImage)
}
}
}


namespace
{
// Counts its own destructions, so tests can observe who owns it.
template <typename TImage>
class DestructionCountingBoundaryCondition : public itk::ZeroFluxNeumannBoundaryCondition<TImage>
{
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<int>;
using BoundaryConditionType = DestructionCountingBoundaryCondition<ImageType>;

unsigned int destructionCount{ 0 };
{
const auto filter = itk::GradientImageFilter<ImageType>::New();
filter->SetBoundaryCondition(std::make_unique<BoundaryConditionType>(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<int>;
using BoundaryConditionType = DestructionCountingBoundaryCondition<ImageType>;

unsigned int destructionCount{ 0 };
const auto filter = itk::GradientImageFilter<ImageType>::New();

auto boundaryCondition = std::make_unique<BoundaryConditionType>(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<int>;

const auto inputImage = ImageType::New();
inputImage->SetRegions(itk::Size<2>::Filled(4));
inputImage->Allocate(true);

const auto filter = itk::GradientImageFilter<ImageType>::New();
filter->SetInput(inputImage);
filter->Update();
const auto modifiedTimeAfterUpdate = filter->GetMTime();

filter->SetBoundaryCondition(std::make_unique<itk::ZeroFluxNeumannBoundaryCondition<ImageType>>());
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<int>;

const auto filter = itk::GradientImageFilter<ImageType>::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<int>;
using BoundaryConditionType = DestructionCountingBoundaryCondition<ImageType>;

unsigned int destructionCount{ 0 };
{
BoundaryConditionType boundaryCondition(destructionCount);

itk::ConstNeighborhoodIterator<ImageType> iterator;
iterator.OverrideBoundaryCondition(&boundaryCondition);

const auto neighborhoodFilter = itk::NeighborhoodOperatorImageFilter<ImageType, ImageType>::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<int>;
using BoundaryConditionType = DestructionCountingBoundaryCondition<ImageType>;

unsigned int destructionCount{ 0 };
{
const auto filter = itk::GradientImageFilter<ImageType>::New();
filter->OverrideBoundaryCondition(new BoundaryConditionType(destructionCount));
EXPECT_EQ(destructionCount, 0u);
}
EXPECT_EQ(destructionCount, 1u);
}
#endif
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@

#include "itkPeriodicBoundaryCondition.h"

#include <memory>

inline std::ostream &
operator<<(std::ostream & o, const itk::CovariantVector<float, 3> & v)
{
Expand Down Expand Up @@ -88,8 +90,7 @@ itkGradientImageFilterTest(int argc, char * argv[])
auto filter2 = FilterType2::New();

using PeriodicBoundaryType = itk::PeriodicBoundaryCondition<InputImageType2>;
// Test the OverrideBoundaryCondition setting;
filter2->OverrideBoundaryCondition(new PeriodicBoundaryType);
filter2->SetBoundaryCondition(std::make_unique<PeriodicBoundaryType>());

ITK_EXERCISE_BASIC_OBJECT_METHODS(filter2, GradientImageFilter, ImageToImageFilter);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
# Defines the %unique_ptr macro used per instantiation below.
string(APPEND ITK_WRAP_PYTHON_SWIG_EXT "%include <std_unique_ptr.i>\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})

# OverrideBoundaryCondition stores the argument in a unique_ptr member.
# 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"
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,27 @@
boundaryCondition.thisown
), "Python should own a freshly constructed boundary condition"

filt.OverrideBoundaryCondition(boundaryCondition)
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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,8 @@ class ITK_TEMPLATE_EXPORT MorphologyImageFilter : public KernelImageFilter<TInpu
/** n-dimensional Kernel radius. */
using RadiusType = typename KernelType::SizeType;

/** 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)
{
Expand Down
Loading