Re-factored locking and BlobRef/ImageRef classes.

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_;

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 <string.h>
 
 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<unsigned char*>(data);
-      data=0;
+      data=(void *) NULL;
     }
   else if (allocator == Magick::Blob::MallocAllocator)
-    {
-      data=(void *) RelinquishMagickMemory(data);
-    }
+    data=(void *) RelinquishMagickMemory(data);
+}
+
+size_t Magick::BlobRef::decrease()
+{
+  size_t
+    count;
+
+  _mutexLock.lock();
+  if (_refCount == 0)
+    {
+      _mutexLock.unlock();
+      throwExceptionExplicit(OptionError,"Invalid call to decrease");
+    }
+  count=--_refCount;
+  _mutexLock.unlock();
+  return(count);
+}
+
+void Magick::BlobRef::increase()
+{
+  _mutexLock.lock();
+  _refCount++;
+  _mutexLock.unlock();
 }

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;
+      image_._imgRef->increase();
+      if (_imgRef->decrease() == 0)
+        delete _imgRef;
 
-      {
-        Lock(&image_._imgRef->_mutexLock);
-        ++image_._imgRef->_refCount;
-      }
-
-      {
-        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<MagickCore::Image *>(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);
+  // 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;
 
-    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());
-      }
-  }
+      _imgRef=new ImageRef(image,constOptions());
+    }
 
-  return(_imgRef->_image);
+  return(image);
 }
 
 void Magick::Image::read(MagickCore::Image *image,

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));
+}

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

Magick++/lib/Magick++/ImageRef.h

@@ -11,6 +11,7 @@
 #if !defined(Magick_ImageRef_header)
 #define Magick_ImageRef_header
 
+#include <string>
 #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

Magick++/lib/Magick++/Thread.h

@@ -14,15 +14,6 @@
 
 #if defined(_VISUALC_)
 #include <windows.h>
-#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

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