add mutex to NiftiIO so it doesn't crash under multithreading

tidy up cmake package finding, add sigc++ for mutex (when openmp and qt aren't used)
asserts in BinaryFile for wrong length/position ranges
don't set bad flags on msvc
remove M_PI as msvc doesn't define it

CMakeLists.txt

@@ -11,7 +11,10 @@ PROJECT(CiftiLib)
 
 SET(CIFTILIB_VERSION 1.4)
 
-SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -W -Wall")
+#MSVC seems like the only compiler that chokes on -W -Wall
+IF (NOT MSVC)
+    SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -W -Wall")
+ENDIF (NOT MSVC)
 
 SET(IGNORE_QT FALSE CACHE BOOL "don't try to use QT")
 
@@ -77,7 +80,7 @@ IF (Boost_VERSION LESS 105600)
     ADD_DEFINITIONS(-DCIFTILIB_BOOST_NO_TRY_LEXICAL)
 ENDIF (Boost_VERSION LESS 105600)
 
-#zlib
+#zlib, useful for volume reading
 FIND_PACKAGE(ZLIB)
 IF (ZLIB_FOUND)
     INCLUDE_DIRECTORIES(${ZLIB_INCLUDE_DIRS})
@@ -89,6 +92,12 @@ IF (APPLE)
     ADD_DEFINITIONS(-DCIFTILIB_OS_MACOSX)
 ENDIF (APPLE)
 
+#openmp provides a fast mutex implementation, faster than QT (and probably faster than glibmm)
+FIND_PACKAGE(OpenMP)
+IF (OPENMP_FOUND)
+    SET(CMAKE_CXX_FLAGS "${OpenMP_CXX_FLAGS} ${CMAKE_CXX_FLAGS}")
+ENDIF (OPENMP_FOUND)
+
 ENABLE_TESTING()
 
 #the library source, doesn't contain build targets

cmake/Modules/Findglib.cmake

@@ -4,13 +4,13 @@ FIND_PACKAGE(PkgConfig)
 PKG_CHECK_MODULES(PC_GLIB glib-2.0)
 
 FIND_PATH(glib_INCLUDE_DIR NAMES glib.h HINTS ${PC_GLIB_INCLUDEDIR} ${PC_GLIB_INCLUDE_DIRS})
-FIND_PATH(glibconfig_INCLUDE_DIR NAMES glibconfig.h HINTS ${PC_GLIB_INCLUDEDIR} ${PC_GLIB_INCLUDE_DIRS})
+FIND_PATH(glib_config_INCLUDE_DIR NAMES glibconfig.h HINTS ${PC_GLIB_INCLUDEDIR} ${PC_GLIB_INCLUDE_DIRS})
 FIND_LIBRARY(glib_LIBRARY NAMES glib glib-2.0 HINTS ${PC_GLIB_LIBDIR} ${PC_GLIB_LIBRARY_DIRS})
 
 SET(glib_LIBRARIES ${glib_LIBRARY} ${PC_GLIB_PKGCONF_LIBRARIES})
-SET(glib_INCLUDE_DIRS ${glib_INCLUDE_DIR} ${glibconfig_INCLUDE_DIR} ${PC_GLIB_PKGCONF_INCLUDE_DIRS})
+SET(glib_INCLUDE_DIRS ${glib_INCLUDE_DIR} ${glib_config_INCLUDE_DIR} ${PC_GLIB_PKGCONF_INCLUDE_DIRS})
 
 INCLUDE(FindPackageHandleStandardArgs)
 FIND_PACKAGE_HANDLE_STANDARD_ARGS(glib DEFAULT_MSG glib_LIBRARY glib_INCLUDE_DIR)
 
-MARK_AS_ADVANCED(glib_INCLUDE_DIR glib_LIBRARY)
+MARK_AS_ADVANCED(glib_INCLUDE_DIR glib_config_INCLUDE_DIR glib_LIBRARY)

cmake/Modules/Findglibmm.cmake

@@ -1,8 +1,10 @@
 
 IF(glibmm_FIND_REQUIRED)
     FIND_PACKAGE(glib REQUIRED)
+    FIND_PACKAGE(sigc++ REQUIRED)
 ELSE(glibmm_FIND_REQUIRED)
     FIND_PACKAGE(glib)
+    FIND_PACKAGE(sigc++)
 ENDIF(glibmm_FIND_REQUIRED)
 
 IF(GLIB_FOUND)
@@ -12,15 +14,15 @@ IF(GLIB_FOUND)
     PKG_CHECK_MODULES(PC_GLIBMM glibmm-2.4)
 
     FIND_PATH(glibmm_INCLUDE_DIR NAMES glibmm/main.h HINTS ${PC_GLIBMM_INCLUDEDIR} ${PC_GLIBMM_INCLUDE_DIRS})
-    FIND_PATH(glibmmconfig_INCLUDE_DIR NAMES glibmmconfig.h HINTS ${PC_GLIBMM_INCLUDEDIR} ${PC_GLIBMM_INCLUDE_DIRS})
+    FIND_PATH(glibmm_config_INCLUDE_DIR NAMES glibmmconfig.h HINTS ${PC_GLIBMM_INCLUDEDIR} ${PC_GLIBMM_INCLUDE_DIRS})
     FIND_LIBRARY(glibmm_LIBRARY NAMES glibmm glibmm-2.4 HINTS ${PC_GLIBMM_LIBDIR} ${PC_GLIBMM_LIBRARY_DIRS})
 
-    SET(glibmm_LIBRARIES ${glibmm_LIBRARY} ${PC_GLIBMM_PKGCONF_LIBRARIES} ${glib_LIBRARIES})
-    SET(glibmm_INCLUDE_DIRS ${glibmm_INCLUDE_DIR} ${glibmmconfig_INCLUDE_DIR} ${PC_GLIBMM_PKGCONF_INCLUDE_DIRS} ${glib_INCLUDE_DIRS})
+    SET(glibmm_LIBRARIES ${glibmm_LIBRARY} ${PC_GLIBMM_PKGCONF_LIBRARIES} ${glib_LIBRARIES} ${sigc++_LIBRARIES})
+    SET(glibmm_INCLUDE_DIRS ${glibmm_INCLUDE_DIR} ${glibmm_config_INCLUDE_DIR} ${PC_GLIBMM_PKGCONF_INCLUDE_DIRS} ${glib_INCLUDE_DIRS} ${sigc++_INCLUDE_DIRS})
 
 ENDIF(GLIB_FOUND)
 
 INCLUDE(FindPackageHandleStandardArgs)
 FIND_PACKAGE_HANDLE_STANDARD_ARGS(glibmm DEFAULT_MSG glibmm_LIBRARY glibmm_INCLUDE_DIR)
 
-MARK_AS_ADVANCED(glibmm_INCLUDE_DIR glibmm_LIBRARY)
+MARK_AS_ADVANCED(glibmm_INCLUDE_DIR glibmm_config_INCLUDE_DIR glibmm_LIBRARY)

cmake/Modules/Findlibxml++.cmake

@@ -14,19 +14,19 @@ IF(GLIBMM_FOUND AND LIBXML2_FOUND)
     PKG_CHECK_MODULES(PC_LIBXMLPP QUIET libxml++-2.6)
 
     FIND_PATH(libxml++_INCLUDE_DIR NAMES libxml++/libxml++.h HINTS ${PC_LIBXMLPP_INCLUDEDIR} ${PC_LIBXMLPP_INCLUDE_DIRS})
-    FIND_PATH(libxml++config_INCLUDE_DIR NAMES libxml++config.h HINTS ${PC_LIBXMLPP_INCLUDEDIR} ${PC_LIBXMLPP_INCLUDE_DIRS})
+    FIND_PATH(libxml++_config_INCLUDE_DIR NAMES libxml++config.h HINTS ${PC_LIBXMLPP_INCLUDEDIR} ${PC_LIBXMLPP_INCLUDE_DIRS})
     FIND_LIBRARY(libxml++_LIBRARY NAMES xml++ xml++-2.6 HINTS ${PC_LIBXMLPP_LIBDIR} ${PC_LIBXMLPP_LIBRARY_DIRS})
 
     SET(libxml++_LIBRARIES ${libxml++_LIBRARY} ${PC_LIBXMLPP_PKGCONF_LIBRARIES} ${glibmm_LIBRARIES} ${LIBXML2_LIBRARIES})
-IF(libxml++config_INCLUDE_DIR)
-    SET(libxml++_INCLUDE_DIRS ${libxml++_INCLUDE_DIR} ${PC_LIBXMLPP_PKGCONF_INCLUDE_DIRS} ${libxml++config_INCLUDE_DIR} ${glibmm_INCLUDE_DIRS} ${LIBXML2_INCLUDE_DIR})
-ELSE(libxml++config_INCLUDE_DIR)
+IF(libxml++_config_INCLUDE_DIR)
+    SET(libxml++_INCLUDE_DIRS ${libxml++_INCLUDE_DIR} ${PC_LIBXMLPP_PKGCONF_INCLUDE_DIRS} ${libxml++_config_INCLUDE_DIR} ${glibmm_INCLUDE_DIRS} ${LIBXML2_INCLUDE_DIR})
+ELSE(libxml++_config_INCLUDE_DIR)
     SET(libxml++_INCLUDE_DIRS ${libxml++_INCLUDE_DIR} ${PC_LIBXMLPP_PKGCONF_INCLUDE_DIRS} ${glibmm_INCLUDE_DIRS} ${LIBXML2_INCLUDE_DIR})
-ENDIF(libxml++config_INCLUDE_DIR)
+ENDIF(libxml++_config_INCLUDE_DIR)
 
 ENDIF(GLIBMM_FOUND AND LIBXML2_FOUND)
 
 INCLUDE(FindPackageHandleStandardArgs)
 FIND_PACKAGE_HANDLE_STANDARD_ARGS(libxml++ DEFAULT_MSG libxml++_LIBRARY libxml++_INCLUDE_DIR)
 
-MARK_AS_ADVANCED(libxml++_INCLUDE_DIR libxml++_LIBRARY)
+MARK_AS_ADVANCED(libxml++_INCLUDE_DIR libxml++_config_INCLUDE_DIR libxml++_LIBRARY)

cmake/Modules/Findsigc++.cmake

@@ -0,0 +1,16 @@
+
+#use pkg-config
+FIND_PACKAGE(PkgConfig)
+PKG_CHECK_MODULES(PC_SIGCXX sigc++-2.0)
+
+FIND_PATH(sigc++_INCLUDE_DIR NAMES sigc++/sigc++.h HINTS ${PC_SIGCXX_INCLUDEDIR} ${PC_SIGCXX_INCLUDE_DIRS})
+FIND_PATH(sigc++_config_INCLUDE_DIR NAMES sigc++config.h HINTS ${PC_SIGCXX_INCLUDEDIR} ${PC_SIGCXX_INCLUDE_DIRS})
+FIND_LIBRARY(sigc++_LIBRARY NAMES sigc sigc-2.0 HINTS ${PC_SIGCXX_LIBDIR} ${PC_SIGCXX_LIBRARY_DIRS})
+
+SET(sigc++_LIBRARIES ${sigc++_LIBRARY} ${PC_SIGCXX_PKGCONF_LIBRARIES})
+SET(sigc++_INCLUDE_DIRS ${sigc++_INCLUDE_DIR} ${sigc++_config_INCLUDE_DIR} ${PC_SIGCXX_PKGCONF_INCLUDE_DIRS})
+
+INCLUDE(FindPackageHandleStandardArgs)
+FIND_PACKAGE_HANDLE_STANDARD_ARGS(sigc++ DEFAULT_MSG sigc++_LIBRARY sigc++_INCLUDE_DIR)
+
+MARK_AS_ADVANCED(sigc++_INCLUDE_DIR sigc++_config_INCLUDE_DIR sigc++_LIBRARY)

src/Common/BinaryFile.cxx

@@ -168,12 +168,14 @@ void BinaryFile::open(const AString& filename, const OpenMode& opmode)
 
 void BinaryFile::read(void* dataOut, const int64_t& count, int64_t* numRead)
 {
+    CiftiAssert(count >= 0);//not sure about allowing 0
     if (!getOpenForRead()) throw CiftiException("file is not open for reading");
     m_impl->read(dataOut, count, numRead);
 }
 
 void BinaryFile::seek(const int64_t& position)
 {
+    CiftiAssert(position >= 0);
     if (m_curMode == NONE) throw CiftiException("file is not open, can't seek");
     m_impl->seek(position);
 }
@@ -186,6 +188,7 @@ int64_t BinaryFile::pos()
 
 void BinaryFile::write(const void* dataIn, const int64_t& count)
 {
+    CiftiAssert(count >= 0);//not sure about allowing 0
     if (!getOpenForWrite()) throw CiftiException("file is not open for writing");
     m_impl->write(dataIn, count);
 }
@@ -247,7 +250,7 @@ void ZFileImpl::read(void* dataOut, const int64_t& count, int64_t* numRead)
     while (totalRead < count)
     {
         int64_t iterSize = min(count - totalRead, CHUNK_SIZE);
-        readret = gzread(m_zfile, ((uint8_t*)dataOut) + totalRead, iterSize);
+        readret = gzread(m_zfile, ((char*)dataOut) + totalRead, iterSize);
         if (readret < 1) break;//0 or -1 indicate eof or error
         totalRead += readret;
     }
@@ -292,7 +295,7 @@ void ZFileImpl::write(const void* dataIn, const int64_t& count)
     while (totalWritten < count)
     {
         int64_t iterSize = min(count - totalWritten, CHUNK_SIZE);
-        int writeret = gzwrite(m_zfile, ((uint8_t*)dataIn) + totalWritten, iterSize);
+        int writeret = gzwrite(m_zfile, ((const char*)dataIn) + totalWritten, iterSize);
         if (writeret < 1) break;//0 or -1 indicate eof or error
         totalWritten += writeret;
     }

src/Common/CMakeLists.txt

@@ -7,6 +7,7 @@ ByteSwapping.h
 CiftiAssert.h
 BinaryFile.h
 CiftiException.h
+CiftiMutex.h
 Compact3DLookup.h
 CompactLookup.h
 FloatMatrix.h

src/Common/CiftiMutex.h

@@ -0,0 +1,105 @@
+#ifndef __CIFTI_MUTEX_H__
+#define __CIFTI_MUTEX_H__
+
+/*LICENSE_START*/ 
+/*
+ *  Copyright (c) 2016, Washington University School of Medicine
+ *  All rights reserved.
+ *
+ *  Redistribution and use in source and binary forms, with or without modification,
+ *  are permitted provided that the following conditions are met:
+ *
+ *  1. Redistributions of source code must retain the above copyright notice,
+ *  this list of conditions and the following disclaimer.
+ *
+ *  2. Redistributions in binary form must reproduce the above copyright notice,
+ *  this list of conditions and the following disclaimer in the documentation
+ *  and/or other materials provided with the distribution.
+ *
+ *  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ *  AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ *  THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ *  ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
+ *  FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ *  (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ *  LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ *  ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
+ *  EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifdef _OPENMP
+#define __CIFTI_MUTEX_H_HAVE_IMPL__
+
+#include "omp.h"
+
+namespace cifti
+{
+    class CiftiMutex
+    {
+        omp_lock_t m_lock;
+    public:
+        CiftiMutex(const CiftiMutex&) { omp_init_lock(&m_lock); };//allow copy, assign, but make them do nothing other than default construct
+        CiftiMutex& operator=(const CiftiMutex&) { return *this; };
+        CiftiMutex() { omp_init_lock(&m_lock); }
+        ~CiftiMutex() { omp_destroy_lock(&m_lock); }
+        friend class CiftiMutexLocker;
+    };
+
+    class CiftiMutexLocker
+    {
+        CiftiMutex* m_mutex;
+        CiftiMutexLocker();//disallow default construction, assign
+        CiftiMutexLocker& operator=(const CiftiMutexLocker& rhs);
+    public:
+        CiftiMutexLocker(CiftiMutex* mutex) { m_mutex = mutex; omp_set_lock(&(m_mutex->m_lock)); }
+        ~CiftiMutexLocker() { omp_unset_lock(&(m_mutex->m_lock)); }
+    };
+}
+
+#else //_OPENMP
+
+#ifdef CIFTILIB_USE_QT
+#define __CIFTI_MUTEX_H_HAVE_IMPL__
+
+#include <QMutex>
+
+namespace cifti
+{
+    typedef QMutex CiftiMutex;
+    typedef QMutexLocker CiftiMutexLocker;
+}
+
+#endif //CIFTILIB_USE_QT
+
+#ifdef CIFTILIB_USE_XMLPP
+#define __CIFTI_MUTEX_H_HAVE_IMPL__
+
+#include <glibmm/thread.h>
+
+namespace cifti
+{
+    typedef Glib::Mutex CiftiMutex;
+
+    //API difference: glib's locker class takes a reference, while QT's takes a pointer
+    class CiftiMutexLocker
+    {
+        CiftiMutex* m_mutex;
+        CiftiMutexLocker();//disallow default construction, assign
+        CiftiMutexLocker& operator=(const CiftiMutexLocker& rhs);
+    public:
+        CiftiMutexLocker(CiftiMutex* mutex) { m_mutex = mutex; m_mutex->lock(); }
+        ~CiftiMutexLocker() { m_mutex->unlock(); }
+    };
+}
+
+#endif //CIFTILIB_USE_XMLPP
+
+#endif //_OPENMP
+
+
+#ifndef __CIFTI_MUTEX_H_HAVE_IMPL__
+#error "you must have openmp support, or define either CIFTILIB_USE_QT or CIFTILIB_USE_XMLPP to select what mutex implementation to use"
+#endif
+
+#endif //__CIFTI_MUTEX_H__

src/Common/MathFunctions.cxx

@@ -1320,34 +1320,6 @@ MathFunctions::clamp(
     return MathFunctions::limitRange(value, minimum, maximum);
 }
 
-/**
- * convert degrees to radians.
- * @param 
- *    degrees value converted to radians.
- * @return
- *    the corresponding radians value.
- */
-float 
-MathFunctions::toRadians(float degrees)
-{
-    float radians = degrees * (M_PI / 180.0f);
-    return radians;
-}
-
-/**
- * convert radians to degrees.
- * @param 
- *    radians value converted to degrees.
- * @return
- *    the corresponding degrees value.
- */
-float 
-MathFunctions::toDegrees(float radians)
-{
-    float degrees = radians * (180.0f / M_PI);
-    return degrees;
-}
-
 /**
  * Distance SQUARED from (x1, y1) to (x2, y2)
  * @param X-coordinate of first point.

src/Common/MathFunctions.h

@@ -286,25 +286,9 @@ class MathFunctions {
                     const float minimum,
                     const float maximum);
 
-    static float toRadians(float angle);
-
-    static float toDegrees(float radians);
-
     ///greatest common divisor
     static uint32_t gcd(uint32_t num1, uint32_t num2);
 
-    /**
-     * Is the value very, very close to zero?
-     * @param value
-     *    Value to test.
-     * @return true if approximately zero, else false.
-     */
-    static inline bool isZero(const float value) {
-        if (value >  0.00001) return false;
-        if (value < -0.00001) return false;
-        return true;
-    }
-
     ///convert quaternion to rotation matrix
     static void quaternToMatrix(const float cijk[4], float matrix[3][3]);
 

src/NiftiIO.h

@@ -33,6 +33,7 @@
 #include "Common/ByteSwapping.h"
 #include "Common/BinaryFile.h"
 #include "Common/CiftiException.h"
+#include "Common/CiftiMutex.h"
 #include "Nifti/NiftiHeader.h"
 
 //include MultiDimIterator from a private include directory, in case people want to use it with NiftiIO
@@ -51,6 +52,7 @@ namespace cifti
         NiftiHeader m_header;
         std::vector<int64_t> m_dims;
         std::vector<char> m_scratch;//scratch memory for byteswapping, type conversion, etc
+        CiftiMutex m_mutex;
         int numBytesPerElem();//for resizing scratch
         template<typename TO, typename FROM>
         void convertRead(TO* out, FROM* in, const int64_t& count);//for reading from file
@@ -96,6 +98,9 @@ namespace cifti
             numSkip += indexSelect[curDim - fullDims] * numDimSkip;
             numDimSkip *= m_dims[curDim];
         }
+        CiftiMutexLocker locked(&m_mutex);//protect starting with resizing until we are done converting, because we use an internal variable for scratch space
+        //we can't guarantee that the output memory is enough to use as scratch space, as we might be doing a narrowing conversion
+        //we are doing FILE ACCESS, so cpu performance isn't really something to worry about
         m_scratch.resize(numElems * numBytesPerElem());
         m_file.seek(numSkip * numBytesPerElem() + m_header.getDataOffset());
         int64_t numRead = 0;
@@ -171,6 +176,8 @@ namespace cifti
             numSkip += indexSelect[curDim - fullDims] * numDimSkip;
             numDimSkip *= m_dims[curDim];
         }
+        CiftiMutexLocker locked(&m_mutex);//protect starting with resizing until we are done writing, because we use an internal variable for scratch space
+        //we are doing FILE ACCESS, so cpu performance isn't really something to worry about
         m_scratch.resize(numElems * numBytesPerElem());
         m_file.seek(numSkip * numBytesPerElem() + m_header.getDataOffset());
         switch (m_header.getDataType())