Merge pull request #1490 from rolalaro/fix_canny

Various fixes for ViSP Canny implementation

modules/core/include/visp3/core/vpCannyEdgeDetection.h

@@ -34,11 +34,16 @@
 // System includes
 #include <map>
 #include <vector>
+#include <iostream>
+#if !defined(_WIN32) && (defined(__unix__) || defined(__unix) || (defined(__APPLE__) && defined(__MACH__))) // UNIX
+#include <sys/resource.h> // To dynamically change the stack size
+#endif
 
 // ViSP include
 #include <visp3/core/vpConfig.h>
 #include <visp3/core/vpImage.h>
 #include <visp3/core/vpImageFilter.h>
+#include <visp3/core/vpRGBa.h>
 
 // 3rd parties include
 #ifdef VISP_HAVE_NLOHMANN_JSON
@@ -50,6 +55,10 @@ BEGIN_VISP_NAMESPACE
  * \brief Class that implements the Canny's edge detector.
  * It is possible to use a boolean mask to ignore some pixels of
  * the input gray-scale image.
+ *
+ * \warning If the stack size is not sufficient, a SEGFAULT can occur on some images which
+ * are really over-exposed.
+ * \note The maximum stack on MacOS seems to be 65532000 bytes, see https://stackoverflow.com/a/13261334
 */
 class VISP_EXPORT vpCannyEdgeDetection
 {
@@ -133,6 +142,8 @@ class VISP_EXPORT vpCannyEdgeDetection
   /**
    * \brief Detect the edges in an image.
    * Convert the color image into a ViSP gray-scale image.
+   * \warning If the stack size is not sufficient, a SEGFAULT can occur on some images which
+   * are really over-exposed.
    *
    * \param[in] cv_I A color image, in OpenCV format.
    * \return vpImage<unsigned char> 255 means an edge, 0 means not an edge.
@@ -143,6 +154,8 @@ class VISP_EXPORT vpCannyEdgeDetection
   /**
    * \brief Detect the edges in an image.
    * Convert the color image into a gray-scale image.
+   * \warning If the stack size is not sufficient, a SEGFAULT can occur on some images which
+   * are really over-exposed.
    *
    * \param[in] I_color : An RGB image, in ViSP format.
    * \return vpImage<unsigned char> 255 means an edge, 0 means not an edge.
@@ -151,6 +164,8 @@ class VISP_EXPORT vpCannyEdgeDetection
 
   /**
    * \brief Detect the edges in a gray-scale image.
+   * \warning If the stack size is not sufficient, a SEGFAULT can occur on some images which
+   * are really over-exposed.
    *
    * \param[in] I : A gray-scale image, in ViSP format.
    * \return vpImage<unsigned char> 255 means an edge, 0 means not an edge.
@@ -261,11 +276,41 @@ class VISP_EXPORT vpCannyEdgeDetection
   }
 
   /**
-   * \brief If set to true, the list of the detected edge-points will be available
-   * calling the method \b vpCannyEdgeDetection::getEdgePointsList().
+   * \brief Set the minimum stack size, expressed in bytes, due to the recursivity of the algorithm.
    *
-   * \param[in] storeEdgePoints The new desired status.
+   * \note The stack size is changed back to its original value after
+   * before leaving the detect() function.
+   * \note On Windows, the minimum stack size is defined at compilation time
+   * and cannot be changed during runtime.
+   * \note The maximum stack on MacOS seems to be 65532000 bytes, see https://stackoverflow.com/a/13261334
+   * \warning If the stack size is not sufficient, a SEGFAULT can occur on some images which
+   * are really over-exposed.
+   *
+   * \param[in] requiredStackSize The required stack size, in bytes.
    */
+#if !defined(_WIN32) && (defined(__unix__) || defined(__unix) || (defined(__APPLE__) && defined(__MACH__))) // UNIX
+  inline void setMinimumStackSize(const rlim_t &requiredStackSize)
+  {
+    m_minStackSize = requiredStackSize;
+  }
+#else
+  inline void setMinimumStackSize(const unsigned int &requiredStackSize)
+  {
+    (void)requiredStackSize;
+    static bool hasNotBeenDisplayed = true;
+    if (hasNotBeenDisplayed) {
+      std::cerr << "setStackSize has no effect on non-POSIX systems. The stack size is defined during compilation." << std::endl;
+      hasNotBeenDisplayed = false;
+    }
+  }
+#endif
+
+/**
+ * \brief If set to true, the list of the detected edge-points will be available
+ * calling the method \b vpCannyEdgeDetection::getEdgePointsList().
+ *
+ * \param[in] storeEdgePoints The new desired status.
+ */
   inline void setStoreEdgePoints(const bool &storeEdgePoints)
   {
     m_storeListEdgePoints = storeEdgePoints;
@@ -286,6 +331,29 @@ class VISP_EXPORT vpCannyEdgeDetection
     }
     return m_edgePointsList;
   }
