Page MenuHomePhabricator

Crash on segmentation threshold [0, 1[
Closed, ResolvedPublic


We use "fa image" with intensity in the range [0.0, 1.0], but most of the time, the range is actually [0.0, 1.0[. For example, the image I use has a max intensity of 0.99999964237213135. The problem is that this value is interpreted as a 1.0 by the QDoubleSpinBox, which make this block crashes in BinaryThresholdTool::ITKThresholdingOldBinary:

typename ThresholdFilterType::Pointer filter = ThresholdFilterType::New();
filter->SetLowerThreshold(thresholdValue); // 1.0
filter->SetUpperThreshold(m_SensibleMaximumThresholdValue); // 0.999999

because m_SensibleMaximumThresholdValue has the real max value of the image but thresholdValue takes its value from the spinbox.

I already fixed it in our in-house version. I'll send a PR soon.

Event Timeline

goch triaged this task as Normal priority.May 22 2017, 5:18 PM
goch added a subscriber: goch.

The fix seems very random to me. Instead of just dividing the lower threshold by two , would it not be better to catch and correct the error at the place mentioned above?

Something similar to:

if(thresholdValue > SensibleMaximumThresholdValue)
  thresholdValue = SensibleMaximumThresholdValue;

I'm with Caspar here. This is a typical floating point precision issue which would preferably be resolved by using something like std::min() and std::max() when assigning lower/upper values.

Hum, looking at it right now, I agree that it looks random :) I know that I tried doing the simple way (like goch's snippet) in the "logical" places but it wasn't working. I don't remember why though so I'll try again and keep you updated.

A new PR:

I do think this is a better fix, so thank you for complaining :) It does look less random.

This being said, you will probably tell me that the condition is not at the place it should be... I know, I tried! I wanted to put it right after

m_CurrentThresholdValue = (m_SensibleMaximumThresholdValue + m_SensibleMinimumThresholdValue) / 2.0;

as any sane person would do :) but it doesn't work!

The difficulty are 1) the UI slider and spinner should be at half value 2) no useless computation 3) don't change the code too much. Here's what currently happen on master:

      UpdatePreview (WOULD CRASH)
        UpdatePreview (NOT AT HALF, IS AT MAX)
      UpdatePreview (WOULD CRASH)

In summary, this is the only place I could find that respects my 3 conditions.

Thanks for the contribution.

Cool. The menus were impossible to use on OS X. However, I suggest that you reopen this issue. I tested master to check if the other problem that was explained in my 3-05-2017 post was solved. It affects all OSes. From my tests, it's still a problem. It might be considered minor but:

  • it's still a bug
  • I already posted a solution 4 months ago, it might be useful to use it

@goyette I assume you meant to add this comment to T23248, T22929, T23985 or most likely T22065? I do not see a connection to this task?

Hahaha, dammit, yes, I meant T22065. Not sure what happened :) I copied my post in the right issue. Thank you.