InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.4k stars 664 forks source link

BUG: FFT object factories not registered by default in ITK remote module tests #2950

Closed tbirdso closed 2 years ago

tbirdso commented 2 years ago

Description

FFT image filters were recently updated to allow swappable backends at runtime via object factory methods. Projects consuming ITK and optional remote modules need only include USEITK.cmake to construct relevant factory manager files and initiate factory registration. However, ITK and its remote modules itself do not construct these files. As a result ITK factories are not available by default in ITK and remote module tests and currently must be manually included, which creates an additional burden on remote modules. itk::ImageFileReader and itk::ImageFileWriter typically handle the overhead of registering ImageIO default factories where needed, but no such overlying class exists for FFT filters.

I see three possible paths forward: 1) Implement the proper framework to register default factories for ITK tests (and remote modules). Ideally we would treat ITK tests as the final project consumers of ITK itself and include(UseITK.cmake) for each test driver so that default factories are available in testing, which would most closely mirror the behavior of external projects that use ITK. My understanding is that ITK tests are compiled in a separate step similar to wrappings. I believe this is the best option, although the most intensive. 2) Manually register FFT factories in ITK and remote module tests. This is a stopgap that is already added to ITK FFT tests, and could be extended to remote modules. It requires no new build architecture but is a manual process that requires knowledge of underlying FFT requirements for each class. For instance, see segfault failures in ITKMontage tests where itk::FFTPadImageFilter init failed because no factory was registered for itk::ForwardFFTImageFilter. 3) Introduce an overhead class for FFTs similar to itk::ImageFileReader and itk::ImageFileWriter to handle default factory registrations where required. This is my least favorite option as it introduces more overhead specifically for FFT classes and is not easily extensible to any future classes that we wish to make depend on object factory registrations.

Thank you @dzenanz for performing initial investigation of the issue in ITKMontage and linking it back to FFT changes.

Steps to Reproduce

Reproducing in ITKMontage:

  1. Build ITK with ITKMontage enabled as a remote module, or separately with ITKMontage built against ITK (both in RelWithDebInfo configuration)
  2. Run ctest -C Release -R itkMontagePCMSynthetic_2cc

Expected behavior

Test passes. FFT factories are registered by default.

Actual behavior

itk::PhaseCorrelationOptimizer<...>::New() segfaults. The optimizer attempts to instantiate an itk::FFTPadImageFilter object which tries to get the prime factor member from a new itk::ForwardFFTImageFilter object, which in turn cannot be created from the object factory because no FFT factories have been registered by default. Attempting to access a member of the nullptr causes the segfault.

Reproducibility

100%

Versions

Environment

Linux

Additional Information

tbirdso commented 2 years ago

I can follow up on the FFT stopgap measure for now pending discussion of the path forward here.

@thewtex @dzenanz could you please weigh in on whether it is reasonable to make ITK test driver compilations consume UseITK.cmake for the purpose of properly registering default factories?

dzenanz commented 2 years ago

There is probably some reason this is not done already, so it might be harder than you think. @blowekamp and @hjmjohnson might know more, since they have longer history with ITK than me.

tbirdso commented 2 years ago

Per discussion with @thewtex it sounds like the best path forward is to make ITKTestKernel depend on ITKFFT and run default FFT factory registrations there as we do with the ProcessArgumentsAndRegisterBuiltInFactories function. This is a good middle ground that removes the need for adding explicit factory registrations in external modules while avoiding the re-architecting that may be necessary to allow tests to implicitly depend on any generic factory registration.

tbirdso commented 2 years ago

ITKMontage CDash results show that default factory registration in ITKTestKernel has resolved test failures. Closing this issue.

cc @dzenanz