Improve thread safety with copy-on-write for metadata #1986

webgurucan · Nov 27, 2019 · 29537c1 · 29537c1
1 parent 8f8b2a1
commit 29537c1
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 18 deletions.
diff --git a/docs/changelog.md b/docs/changelog.md
@@ -4,6 +4,11 @@
 
 Requires libvips v8.8.1.
 
+#### v0.23.4 - TBD
+
+* Improve thread safety by using copy-on-write when updating metadata.
+  [#1986](https://github.com/lovell/sharp/issues/1986)
+
 #### v0.23.3 - 17<sup>th</sup> November 2019
 
 * Ensure `trim` operation supports images contained in the alpha channel.

diff --git a/src/common.cc b/src/common.cc
@@ -277,7 +277,7 @@ namespace sharp {
             }
             image = VImage::new_from_buffer(descriptor->buffer, descriptor->bufferLength, nullptr, option);
             if (imageType == ImageType::SVG || imageType == ImageType::PDF || imageType == ImageType::MAGICK) {
-              SetDensity(image, descriptor->density);
+              image = SetDensity(image, descriptor->density);
             }
           } catch (vips::VError const &err) {
             throw vips::VError(std::string("Input buffer has corrupt header: ") + err.what());
@@ -323,7 +323,7 @@ namespace sharp {
             }
             image = VImage::new_from_file(descriptor->file.data(), option);
             if (imageType == ImageType::SVG || imageType == ImageType::PDF || imageType == ImageType::MAGICK) {
-              SetDensity(image, descriptor->density);
+              image = SetDensity(image, descriptor->density);
             }
           } catch (vips::VError const &err) {
             throw vips::VError(std::string("Input file has corrupt header: ") + err.what());
@@ -370,15 +370,19 @@ namespace sharp {
   /*
     Set EXIF Orientation of image.
   */
-  void SetExifOrientation(VImage image, int const orientation) {
-    image.set(VIPS_META_ORIENTATION, orientation);
+  VImage SetExifOrientation(VImage image, int const orientation) {
+    VImage copy = image.copy();
+    copy.set(VIPS_META_ORIENTATION, orientation);
+    return copy;
   }
 
   /*
     Remove EXIF Orientation from image.
   */
-  void RemoveExifOrientation(VImage image) {
-    vips_image_remove(image.get_image(), VIPS_META_ORIENTATION);
+  VImage RemoveExifOrientation(VImage image) {
+    VImage copy = image.copy();
+    copy.remove(VIPS_META_ORIENTATION);
+    return copy;
   }
 
   /*
@@ -398,11 +402,13 @@ namespace sharp {
   /*
     Set pixels/mm resolution based on a pixels/inch density.
   */
-  void SetDensity(VImage image, const double density) {
+  VImage SetDensity(VImage image, const double density) {
     const double pixelsPerMm = density / 25.4;
-    image.set("Xres", pixelsPerMm);
-    image.set("Yres", pixelsPerMm);
-    image.set(VIPS_META_RESOLUTION_UNIT, "in");
+    VImage copy = image.copy();
+    copy.set("Xres", pixelsPerMm);
+    copy.set("Yres", pixelsPerMm);
+    copy.set(VIPS_META_RESOLUTION_UNIT, "in");
+    return copy;
   }
 
   /*

diff --git a/src/common.h b/src/common.h
@@ -175,12 +175,12 @@ namespace sharp {
   /*
     Set EXIF Orientation of image.
   */
-  void SetExifOrientation(VImage image, int const orientation);
+  VImage SetExifOrientation(VImage image, int const orientation);
 
   /*
     Remove EXIF Orientation from image.
   */
-  void RemoveExifOrientation(VImage image);
+  VImage RemoveExifOrientation(VImage image);
 
   /*
     Does this image have a non-default density?
@@ -195,7 +195,7 @@ namespace sharp {
   /*
     Set pixels/mm resolution based on a pixels/inch density.
   */
-  void SetDensity(VImage image, const double density);
+  VImage SetDensity(VImage image, const double density);
 
   /*
     Check the proposed format supports the current dimensions.

diff --git a/src/pipeline.cc b/src/pipeline.cc
@@ -104,7 +104,7 @@ class PipelineWorker : public Nan::AsyncWorker {
       if (baton->rotateBeforePreExtract) {
         if (rotation != VIPS_ANGLE_D0) {
           image = image.rot(rotation);
-          sharp::RemoveExifOrientation(image);
+          image = sharp::RemoveExifOrientation(image);
         }
         if (baton->rotationAngle != 0.0) {
           std::vector<double> background;
@@ -404,20 +404,20 @@ class PipelineWorker : public Nan::AsyncWorker {
       // Rotate post-extract 90-angle
       if (!baton->rotateBeforePreExtract &&  rotation != VIPS_ANGLE_D0) {
           image = image.rot(rotation);
-          sharp::RemoveExifOrientation(image);
+          image = sharp::RemoveExifOrientation(image);
       }
 
 
       // Flip (mirror about Y axis)
       if (baton->flip) {
         image = image.flip(VIPS_DIRECTION_VERTICAL);
-        sharp::RemoveExifOrientation(image);
+        image = sharp::RemoveExifOrientation(image);
       }
 
       // Flop (mirror about X axis)
       if (baton->flop) {
         image = image.flip(VIPS_DIRECTION_HORIZONTAL);
-        sharp::RemoveExifOrientation(image);
+        image = sharp::RemoveExifOrientation(image);
       }
 
       // Join additional color channels to the image
@@ -700,7 +700,7 @@ class PipelineWorker : public Nan::AsyncWorker {
 
       // Override EXIF Orientation tag
       if (baton->withMetadata && baton->withMetadataOrientation != -1) {
-        sharp::SetExifOrientation(image, baton->withMetadataOrientation);
+        image = sharp::SetExifOrientation(image, baton->withMetadataOrientation);
       }
 
       // Number of channels used in output image