artemp/issue 978

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Changelog
 
+## 4.5.9
+- Image - fix object lifetimes/scopes in relevant async methods (#978)
+
 ## 4.5.8
 - Upgrade to mapnik@e553f55dc
   (https://github.com/mapnik/mapnik/compare/c3eda40e0...e553f55dc)

package.json

@@ -4,7 +4,7 @@
   "url": "http://github.com/mapnik/node-mapnik",
   "homepage": "http://mapnik.org",
   "author": "Dane Springmeyer <dane@mapbox.com> (mapnik.org)",
-  "version": "4.5.8",
+  "version": "4.5.9-dev2",
   "mapnik_version": "e553f55dc",
   "main": "./lib/mapnik.js",
   "binary": {

src/mapnik_image.cpp

@@ -105,6 +105,20 @@ Image::Image(Napi::CallbackInfo const& info)
         if (ext) image_ = *ext.Data();
         return;
     }
+
+    if (info.Length() == 1 && info[0].IsBuffer())
+    {
+        auto buf = info[0].As<Napi::Buffer<unsigned char>>();
+        auto obj = info[0].As<Napi::Object>();
+        bool premultiplied = obj.Get("premultiplied").As<Napi::Boolean>();
+        bool painted = obj.Get("painted").As<Napi::Boolean>();
+        int width = obj.Get("width").As<Napi::Number>().Int32Value();
+        int height = obj.Get("height").As<Napi::Number>().Int32Value();
+        mapnik::image_rgba8 im_wrapper(width, height, buf.Data(), premultiplied, painted);
+        image_ = std::make_shared<mapnik::image_any>(im_wrapper);
+        buf_ref_ = Napi::Persistent(buf);
+        return;
+    }
     if (info.Length() >= 2)
     {
         mapnik::image_dtype type = mapnik::image_dtype_rgba8;
@@ -643,7 +657,11 @@ Napi::Value Image::view(Napi::CallbackInfo const& info)
     Napi::Number w = info[2].As<Napi::Number>();
     Napi::Number h = info[3].As<Napi::Number>();
     Napi::Value image_obj = Napi::External<image_ptr>::New(env, &image_);
-    Napi::Object obj = ImageView::constructor.New({image_obj, x, y, w, h });
+    if (buf_ref_.IsEmpty())
+    {
+        return scope.Escape(ImageView::constructor.New({image_obj, x, y, w, h}));
+    }
+    Napi::Object obj = ImageView::constructor.New({image_obj, x, y, w, h, buf_ref_.Value()});
     return scope.Escape(obj);
 }
 
@@ -707,7 +725,8 @@ void Image::scaling(Napi::CallbackInfo const& info, Napi::Value const& value)
 Napi::Value Image::data(Napi::CallbackInfo const& info)
 {
     Napi::Env env = info.Env();
-    if (image_) return Napi::Buffer<unsigned char>::Copy(env, image_->bytes(), image_->size());
+    Napi::EscapableHandleScope scope(env);
+    if (image_) return scope.Escape(Napi::Buffer<unsigned char>::Copy(env, image_->bytes(), image_->size()));
     return info.Env().Null();
 }
 
@@ -730,6 +749,7 @@ Napi::Value Image::data(Napi::CallbackInfo const& info)
 Napi::Value Image::buffer(Napi::CallbackInfo const& info)
 {
     Napi::Env env = info.Env();
-    if (image_) return Napi::Buffer<unsigned char>::New(env, image_->bytes(), image_->size());
+    Napi::EscapableHandleScope scope(env);
+    if (image_) return scope.Escape(Napi::Buffer<unsigned char>::New(env, image_->bytes(), image_->size()));
     return info.Env().Null();
 }

src/mapnik_image.hpp

@@ -76,4 +76,5 @@ class Image : public Napi::ObjectWrap<Image>
     static void encode_common_args_(Napi::CallbackInfo const& info, std::string& format, palette_ptr& palette);
     static Napi::Value from_svg_sync_impl(Napi::CallbackInfo const& info, bool from_file);
     image_ptr image_;
+    Napi::Reference<Napi::Buffer<unsigned char>> buf_ref_;
 };

src/mapnik_image_encode.cpp

@@ -56,13 +56,23 @@ struct AsyncEncode : Napi::AsyncWorker
 {
     using Base = Napi::AsyncWorker;
     // ctor
-    AsyncEncode(image_ptr image, palette_ptr palette, std::string const& format, Napi::Function const& callback)
+    AsyncEncode(Image* obj, image_ptr image, palette_ptr palette, std::string const& format, Napi::Function const& callback)
         : Base(callback),
+          obj_(obj),
           image_(image),
           palette_(palette),
           format_(format)
     {
     }
+    ~AsyncEncode() {}
+    void OnWorkComplete(Napi::Env env, napi_status status) override
+    {
+        if (obj_ && !obj_->IsEmpty())
+        {
+            obj_->Unref();
+        }
+        Base::OnWorkComplete(env, status);
+    }
     void Execute() override
     {
         try
@@ -101,6 +111,7 @@ struct AsyncEncode : Napi::AsyncWorker
     }
 
   private:
+    Image* obj_;
     image_ptr image_;
     palette_ptr palette_;
     std::string format_;
@@ -205,7 +216,8 @@ Napi::Value Image::encode(Napi::CallbackInfo const& info)
         return env.Undefined();
     }
     Napi::Function callback = callback_val.As<Napi::Function>();
-    auto* worker = new AsyncEncode{image_, palette, format, callback};
+    this->Ref();
+    auto* worker = new AsyncEncode{this, image_, palette, format, callback};
     worker->Queue();
     return env.Undefined();
 }