+
+  /**
+   * \brief Get the minimum stack size used by the algorithm.
+   *
+   * \note On Windows, the minimum stack size is defined at compilation time
+   * and cannot be changed during runtime.
+   *
+   * \return rlim_t The minimum stack size.
+   */
+#if !defined(_WIN32) && (defined(__unix__) || defined(__unix) || (defined(__APPLE__) && defined(__MACH__))) // UNIX
+  inline rlim_t getMinimumStackSize() const
+  {
+    return m_minStackSize;
+  }
+#else
+  inline unsigned int getMinimumStackSize() const
+  {
+    const unsigned int limit = 65532000;
+    return limit;
+  }
+#endif
+
+
   //@}
 private:
   typedef enum EdgeType
@@ -325,6 +393,9 @@ class VISP_EXPORT vpCannyEdgeDetection
                                     must be lower than the upper threshold \b m_upperThreshold.*/
 
   // // Edge tracking attributes
+#if !defined(_WIN32) && (defined(__unix__) || defined(__unix) || (defined(__APPLE__) && defined(__MACH__))) // UNIX
+  rlim_t m_minStackSize; /*!< Minimum stack size, due to the recursivity used in this step of the algorithm.*/
+#endif
   bool m_storeListEdgePoints; /*!< If true, the vector \b m_edgePointsList will contain the list of the edge points resulting from the whole algorithm.*/
   std::map<std::pair<unsigned int, unsigned int>, EdgeType> m_edgePointsCandidates; /*!< Map that contains the strong edge points, i.e. the points for which we know for sure they are edge points,
                                                 and the weak edge points, i.e. the points for which we still must determine if they are actual edge points.*/

modules/core/include/visp3/core/vpImageFilter.h

@@ -332,13 +332,31 @@ class VISP_EXPORT vpImageFilter
                                             const vpImage<OutType> *p_dIx = nullptr, const vpImage<OutType> *p_dIy = nullptr,
                                             const unsigned int &gaussianKernelSize = 5,
                                             const OutType &gaussianStdev = 2.f, const unsigned int &apertureGradient = 3,
-                                            const float &lowerThresholdRatio = 0.6, const float &upperThresholdRatio = 0.8,
+                                            const float &lowerThresholdRatio = 0.6f, const float &upperThresholdRatio = 0.8f,
                                             const vpCannyFilteringAndGradientType &filteringType = CANNY_GBLUR_SOBEL_FILTERING,
                                             const vpImage<bool> *p_mask = nullptr)
   {
     const unsigned int w = I.getWidth();
     const unsigned int h = I.getHeight();
 
+    if ((lowerThresholdRatio <= 0.f) || (lowerThresholdRatio >= 1.f)) {
+      std::stringstream errMsg;
+      errMsg << "Lower ratio (" << lowerThresholdRatio << ") " << (lowerThresholdRatio < 0.f ? "should be greater than 0 !" : "should be lower than 1 !");
+      throw(vpException(vpException::fatalError, errMsg.str()));
+    }
+
+    if ((upperThresholdRatio <= 0.f) || (upperThresholdRatio >= 1.f)) {
+      std::stringstream errMsg;
+      errMsg << "Upper ratio (" << upperThresholdRatio << ") " << (upperThresholdRatio < 0.f ? "should be greater than 0 !" : "should be lower than 1 !");
+      throw(vpException(vpException::fatalError, errMsg.str()));
+    }
+
+    if (lowerThresholdRatio  >= upperThresholdRatio) {
+      std::stringstream errMsg;
+      errMsg << "Lower ratio (" << lowerThresholdRatio << ") should be lower than the upper ratio (" << upperThresholdRatio << ")";
+      throw(vpException(vpException::fatalError, errMsg.str()));
+    }
+
     vpImage<unsigned char> dI(h, w);
     vpImage<OutType> dIx(h, w), dIy(h, w);
     if ((p_dIx != nullptr) && (p_dIy != nullptr)) {
@@ -387,8 +405,14 @@ class VISP_EXPORT vpImageFilter
       }
       ++i;
     }
