From 8ad276aac7d3f2f253a6cb47068e455f7f8c4ed8 Mon Sep 17 00:00:00 2001 From: Tim Coalson Date: Fri, 29 Jul 2016 19:32:21 -0500 Subject: [PATCH] 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 | 13 +++- cmake/Modules/Findglib.cmake | 6 +- cmake/Modules/Findglibmm.cmake | 10 +-- cmake/Modules/Findlibxml++.cmake | 12 ++-- cmake/Modules/Findsigc++.cmake | 16 +++++ src/Common/BinaryFile.cxx | 7 ++- src/Common/CMakeLists.txt | 1 + src/Common/CiftiMutex.h | 105 +++++++++++++++++++++++++++++++ src/Common/MathFunctions.cxx | 28 --------- src/Common/MathFunctions.h | 16 ----- src/NiftiIO.h | 7 +++ 11 files changed, 160 insertions(+), 61 deletions(-) create mode 100644 cmake/Modules/Findsigc++.cmake create mode 100644 src/Common/CiftiMutex.h diff --git a/CMakeLists.txt b/CMakeLists.txt index eba994a..4d373b7 100644 --- a/CMakeLists.txt +++ b/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 diff --git a/cmake/Modules/Findglib.cmake b/cmake/Modules/Findglib.cmake index fd720de..725586e 100644 --- a/cmake/Modules/Findglib.cmake +++ b/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) diff --git a/cmake/Modules/Findglibmm.cmake b/cmake/Modules/Findglibmm.cmake index 624da18..c9b9bef 100644 --- a/cmake/Modules/Findglibmm.cmake +++ b/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) diff --git a/cmake/Modules/Findlibxml++.cmake b/cmake/Modules/Findlibxml++.cmake index 4420a21..1a80939 100644 --- a/cmake/Modules/Findlibxml++.cmake +++ b/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) diff --git a/cmake/Modules/Findsigc++.cmake b/cmake/Modules/Findsigc++.cmake new file mode 100644 index 0000000..64ed168 --- /dev/null +++ b/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) diff --git a/src/Common/BinaryFile.cxx b/src/Common/BinaryFile.cxx index 809f7b0..1c234d7 100644 --- a/src/Common/BinaryFile.cxx +++ b/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; } diff --git a/src/Common/CMakeLists.txt b/src/Common/CMakeLists.txt index 40255e2..1131092 100644 --- a/src/Common/CMakeLists.txt +++ b/src/Common/CMakeLists.txt @@ -7,6 +7,7 @@ ByteSwapping.h CiftiAssert.h BinaryFile.h CiftiException.h +CiftiMutex.h Compact3DLookup.h CompactLookup.h FloatMatrix.h diff --git a/src/Common/CiftiMutex.h b/src/Common/CiftiMutex.h new file mode 100644 index 0000000..5a6e357 --- /dev/null +++ b/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 + +namespace cifti +{ + typedef QMutex CiftiMutex; + typedef QMutexLocker CiftiMutexLocker; +} + +#endif //CIFTILIB_USE_QT + +#ifdef CIFTILIB_USE_XMLPP +#define __CIFTI_MUTEX_H_HAVE_IMPL__ + +#include + +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__ diff --git a/src/Common/MathFunctions.cxx b/src/Common/MathFunctions.cxx index 017e775..9e8a352 100644 --- a/src/Common/MathFunctions.cxx +++ b/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. diff --git a/src/Common/MathFunctions.h b/src/Common/MathFunctions.h index 39f5038..b7bd766 100644 --- a/src/Common/MathFunctions.h +++ b/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]); diff --git a/src/NiftiIO.h b/src/NiftiIO.h index cdf3c1a..509713a 100644 --- a/src/NiftiIO.h +++ b/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 m_dims; std::vector m_scratch;//scratch memory for byteswapping, type conversion, etc + CiftiMutex m_mutex; int numBytesPerElem();//for resizing scratch template 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())