src/mapnik_image_from_bytes.cpp

@@ -335,11 +335,11 @@ Napi::Value Image::fromBufferSync(Napi::CallbackInfo const& info)
 
     try
     {
-        mapnik::image_rgba8 im_wrapper(width, height, obj.As<Napi::Buffer<unsigned char>>().Data(), premultiplied, painted);
-        image_ptr imagep = std::make_shared<mapnik::image_any>(im_wrapper);
-        Napi::Value arg = Napi::External<image_ptr>::New(env, &imagep);
-        Napi::Object image_obj = constructor.New({arg});
-        image_obj.Set("_buffer", obj);
+        obj.Set("width", width);
+        obj.Set("height", height);
+        obj.Set("premultiplied", premultiplied);
+        obj.Set("painted", painted);
+        Napi::Object image_obj = constructor.New({obj});
         return scope.Escape(image_obj);
     }
     catch (std::exception const& ex)

src/mapnik_image_view.cpp

@@ -87,7 +87,7 @@ ImageView::ImageView(Napi::CallbackInfo const& info)
     : Napi::ObjectWrap<ImageView>(info)
 {
     Napi::Env env = info.Env();
-    if (info.Length() == 5 && info[0].IsExternal() && info[1].IsNumber() && info[2].IsNumber() && info[3].IsNumber() && info[4].IsNumber())
+    if (info.Length() >= 5 && info[0].IsExternal() && info[1].IsNumber() && info[2].IsNumber() && info[3].IsNumber() && info[4].IsNumber())
     {
         std::size_t x = info[1].As<Napi::Number>().Int64Value();
         std::size_t y = info[2].As<Napi::Number>().Int64Value();
@@ -98,6 +98,10 @@ ImageView::ImageView(Napi::CallbackInfo const& info)
         {
             image_ = *ext.Data();
             image_view_ = std::make_shared<mapnik::image_view_any>(mapnik::create_view(*image_, x, y, w, h));
+            if (info.Length() == 6 && info[5].IsBuffer())
+            {
+                buf_ref_ = Napi::Persistent(info[5].As<Napi::Buffer<unsigned char>>());
+            }
             return;
         }
     }
@@ -264,13 +268,23 @@ struct AsyncEncode : Napi::AsyncWorker
 {
     using Base = Napi::AsyncWorker;
     // ctor
-    AsyncEncode(image_view_ptr image_view, palette_ptr palette, std::string const& format, Napi::Function const& callback)
+    AsyncEncode(ImageView* obj, image_view_ptr image_view, palette_ptr palette, std::string const& format, Napi::Function const& callback)
         : Base(callback),
+          obj_(obj),
           image_view_(image_view),
           palette_(palette),
           format_(format)
     {
     }
+    ~AsyncEncode() {}
+    void OnWorkComplete(Napi::Env env, napi_status status) override
+    {
+        if (obj_ && !obj_->IsEmpty())
+        {
+            obj_->Unref();
+        }
+        Base::OnWorkComplete(env, status);
+    }
     void Execute() override
     {
         try
@@ -309,6 +323,7 @@ struct AsyncEncode : Napi::AsyncWorker
     }
 
   private:
+    ImageView* obj_;
     image_view_ptr image_view_;
     palette_ptr palette_;
     std::string format_;
@@ -369,7 +384,8 @@ Napi::Value ImageView::encode(Napi::CallbackInfo const& info)
         return env.Undefined();
     }
     Napi::Function callback = callback_val.As<Napi::Function>();
-    auto* worker = new AsyncEncode{image_view_, palette, format, callback};
+    this->Ref();
+    auto* worker = new AsyncEncode{this, image_view_, palette, format, callback};
     worker->Queue();
     return env.Undefined();
 }

