From debd02e3cb441e34f5b63bbfbf69e404f8135159 Mon Sep 17 00:00:00 2001 From: dirk Date: Mon, 29 Dec 2014 21:34:46 +0000 Subject: [PATCH] Re-factored locking and BlobRef/ImageRef classes. --- Magick++/lib/Blob.cpp | 77 +++++--------------------- Magick++/lib/BlobRef.cpp | 40 +++++++++++--- Magick++/lib/Image.cpp | 95 +++++++------------------------- Magick++/lib/ImageRef.cpp | 92 ++++++++++++++++++++++++++----- Magick++/lib/Magick++/BlobRef.h | 16 ++++-- Magick++/lib/Magick++/ImageRef.h | 34 +++++++++--- Magick++/lib/Magick++/Thread.h | 47 +--------------- Magick++/lib/Thread.cpp | 10 ++-- 8 files changed, 188 insertions(+), 223 deletions(-) diff --git a/Magick++/lib/Blob.cpp b/Magick++/lib/Blob.cpp index e890094bf..50b23eef7 100644 --- a/Magick++/lib/Blob.cpp +++ b/Magick++/lib/Blob.cpp @@ -28,51 +28,25 @@ Magick::Blob::Blob(const Magick::Blob& blob_) : _blobRef(blob_._blobRef) { // Increase reference count - Lock(&_blobRef->mutexLock); - ++_blobRef->refCount; + _blobRef->increase(); } Magick::Blob::~Blob() { - bool - doDelete; + if (_blobRef->decrease() == 0) + delete _blobRef; - doDelete=false; - { - Lock(&_blobRef->mutexLock); - if (--_blobRef->refCount == 0) - doDelete=true; - } - - if (doDelete) - { - // Delete old blob reference with associated data - delete _blobRef; - } - _blobRef=0; + _blobRef=(Magick::BlobRef *) NULL; } Magick::Blob& Magick::Blob::operator=(const Magick::Blob& blob_) { - bool - doDelete; - if (this != &blob_) { - { - Lock(&blob_._blobRef->mutexLock); - ++blob_._blobRef->refCount; - } - doDelete=false; - { - Lock(&_blobRef->mutexLock); - if (--_blobRef->refCount == 0) - doDelete=true; - } - if (doDelete) - { - delete _blobRef; - } + blob_._blobRef->increase(); + if (_blobRef->decrease() == 0) + delete _blobRef; + _blobRef=blob_._blobRef; } return(*this); @@ -130,20 +104,8 @@ size_t Magick::Blob::length(void) const void Magick::Blob::update(const void* data_,size_t length_) { - bool - doDelete; - - doDelete=false; - { - Lock(&_blobRef->mutexLock); - if (--_blobRef->refCount == 0) - doDelete=true; - } - if (doDelete) - { - // Delete old blob reference with associated data - delete _blobRef; - } + if (_blobRef->decrease() == 0) + delete _blobRef; _blobRef=new Magick::BlobRef(data_,length_); } @@ -151,21 +113,10 @@ void Magick::Blob::update(const void* data_,size_t length_) void Magick::Blob::updateNoCopy(void* data_,size_t length_, Magick::Blob::Allocator allocator_) { - bool - doDelete; - - doDelete=false; - { - Lock(&_blobRef->mutexLock); - if (--_blobRef->refCount == 0) - doDelete=true; - } - if (doDelete) - { - // Delete old blob reference with associated data - delete _blobRef; - } - _blobRef=new Magick::BlobRef(0,0); + if (_blobRef->decrease() == 0) + delete _blobRef; + + _blobRef=new Magick::BlobRef((const void*) NULL,0); _blobRef->data=data_; _blobRef->length=length_; _blobRef->allocator=allocator_; diff --git a/Magick++/lib/BlobRef.cpp b/Magick++/lib/BlobRef.cpp index 58bc18413..ac8bb4f10 100644 --- a/Magick++/lib/BlobRef.cpp +++ b/Magick++/lib/BlobRef.cpp @@ -9,22 +9,23 @@ #define MAGICK_PLUSPLUS_IMPLEMENTATION 1 #include "Magick++/Include.h" -#include "Magick++/Thread.h" #include "Magick++/BlobRef.h" +#include "Magick++/Exception.h" +#include "Magick++/Thread.h" #include Magick::BlobRef::BlobRef(const void* data_,size_t length_) - : data(0), + : allocator(Magick::Blob::NewAllocator), length(length_), - allocator(Magick::Blob::NewAllocator), - refCount(1), - mutexLock() + data((void*) NULL), + _mutexLock(), + _refCount(1) { - if (data_) + if (data_ != (const void*) NULL) { data=new unsigned char[length_]; - memcpy(data,data_, length_); + memcpy(data,data_,length_); } } @@ -33,10 +34,31 @@ Magick::BlobRef::~BlobRef(void) if (allocator == Magick::Blob::NewAllocator) { delete[] static_cast(data); - data=0; + data=(void *) NULL; } else if (allocator == Magick::Blob::MallocAllocator) + data=(void *) RelinquishMagickMemory(data); +} + +size_t Magick::BlobRef::decrease() +{ + size_t + count; + + _mutexLock.lock(); + if (_refCount == 0) { - data=(void *) RelinquishMagickMemory(data); + _mutexLock.unlock(); + throwExceptionExplicit(OptionError,"Invalid call to decrease"); } + count=--_refCount; + _mutexLock.unlock(); + return(count); +} + +void Magick::BlobRef::increase() +{ + _mutexLock.lock(); + _refCount++; + _mutexLock.unlock(); } diff --git a/Magick++/lib/Image.cpp b/Magick++/lib/Image.cpp index 22cd5e620..2cb436033 100644 --- a/Magick++/lib/Image.cpp +++ b/Magick++/lib/Image.cpp @@ -210,10 +210,7 @@ Magick::Image::Image(const Geometry &size_,const Color &color_) Magick::Image::Image(const Image &image_) : _imgRef(image_._imgRef) { - Lock(&_imgRef->_mutexLock); - - // Increase reference count - ++_imgRef->_refCount; + _imgRef->increase(); } Magick::Image::Image(const size_t width_,const size_t height_, @@ -258,49 +255,23 @@ Magick::Image::Image(const std::string &imageSpec_) Magick::Image::~Image() { - bool - doDelete=false; - - { - Lock(&_imgRef->_mutexLock); - if (--_imgRef->_refCount == 0) - doDelete=true; - } - - if (doDelete) + if (_imgRef->decrease() == 0) delete _imgRef; - _imgRef=0; + _imgRef=(Magick::ImageRef *) NULL; } Magick::Image& Magick::Image::operator=(const Magick::Image &image_) { - if(this != &image_) + if (this != &image_) { - bool - doDelete=false; - - { - Lock(&image_._imgRef->_mutexLock); - ++image_._imgRef->_refCount; - } + image_._imgRef->increase(); + if (_imgRef->decrease() == 0) + delete _imgRef; - { - Lock(&_imgRef->_mutexLock); - if (--_imgRef->_refCount == 0) - doDelete=true; - } - - if (doDelete) - { - // Delete old image reference with associated image and options. - delete _imgRef; - _imgRef=0; - } // Use new image reference _imgRef=image_._imgRef; } - return(*this); } @@ -4254,22 +4225,7 @@ void Magick::Image::sigmoidalContrast(const size_t sharpen_, std::string Magick::Image::signature(const bool force_) const { - const char - *property; - - Lock(&_imgRef->_mutexLock); - - // Re-calculate image signature if necessary - GetPPException; - if (force_ || !GetImageProperty(constImage(),"Signature",exceptionInfo) || - constImage()->taint) - SignatureImage(const_cast(constImage()), - exceptionInfo); - - property=GetImageProperty(constImage(),"Signature",exceptionInfo); - ThrowPPException; - - return(std::string(property)); + return(_imgRef->signature()); } void Magick::Image::sketch(const double radius_,const double sigma_, @@ -4822,11 +4778,8 @@ const MagickCore::QuantizeInfo *Magick::Image::constQuantizeInfo(void) const void Magick::Image::modifyImage(void) { - { - Lock(&_imgRef->_mutexLock); - if (_imgRef->_refCount == 1) - return; - } + if (_imgRef->isOwner()) + return; GetPPException; replaceImage(CloneImage(image(),0,0,MagickTrue,exceptionInfo)); @@ -4839,7 +4792,7 @@ MagickCore::Image *Magick::Image::replaceImage(MagickCore::Image *replacement_) *image; if (replacement_) - image = replacement_; + image=replacement_; else { GetPPException; @@ -4847,23 +4800,17 @@ MagickCore::Image *Magick::Image::replaceImage(MagickCore::Image *replacement_) ThrowPPException; } - { - Lock(&_imgRef->_mutexLock); - - if (_imgRef->_refCount == 1) - { - // We own the image, just replace it - _imgRef->image(image); - } - else - { - // We don't own the image, dereference and replace with copy - --_imgRef->_refCount; - _imgRef=new ImageRef(image,constOptions()); - } - } + // We can replace the image if we own it. + if (!_imgRef->replaceImage(image)) + { + // We don't own the image, dereference and replace with new reference + if (_imgRef->decrease() == 0) + delete _imgRef; + + _imgRef=new ImageRef(image,constOptions()); + } - return(_imgRef->_image); + return(image); } void Magick::Image::read(MagickCore::Image *image, diff --git a/Magick++/lib/ImageRef.cpp b/Magick++/lib/ImageRef.cpp index 5dcc03a9d..440682c92 100644 --- a/Magick++/lib/ImageRef.cpp +++ b/Magick++/lib/ImageRef.cpp @@ -16,9 +16,9 @@ Magick::ImageRef::ImageRef(void) : _image(0), + _mutexLock(), _options(new Options), - _refCount(1), - _mutexLock() + _refCount(1) { GetPPException; _image=AcquireImage(_options->imageInfo(),exceptionInfo); @@ -27,17 +27,17 @@ Magick::ImageRef::ImageRef(void) Magick::ImageRef::ImageRef(MagickCore::Image *image_) : _image(image_), + _mutexLock(), _options(new Options), - _refCount(1), - _mutexLock() + _refCount(1) { } Magick::ImageRef::ImageRef(MagickCore::Image *image_,const Options *options_) : _image(image_), + _mutexLock(), _options(0), - _refCount(1), - _mutexLock() + _refCount(1) { _options=new Options(*options_); } @@ -46,26 +46,50 @@ Magick::ImageRef::~ImageRef(void) { // Deallocate image if (_image != (MagickCore::Image*) NULL) - { - DestroyImageList(_image); - _image=(MagickCore::Image*) NULL; - } + _image=DestroyImageList(_image); // Deallocate image options delete _options; _options=(Options *) NULL; } +size_t Magick::ImageRef::decrease() +{ + size_t + count; + + _mutexLock.lock(); + if (_refCount == 0) + { + _mutexLock.unlock(); + throwExceptionExplicit(OptionError,"Invalid call to decrease"); + } + count=--_refCount; + _mutexLock.unlock(); + return(count); +} + MagickCore::Image *&Magick::ImageRef::image(void) { return(_image); } -void Magick::ImageRef::image(MagickCore::Image * image_) +void Magick::ImageRef::increase() { - if (_image != (MagickCore::Image*) NULL) - DestroyImageList(_image); - _image=image_; + _mutexLock.lock(); + _refCount++; + _mutexLock.unlock(); +} + +bool Magick::ImageRef::isOwner() +{ + bool + isOwner; + + _mutexLock.lock(); + isOwner=(_refCount == 1); + _mutexLock.unlock(); + return(isOwner); } void Magick::ImageRef::options(Magick::Options *options_) @@ -78,3 +102,43 @@ Magick::Options *Magick::ImageRef::options(void) { return(_options); } + +bool Magick::ImageRef::replaceImage(MagickCore::Image * replacement_) +{ + bool + replaced; + + replaced=false; + _mutexLock.lock(); + if (_refCount == 1) + { + if (_image != (MagickCore::Image*) NULL) + (void) DestroyImageList(_image); + _image=replacement_; + replaced=true; + } + _mutexLock.unlock(); + return(replaced); +} + +std::string Magick::ImageRef::signature(const bool force_) +{ + const char + *property; + + // Re-calculate image signature if necessary + GetPPException; + _mutexLock.lock(); + property=(const char *) NULL; + if (!force_ && (_image->taint == MagickFalse)) + property=GetImageProperty(_image,"Signature",exceptionInfo); + if (property == (const char *) NULL) + { + (void) SignatureImage(_image,exceptionInfo); + property=GetImageProperty(_image,"Signature",exceptionInfo); + } + _mutexLock.unlock(); + ThrowPPException; + + return(std::string(property)); +} diff --git a/Magick++/lib/Magick++/BlobRef.h b/Magick++/lib/Magick++/BlobRef.h index effed402c..17c4c749e 100644 --- a/Magick++/lib/Magick++/BlobRef.h +++ b/Magick++/lib/Magick++/BlobRef.h @@ -20,22 +20,30 @@ namespace Magick class BlobRef { public: + // Construct with data, making private copy of data BlobRef(const void* data_,size_t length_); // Destructor (actually destroys data) ~BlobRef(void); - void* data; // Blob data - size_t length; // Blob length + // Decreases reference count and return the new count + size_t decrease(); + + // Increases reference count + void increase(); + Blob::Allocator allocator; // Memory allocation system in use - ::ssize_t refCount; // Reference count - MutexLock mutexLock; // Mutex lock + size_t length; // Blob length + void* data; // Blob data private: // Copy constructor and assignment are not supported BlobRef(const BlobRef&); BlobRef& operator=(const BlobRef&); + + MutexLock _mutexLock; // Mutex lock + size_t _refCount; // Reference count }; } // namespace Magick diff --git a/Magick++/lib/Magick++/ImageRef.h b/Magick++/lib/Magick++/ImageRef.h index 47d82eea3..887760c22 100644 --- a/Magick++/lib/Magick++/ImageRef.h +++ b/Magick++/lib/Magick++/ImageRef.h @@ -11,6 +11,7 @@ #if !defined(Magick_ImageRef_header) #define Magick_ImageRef_header +#include #include "Magick++/Include.h" #include "Magick++/Thread.h" @@ -23,9 +24,7 @@ namespace Magick // class MagickPPExport ImageRef { - friend class Image; - - private: + public: // Construct with null image and default options ImageRef(void); @@ -39,22 +38,41 @@ namespace Magick // Destroy image and options ~ImageRef(void); - // Copy constructor and assignment are not supported - ImageRef(const ImageRef&); - ImageRef& operator=(const ImageRef&); + // Decreases reference count and return the new count + size_t decrease(); // Retrieve image from reference - void image(MagickCore::Image *image_); MagickCore::Image *&image(void); + // Increases reference count + void increase(); + + // Returns true if the reference count is one + bool isOwner(); + // Retrieve Options from reference void options(Options *options_); Options *options(void); + // Tries to replaces the images with the specified image, returns fails + // when this fails + bool replaceImage(MagickCore::Image *replacement_); + + // Image signature. Set force_ to true in order to re-calculate + // the signature regardless of whether the image data has been + // modified. + std::string signature(const bool force_=false); + + private: + + // Copy constructor and assignment are not supported + ImageRef(const ImageRef&); + ImageRef& operator=(const ImageRef&); + MagickCore::Image *_image; // ImageMagick Image + MutexLock _mutexLock; // Mutex lock Options *_options; // User-specified options ::ssize_t _refCount; // Reference count - MutexLock _mutexLock; // Mutex lock }; } // end of namespace Magick diff --git a/Magick++/lib/Magick++/Thread.h b/Magick++/lib/Magick++/Thread.h index 23e6fb10e..678548afe 100644 --- a/Magick++/lib/Magick++/Thread.h +++ b/Magick++/lib/Magick++/Thread.h @@ -14,15 +14,6 @@ #if defined(_VISUALC_) #include -#if defined(_MT) -struct win32_mutex -{ - HANDLE id; -}; - -// This is a binary semphore -- increase for a counting semaphore -#define MAXSEMLEN 1 -#endif // defined(_MT) #endif // defined(_VISUALC_) #if defined(MAGICKCORE_HAVE_PTHREAD) @@ -60,45 +51,9 @@ namespace Magick pthread_mutex_t _mutex; #endif #if defined(_MT) && defined(_VISUALC_) - win32_mutex _mutex; + HANDLE _mutex; #endif }; - - // Lock mutex while object is in scope - class MagickPPExport Lock - { - public: - - // Construct with mutex lock (locks mutex) - Lock(MutexLock *mutexLock_); - - // Destrutor (unlocks mutex) - ~Lock(void); - - private: - - // Don't support copy constructor - Lock(const Lock& original_); - - // Don't support assignment - Lock& operator=(const Lock& original_); - - MutexLock* _mutexLock; - }; -} - -// Construct with mutex lock (locks mutex) -inline Magick::Lock::Lock(MutexLock *mutexLock_) - : _mutexLock(mutexLock_) -{ - _mutexLock->lock(); -} - -// Destrutor (unlocks mutex) -inline Magick::Lock::~Lock(void) -{ - _mutexLock->unlock(); - _mutexLock=(MutexLock*) NULL; } #endif // Magick_Thread_header diff --git a/Magick++/lib/Thread.cpp b/Magick++/lib/Thread.cpp index 695de5c14..19c923446 100644 --- a/Magick++/lib/Thread.cpp +++ b/Magick++/lib/Thread.cpp @@ -48,8 +48,8 @@ Magick::MutexLock::MutexLock(void) security.bInheritHandle=TRUE; /* Create the semaphore, with initial value signaled */ - _mutex.id=::CreateSemaphore(&security,1,MAXSEMLEN,(LPCSTR) NULL); - if (_mutex.id != (HANDLE) NULL) + _mutex=::CreateSemaphore(&security,1,1,(LPCSTR) NULL); + if (_mutex != (HANDLE) NULL) return; throwExceptionExplicit(OptionError,"mutex initialization failed"); } @@ -73,7 +73,7 @@ Magick::MutexLock::~MutexLock(void) strerror(sysError)); #endif #if defined(_MT) && defined(_VISUALC_) - if (::CloseHandle(_mutex.id) != 0) + if (::CloseHandle(_mutex) != 0) return; throwExceptionExplicit(OptionError,"mutex destruction failed"); #endif @@ -92,7 +92,7 @@ void Magick::MutexLock::lock(void) strerror(sysError)); #endif #if defined(_MT) && defined(_VISUALC_) - if (WaitForSingleObject(_mutex.id,INFINITE) != WAIT_FAILED) + if (WaitForSingleObject(_mutex,INFINITE) != WAIT_FAILED) return; throwExceptionExplicit(OptionError,"mutex lock failed"); #endif @@ -111,7 +111,7 @@ void Magick::MutexLock::unlock(void) strerror(sysError)); #endif #if defined(_MT) && defined(_VISUALC_) - if (ReleaseSemaphore(_mutex.id,1,(LPLONG) NULL) == TRUE) + if (ReleaseSemaphore(_mutex,1,(LPLONG) NULL) == TRUE) return; throwExceptionExplicit(OptionError,"mutex unlock failed"); #endif -- 2.40.0