itk::WatershedImageFilter crashes (assertion error) on certain inputs

[brad-t-moore]

Description

itk::WatershedImageFilter crashes (assertion error) on certain inputs. This will cause the Python kernel to crash or a C++ program to terminate.

Steps to Reproduce

int itkWatershedImageFilterBadValuesTest(int argc, char* argv[]) {
    int result = 0;

    char* path = argv[1];
    std::cout << "The file path is: " << path << std::endl;

    using ImageType = itk::Image< float, 2>;
    auto reader = itk::ImageFileReader< ImageType >::New();
    reader->SetFileName(path);
    auto img = reader->GetOutput();

    auto watershed = itk::WatershedImageFilter< ImageType >::New();
    watershed->SetLevel(1);
    watershed->SetThreshold(0.1);
    watershed->SetInput(img);
    watershed->Update();

    std::cout << "Got through here" << std::endl;

    watershed->SetLevel(3);
    watershed->SetThreshold(10);

    try {
        watershed->Update();
    }
    catch (itk::ExceptionObject &e) {
        e.Print(std::cout);
    }
    catch (char* e) {
        std::cout << "char* exception: " << e << std::endl;
    }
    catch (...) {
        std::cout << "Failed by exception!";
        result = -1;
    }

    return result;
}

floatmax1ultrasoundeye.zip

Expected behavior

Watershed was part of a image processing pipeline I was numerically optimizing. The inputs are likely poor for the image it's segmenting, but I would expect an exception (at the worst) or simply a single label image.

Actual behavior

for (segment_ptr = segments->Begin(); segment_ptr != segments->End(); ++segment_ptr)
  {
    labelFROM = (*segment_ptr).first;

    // Must take into account any equivalencies that have already been
    // recorded.
    labelTO = m_MergedSegmentsTable->RecursiveLookup((*segment_ptr).second.edge_list.front().label);   <--------- ERROR
    while (labelTO == labelFROM) // Pop off any bogus merges with ourself
    {                            // that may have been left in this list.
      (*segment_ptr).second.edge_list.pop_front();
      labelTO = m_MergedSegmentsTable->RecursiveLookup((*segment_ptr).second.edge_list.front().label);
    }

![image](https://user-images.githubusercontent.com/59569148/93356501-3e32c780-f80d-11ea-843c-f9fd716dd325.png)

Reproducibility

Versions

Environment

Additional Information

[brad-t-moore]

I'm confirming whether the example crashes on FloodLevel < 1.0

[dzenanz]

Did you close accidentally or on purpose?

[brad-t-moore]

I'm trying to see if it is a real issue. The parameters in my test were out of bounds for the documentation.

On Wed, Sep 16, 2020, 3:20 PM Dženan Zukić notifications@github.com wrote:

Did you close accidentally or on purpose?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/InsightSoftwareConsortium/ITK/issues/2007#issuecomment-693608993, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOGPH7F7FNZAAXYBISJG3CLSGEFWJANCNFSM4RO7QC2A .

[brad-t-moore]

There's a definitely an issue. I think the above is the key part, but I'm verifying again. I'm running an optimizer and the values are clamped between 0 and 0.9 which according to the documentation should be valid for level and threshold.

It might that there's still something where level < threshold or vice versa and maybe that's not a correct input, but given that the filter throws an exception for other reasons I'll likely add another exception to the list iterator above. Note, it still currently causes an assertion error in the list.

By the way, any thoughts on ImageFilters throwing exceptions? Wouldn't it be better to simply return a degenerate labeling (all background or foreground)?

[brad-t-moore]

I'm rerunning my optimization code, but clamping threshold to a percentage of level (forcing threshold < level). That might have been erroneous input. Regardless, I'm going to propose submitting the following:

    labelTO = m_MergedSegmentsTable->RecursiveLookup((*segment_ptr).second.edge_list.front().label);
    while (labelTO == labelFROM) // Pop off any bogus merges with ourself
    {                            // that may have been left in this list.
      (*segment_ptr).second.edge_list.pop_front();
      labelTO = m_MergedSegmentsTable->RecursiveLookup((*segment_ptr).second.edge_list.front().label);
    }

to this:

    if (segment_ptr->second.edge_list.empty())
    {
      itkGenericExceptionMacro(<< "CompileMergeList:: An unexpected and fatal error has occurred.");
    }
    labelTO = m_MergedSegmentsTable->RecursiveLookup((*segment_ptr).second.edge_list.front().label);
    while (labelTO == labelFROM) // Pop off any bogus merges with ourself
    {                            // that may have been left in this list.
      (*segment_ptr).second.edge_list.pop_front();
      labelTO = m_MergedSegmentsTable->RecursiveLookup((*segment_ptr).second.edge_list.front().label);
    }

[dzenanz]

Please turn it into a PR.

[brad-t-moore]

Will do.

[brad-t-moore]

@dzenanz @thewtex

I had it fail on another input with the threshold less than the level. Let me know if that is concerning or not? I think my fix will stop the crash, but does it make sense that this throws exceptions so often? It also throws exceptions in other places which I'm currently just catching and considering a failed segmentation.

[dzenanz]

Filters are allowed to throw exceptions. That is better than failing silently. With an exception it is clear where the problem is. Re-throwing is an option, I guess. Maybe @blowekamp has an opinion.

[blowekamp]

I moved to the MophologicalWatershed filter due to stability and reproducibility issues that were never tracked down in this filter.

Throwing exceptions is certainly preferred to segfault or assert failure for conditions occur without precondition violations.

If "threshold < level" should be a hard precondition it can be verified early in and overloaded ProcessObject::VerifyPreconditions method to throw an exception when violated.

[brad-t-moore]

@blowekamp That's good to know. I think a review of this implementation could be warranted. For the PR, I'll just fix the assert failure. Unfortunately, it looks like the exceptions are not predictable from the input data. I had another failure with a normal image and threshold > level. Without digging into the actual algorithm we're using here, I'm not convinced why this isn't returning zero labels or a single label instead of throwing an exception.

[brad-t-moore]

Updating the PR - I think the unused argc in the test driver is causing OS X to fail.