src/mapnik_image_view.hpp

@@ -37,4 +37,5 @@ class ImageView : public Napi::ObjectWrap<ImageView>
     static Napi::FunctionReference constructor;
     image_view_ptr image_view_;
     image_ptr image_;
+    Napi::Reference<Napi::Buffer<unsigned char>> buf_ref_;
 };

src/mapnik_map_render.cpp

@@ -24,8 +24,6 @@
 #include <mapnik/grid/grid.hpp>          // for hit_grid, grid
 #include <mapnik/grid/grid_renderer.hpp> // for grid_renderer
 #endif
-// stl
-#include <future>
 
 namespace detail {
 struct agg_renderer_visitor

test/compositing.test.js

@@ -38,25 +38,24 @@ test('should stitch input images correctly', (assert) => {
     output_image.composite(item[0], {dx: item[1], dy: item[2]}, function(err) {
       if (err) throw err;
       assert.ok(output_image);
-    });
-  });
-  var views = [output_image.view(0, 0, 256, 256),
-               output_image.view(256, 0, 256, 256),
-               output_image.view(256, 256, 256, 256),
-               output_image.view(0, 256, 256, 256)];
+      var views = [output_image.view(0, 0, 256, 256),
+                   output_image.view(256, 0, 256, 256),
+                   output_image.view(256, 256, 256, 256),
+                   output_image.view(0, 256, 256, 256)];
 
-  views.forEach(function(view, index) {
-    var equals = true;
-    for (var x = 0; x < view.width(); ++x) {
-      for (var y = 0; y < view.height(); ++y) {
-        if (view.getPixel(x, y) != input[index][0].getPixel(x, y)) {
-          equals = false;
-          break;
+      views.forEach(function(view, index) {
+        var equals = true;
+        for (var x = 0; x < view.width(); ++x) {
+          for (var y = 0; y < view.height(); ++y) {
+            if (view.getPixel(x, y) != input[index][0].getPixel(x, y)) {
+              equals = false;
+              break;
+            }
+          }
         }
-      }
-    }
-    output_image.save("output.png");
-    assert.equal(equals, true);
+        assert.equal(equals, true);
+      });
+    });
   });
   assert.end();
 });

test/image.test.js

@@ -1653,7 +1653,6 @@ test('be able to create image with zero allocation / from raw buffer', (assert)
   var im2 = new mapnik.Image.fromBufferSync(im.width(), im.height(), data);
   // We attach `data` onto the image so that v8 will not
   // clean it up before im2 is destroyed
-  assert.equal(im2._buffer,data);
   assert.equal(im2.premultiplied(), false);
   assert.equal(0, im.compare(im2, {threshold:0}));
   im.premultiplySync();
@@ -1669,13 +1668,6 @@ test('be able to create image with zero allocation / from raw buffer', (assert)
   });
   assert.equal(im3.premultiplied(), true);
   assert.equal(im3.painted(), true);
-
-  // Just for testing (not good practice) we modify data
-  // and ensure that im2._buffer also reflects the modification
-  // as another way to prove they are the same instance
-  assert.equal(im2._buffer[0],23);
-  data[0] = 99;
-  assert.equal(im2._buffer[0],99);
   assert.end();
 });