+    if (notFound) {
+      std::stringstream errMsg;
+      errMsg << "Could not find a bin for which " << upperThresholdRatio * 100.f << " percents of the pixels had a gradient lower than the upper threshold.";
+      throw(vpException(vpException::fatalError, errMsg.str()));
+    }
     float upperThresh = std::max<float>(bon, 1.f);
     lowerThresh = lowerThresholdRatio * bon;
+    lowerThresh = std::max<float>(lowerThresh, std::numeric_limits<float>::epsilon());
     return upperThresh;
   }
 

modules/core/src/image/vpCannyEdgeDetection.cpp

@@ -33,6 +33,10 @@
 
 #include <visp3/core/vpImageConvert.h>
 
+#ifdef VISP_USE_MSVC
+#pragma comment(linker, "/STACK:65532000") // Increase max recursion depth
+#endif
+
 #if (VISP_CXX_STANDARD == VISP_CXX_STANDARD_98) // Check if cxx98
 namespace
 {
@@ -120,6 +124,9 @@ vpCannyEdgeDetection::vpCannyEdgeDetection(const int &gaussianKernelSize, const
   , m_lowerThresholdRatio(lowerThresholdRatio)
   , m_upperThreshold(upperThreshold)
   , m_upperThresholdRatio(upperThresholdRatio)
+#if !defined(_WIN32) && (defined(__unix__) || defined(__unix) || (defined(__APPLE__) && defined(__MACH__))) // UNIX
+  , m_minStackSize(65532000)  // Maximum stack size on MacOS, see https://stackoverflow.com/a/13261334
+#endif
   , m_storeListEdgePoints(storeEdgePoints)
   , mp_mask(nullptr)
 {
@@ -239,7 +246,27 @@ vpCannyEdgeDetection::detect(const vpImage<vpRGBa> &I_color)
 vpImage<unsigned char>
 vpCannyEdgeDetection::detect(const vpImage<unsigned char> &I)
 {
-  // // Clearing the previous results
+#if !defined(_WIN32) && (defined(__unix__) || defined(__unix) || (defined(__APPLE__) && defined(__MACH__))) // UNIX
+  // Increase stack size due to the recursive algorithm
+  rlim_t initialStackSize;
+  struct rlimit rl;
+  int result;
+  result = getrlimit(RLIMIT_STACK, &rl);
+  if (result == 0) {
+    initialStackSize = rl.rlim_cur;
+    if (rl.rlim_cur < m_minStackSize) {
+      rl.rlim_cur = m_minStackSize;
+      result = setrlimit(RLIMIT_STACK, &rl);
+      if (result != 0) {
+        throw(vpException(vpException::fatalError, "setrlimit returned result = %d\n", result));
+      }
+    }
+  }
+  else {
+    throw(vpException(vpException::fatalError, "getrlimit returned result = %d\n", result));
+  }
+#endif
+// // Clearing the previous results
   m_edgeMap.resize(I.getHeight(), I.getWidth(), 0);
   m_edgeCandidateAndGradient.clear();
   m_edgePointsCandidates.clear();
@@ -272,6 +299,18 @@ vpCannyEdgeDetection::detect(const vpImage<unsigned char> &I)
 
   // // Step 5: edge tracking
   performEdgeTracking();
+
+#if !defined(_WIN32) && (defined(__unix__) || defined(__unix) || (defined(__APPLE__) && defined(__MACH__))) // UNIX
+  // Reset stack size to its original value
+  if (rl.rlim_cur > initialStackSize) {
+    rl.rlim_cur = initialStackSize;
+    result = setrlimit(RLIMIT_STACK, &rl);
+    if (result != 0) {
+      throw(vpException(vpException::fatalError, "setrlimit returned result = %d\n", result));
+
+    }
+  }
+#endif
   return m_edgeMap;
 }
 
@@ -501,9 +540,7 @@ vpCannyEdgeDetection::performEdgeTracking()
       }
     }
     else if (it->second == WEAK_EDGE) {
-      if (recursiveSearchForStrongEdge(it->first)) {
-        m_edgeMap[it->first.first][it->first.second] = 255;
-      }
+      recursiveSearchForStrongEdge(it->first);
     }
   }
 }
