COMP: Fix cast filter proxy pixel writes (supersedes #6898)

[hjmjohnson]

Supersedes #6898. Same fix, plus the regression test @N-Dekker asked for and the root cause @blowekamp asked about: any subclass of itk::Image — including otb::Image — triggers this, because AccessorFunctorType is inherited from itk::Image rather than redeclared for Self.

Root cause, verified empirically

itk::Image declares AccessorFunctorType = DefaultPixelAccessorFunctor<Self>, where Self is fixed to Image<TPixel, VDim> itself. A subclass that doesn't redeclare this typedef inherits the base class's version, so ImageBufferRange's SupportsDirectPixelAccess check — which compares that typedef against DefaultPixelAccessorFunctor<TImage> for the actual image type — fails for any such subclass. *outputIt then returns a PixelProxy value instead of a real reference, and the unfixed code's non-const reference bind fails to compile for a fixed-size, non-VariableLengthVector output pixel type.

otb::Image is exactly this: a plain, unmodified itk::Image subclass. Confirmed by compiling a minimal local reproducer (a trivial itk::Image subclass, no OTB dependency) against the pre-fix header and getting the identical compiler error reported in #6898, then confirming it compiles clean with the fix restored.

Answering @blowekamp's question directly: the triggering template parameters are itk::CastImageFilter<itk::VectorImage<float,2>, AnySubclassOf<itk::Image<itk::Vector<double,2>,2>>> — matching OTB's otb::VectorImage<float,2> → otb::Image<itk::Vector<double,2>,2> cast in otbStereoRectificationGridGenerator.cxx.

Test added

TestVectorImageCastToImageSubclass() in itkCastImageFilterTest.cxx, modeled on the existing TestVectorImageCast2. Casts a VectorImage<float,2> to a local ImageSubclass<Vector<double,2>,2> and verifies the round-tripped pixel values, not just that it compiles. Passes locally; fails to compile against the pre-fix header with the exact error from #6898.

[greptile-apps]

Confidence Score: 4/5

Not ready to merge: the destination-pixel copy needs correction, and the repository’s comment requirement must be met.

Findings

1.  Uninitialized destination pixel copy ▶
2.  Shorten test comments ▶

Summary

The PR adds pixel write-back for output-image proxies and tests casting vector-image pixels into an image subclass. The new path copies an uninitialized destination pixel before replacing its components, and the new test comments exceed the repository’s prose limit. Both need correction before merging.

_{Reviews (1) · Last reviewed commit: "COMP: Fix cast filter proxy pixel writes"}

[dzenanz]

Mostly looks good.

[hjmjohnson]

Two force-pushes: the first (b50acb500b9 → be90fa639e6) is the GTest conversion requested above, content only; the second (be90fa639e6 → d0b8af9b2b7) is a plain rebase on main with no content change (git range-diff reports =).

[N-Dekker]

Thanks for making it a GTest! 👍 May I still have a look before it is merged? (Just pressing Request changes to get a little bit more time!)

[N-Dekker]

Just for my understanding, the original PR did OutputPixelType outputPixel{ *outputIt }; here. Was the part { *outputIt } intentionally removed?

Probably OK, just wondering if it was intended. (It might make it harder to share code between the if and the else clause.)

[N-Dekker]

the root cause @blowekamp asked about: any subclass of itk::Image — including otb::Image — triggers this, because AccessorFunctorType is inherited from itk::Image rather than redeclared for Self.

Ah, I see now, the range uses a proxy when it cannot use direct pixel access, based on:

  static constexpr bool SupportsDirectPixelAccess =
    std::is_same_v<PixelType, InternalPixelType> &&
    std::is_same_v<typename TImage::AccessorType, DefaultPixelAccessor<PixelType>> &&
    std::is_same_v<AccessorFunctorType, DefaultPixelAccessorFunctor<std::remove_const_t<TImage>>>;

At

ITK/Modules/Core/Common/include/itkImageBufferRange.h

Lines 80 to 87 in 84bb830

	// Tells whether or not this range supports direct pixel access. If it does,
	// iterator::operator*() returns a reference to the internally stored pixel,
	// otherwise iterator::operator*() returns a proxy, which internally uses the
	// AccessorFunctor of the image to access the pixel indirectly.
	static constexpr bool SupportsDirectPixelAccess =
	std::is_same_v<PixelType, InternalPixelType> &&
	std::is_same_v<typename TImage::AccessorType, DefaultPixelAccessor<PixelType>> &&
	std::is_same_v<AccessorFunctorType, DefaultPixelAccessorFunctor<std::remove_const_t<TImage>>>;

Maybe this check on AccessorFunctorType in ImageBufferRange is too strict? AccessorFunctorType should be a template instantiation of DefaultPixelAccessorFunctor<T>, but maybe we should not require that it is exactly DefaultPixelAccessorFunctor<std::remove_const_t<TImage>>?

What if SupportsDirectPixelAccess just checks that AccessorFunctorType == DefaultPixelAccessorFunctor<AccessorFunctorType::ImageType> ?

---

Just checked: it appears that the changes in itkCastImageFilter.hxx are not necessary, in order to make the test pass!!! It is necessary though to fix SupportsDirectPixelAccess in itkImageBufferRange.h, as follows:

  static constexpr bool SupportsDirectPixelAccess =
    std::is_same_v<PixelType, InternalPixelType> &&
    std::is_same_v<typename TImage::AccessorType, DefaultPixelAccessor<PixelType>> &&
    std::is_same_v<AccessorFunctorType, DefaultPixelAccessorFunctor<typename AccessorFunctorType::ImageType>>;

That will make the new test pass successfully, even with the old itkCastImageFilter.hxx!

[N-Dekker]

Update: I'm preparing a PR to let ImageBufferRange and ImageRegionRange do direct pixel access for subclasses of itk::Image. This should solve the compile errors at OrfeoToolBox, reported from by @nilason at #6898, and make it faster!

---

• Update: the PR that I was preparing is here now: ENH: Let Range classes do direct pixel access to itk::Image subclasses #6905

[N-Dekker]

• This comment ("Accesses pixels through a proxy...") should be removed or adjusted when we merge ENH: Let Range classes do direct pixel access to itk::Image subclasses #6905

[N-Dekker]

[nitpick] Please replace these two GetLargestPossibleRegion() calls here with use of the local variable region.

[N-Dekker]

[nitpick] While I like to see small images being used in tests, an 1x3 image seems a bit too exotic to me. Some filters don't even support images smaller than 4x4:

ITK/Modules/Filtering/ImageFilterBase/include/itkRecursiveSeparableImageFilter.hxx

Line 200 in 84bb830

<< " is less than 4. This filter requires a minimum of four pixels along the dimension to be processed.");

So I would prefer an image size of 4x5.

[N-Dekker]

For code readability, I would suggest to add a named constant for the vector length:

    constexpr unsigned int vectorLength{ 2 };
    using OutputImageType = ImageSubclass<itk::Vector<double, vectorLength>, 2>

vectorLength can then also be used to specify the size of VariableLengthVector (further below here). Assuming that it is essential that VariableLengthVector and itk::Vector have the same vector length, of course! Is that assumption correct?

vectorLength can then also be used to check the vector elements in a for loop, as follows:

    for( unsigned int i{}; i < vectorLength; ++i )
    {
      EXPECT_EQ(castedImageIterator.Get()[i], static_cast<double>(originalImageIterator.Get()[i]));
    }

[blowekamp]

See #6905
·N-Dekker for potentially a better fix a the root of the issue.

[N-Dekker]

See #6905 ·N-Dekker for potentially a better fix a the root of the issue.

Thanks @blowekamp ! Note that even if my PR #6905 fixes the problem for subclasses of itk::Image (as I think it does), it may still be useful to have a unit test for ImageSubclass as output of CastImageFilter, like the one presented here by Hans (@hjmjohnson).

I wonder if there are still other use cases that would motivate the proposed modification of CastImageFilter 🤷 I hope not, because I feel that the proposed extra code is a bit complicated.

[hjmjohnson]

I am reading the comment from N-Dekker, that his PR makes this one unnecessary. I could be wrong; perhaps #6905 complements this one? In any case. I'm closing this issue as is. Feel free to take tests or other elements, or take over this PR if there are components that are useful.