@@ -565,6 +602,9 @@ vpCannyEdgeDetection::recursiveSearchForStrongEdge(const std::pair<unsigned int,
   if (hasFoundStrongEdge) {
     m_edgePointsCandidates[coordinates] = STRONG_EDGE;
     m_edgeMap[coordinates.first][coordinates.second] = 255;
+    if (m_storeListEdgePoints) {
+      m_edgePointsList.push_back(vpImagePoint(coordinates.first, coordinates.second));
+    }
   }
   return hasFoundStrongEdge;
 }

modules/core/src/image/vpImageFilter_canny.cpp

@@ -203,6 +203,24 @@ float vpImageFilter::computeCannyThreshold(const cv::Mat &cv_I, const cv::Mat *p
                                            const float &lowerThresholdRatio, const float &upperThresholdRatio,
                                            const vpImageFilter::vpCannyFilteringAndGradientType &filteringType)
 {
+  if ((lowerThresholdRatio <= 0.f) || (lowerThresholdRatio >= 1.f)) {
+    std::stringstream errMsg;
+    errMsg << "Lower ratio (" << lowerThresholdRatio << ") " << (lowerThresholdRatio < 0.f ? "should be greater than 0 !" : "should be lower than 1 !");
+    throw(vpException(vpException::fatalError, errMsg.str()));
+  }
+
+  if ((upperThresholdRatio <= 0.f) || (upperThresholdRatio >= 1.f)) {
+    std::stringstream errMsg;
+    errMsg << "Upper ratio (" << upperThresholdRatio << ") " << (upperThresholdRatio < 0.f ? "should be greater than 0 !" : "should be lower than 1 !");
+    throw(vpException(vpException::fatalError, errMsg.str()));
+  }
+
+  if (lowerThresholdRatio  >= upperThresholdRatio) {
+    std::stringstream errMsg;
+    errMsg << "Lower ratio (" << lowerThresholdRatio << ") should be lower than the upper ratio (" << upperThresholdRatio << ")";
+    throw(vpException(vpException::fatalError, errMsg.str()));
+  }
+
   double w = cv_I.cols;
   double h = cv_I.rows;
   int bins = 256;
@@ -236,13 +254,21 @@ float vpImageFilter::computeCannyThreshold(const cv::Mat &cv_I, const cv::Mat *p
   float accu = 0;
   float t = static_cast<float>(upperThresholdRatio * w * h);
   float bon = 0;
-  for (int i = 0; i < bins; ++i) {
+  int i = 0;
+  bool notFound = true;
+  while ((i < bins) && notFound) {
     float tf = hist.at<float>(i);
     accu = accu + tf;
     if (accu > t) {
       bon = static_cast<float>(i);
-      break;
+      notFound = false;
     }
+    ++i;
+  }
+  if (notFound) {
+    std::stringstream errMsg;
+    errMsg << "Could not find a bin for which " << upperThresholdRatio * 100.f << " percents of the pixels had a gradient lower than the upper threshold.";
+    throw(vpException(vpException::fatalError, errMsg.str()));
   }
   float upperThresh = std::max<float>(bon, 1.f);
   lowerThresh = lowerThresholdRatio * bon;