[core] Fix image type of Map::renderStill

It's a premultiplied image. This implies that we were misusing encodePNG
in most cases, as we were passing premultiplied pixels which were then
interpreted as unmultiplied. I changed encodePNG to accept premultipled
pixels, and unpremultiply in the implementations.

21 files changed, 70 insertions, 67 deletions

diff --git a/bin/render.cpp b/bin/render.cpp
index 8a8bcc1a4b..ceb95c165c 100644
--- a/bin/render.cpp
+++ b/bin/render.cpp
@@ -104,14 +104,14 @@ int main(int argc, char *argv[]) {
 
     uv_async_t *async = new uv_async_t;
     uv_async_init(uv_default_loop(), async, [](UV_ASYNC_PARAMS(as)) {
-        std::unique_ptr<UnassociatedImage> image(reinterpret_cast<UnassociatedImage*>(as->data));
+        std::unique_ptr<PremultipliedImage> image(reinterpret_cast<PremultipliedImage*>(as->data));
         uv_close(reinterpret_cast<uv_handle_t *>(as), [](uv_handle_t *handle) {
             delete reinterpret_cast<uv_async_t *>(handle);
         });
         util::write_file(output, encodePNG(*image));
     });
 
-    map.renderStill([async](std::exception_ptr error, UnassociatedImage&& image) {
+    map.renderStill([async](std::exception_ptr error, PremultipliedImage&& image) {
         try {
             if (error) {
                 std::rethrow_exception(error);
@@ -121,7 +121,7 @@ int main(int argc, char *argv[]) {
             exit(1);
         }
 
-        async->data = new UnassociatedImage(std::move(image));
+        async->data = new PremultipliedImage(std::move(image));
         uv_async_send(async);
     });
 
diff --git a/include/mbgl/map/map.hpp b/include/mbgl/map/map.hpp
index 80b50c2fa4..35895660f2 100644
--- a/include/mbgl/map/map.hpp
+++ b/include/mbgl/map/map.hpp
@@ -58,7 +58,7 @@ public:
 
     // Register a callback that will get called (on the render thread) when all resources have
     // been loaded and a complete render occurs.
-    using StillImageCallback = std::function<void (std::exception_ptr, UnassociatedImage&&)>;
+    using StillImageCallback = std::function<void (std::exception_ptr, PremultipliedImage&&)>;
     void renderStill(StillImageCallback callback);
 
     // Triggers a synchronous render.
diff --git a/include/mbgl/map/view.hpp b/include/mbgl/map/view.hpp
index 68bde5fe30..ecdd93377f 100644
--- a/include/mbgl/map/view.hpp
+++ b/include/mbgl/map/view.hpp
@@ -70,7 +70,7 @@ public:
 
     // Reads the pixel data from the current framebuffer. If your View implementation
     // doesn't support reading from the framebuffer, return a null pointer.
-    virtual UnassociatedImage readStillImage();
+    virtual PremultipliedImage readStillImage();
 
     // Notifies a watcher of map x/y/scale/rotation changes.
     // Must only be called from the same thread that caused the change.
diff --git a/include/mbgl/platform/default/headless_view.hpp b/include/mbgl/platform/default/headless_view.hpp
index 8b25620524..4b25b81bb9 100644
--- a/include/mbgl/platform/default/headless_view.hpp
+++ b/include/mbgl/platform/default/headless_view.hpp
@@ -39,7 +39,7 @@ public:
     void invalidate() override;
     void beforeRender() override;
     void afterRender() override;
-    UnassociatedImage readStillImage() override;
+    PremultipliedImage readStillImage() override;
 
     void resizeFramebuffer();
     void resize(uint16_t width, uint16_t height);
diff --git a/include/mbgl/util/image.hpp b/include/mbgl/util/image.hpp
index 0604691dd1..b8e018f696 100644
--- a/include/mbgl/util/image.hpp
+++ b/include/mbgl/util/image.hpp
@@ -34,7 +34,7 @@ using PremultipliedImage = Image<ImageAlphaMode::Premultiplied>;
 
 // TODO: don't use std::string for binary data.
 PremultipliedImage decodeImage(const std::string&);
-std::string encodePNG(const UnassociatedImage&);
+std::string encodePNG(const PremultipliedImage&);
 
 }
 
diff --git a/platform/darwin/image.mm b/platform/darwin/image.mm
index e7187366b9..854b9157f8 100644
--- a/platform/darwin/image.mm
+++ b/platform/darwin/image.mm
@@ -10,7 +10,7 @@
 
 namespace mbgl {
 
-std::string encodePNG(const UnassociatedImage& src) {
+std::string encodePNG(const PremultipliedImage& src) {
     CGDataProviderRef provider = CGDataProviderCreateWithData(NULL, src.data.get(), src.size(), NULL);
     if (!provider) {
         return "";
@@ -23,7 +23,7 @@ std::string encodePNG(const UnassociatedImage& src) {
     }
 
     CGImageRef image = CGImageCreate(src.width, src.height, 8, 32, 4 * src.width, color_space,
-        kCGBitmapByteOrderDefault | kCGImageAlphaLast, provider, NULL, false,
+        kCGBitmapByteOrderDefault | kCGImageAlphaPremultipliedLast, provider, NULL, false,
         kCGRenderingIntentDefault);
     if (!image) {
         CGColorSpaceRelease(color_space);
diff --git a/platform/default/headless_view.cpp b/platform/default/headless_view.cpp
index d96b3ffcd1..d6fd892700 100644
--- a/platform/default/headless_view.cpp
+++ b/platform/default/headless_view.cpp
@@ -167,13 +167,13 @@ void HeadlessView::resize(const uint16_t width, const uint16_t height) {
     needsResize = true;
 }
 
-UnassociatedImage HeadlessView::readStillImage() {
+PremultipliedImage HeadlessView::readStillImage() {
     assert(isActive());
 
     const unsigned int w = dimensions[0] * pixelRatio;
     const unsigned int h = dimensions[1] * pixelRatio;
 
-    UnassociatedImage image { w, h };
+    PremultipliedImage image { w, h };
     MBGL_CHECK_ERROR(glReadPixels(0, 0, w, h, GL_RGBA, GL_UNSIGNED_BYTE, image.data.get()));
 
     const int stride = image.stride();
diff --git a/platform/default/image.cpp b/platform/default/image.cpp
index dc28c68ef9..bf8071af5c 100644
--- a/platform/default/image.cpp
+++ b/platform/default/image.cpp
@@ -1,5 +1,6 @@
 #include <mbgl/util/image.hpp>
 #include <mbgl/util/string.hpp>
+#include <mbgl/util/premultiply.hpp>
 
 #include <png.h>
 
@@ -19,7 +20,12 @@ const static bool png_version_check = []() {
 #pragma GCC diagnostic pop
 namespace mbgl {
 
-std::string encodePNG(const UnassociatedImage& src) {
+std::string encodePNG(const PremultipliedImage& pre) {
+    PremultipliedImage copy { pre.width, pre.height };
+    std::copy(pre.data.get(), pre.data.get() + pre.size(), copy.data.get());
+
+    UnassociatedImage src = util::unpremultiply(std::move(copy));
+
     png_voidp error_ptr = 0;
     png_structp png_ptr = png_create_write_struct(PNG_LIBPNG_VER_STRING, error_ptr, NULL, NULL);
     if (!png_ptr) {
diff --git a/platform/node/src/node_map.cpp b/platform/node/src/node_map.cpp
index 0ce7d9398b..d4c551d666 100644
--- a/platform/node/src/node_map.cpp
+++ b/platform/node/src/node_map.cpp
@@ -300,7 +300,7 @@ void NodeMap::startRender(std::unique_ptr<NodeMap::RenderOptions> options) {
     map->setBearing(options->bearing);
     map->setPitch(options->pitch);
 
-    map->renderStill([this](const std::exception_ptr eptr, mbgl::UnassociatedImage&& result) {
+    map->renderStill([this](const std::exception_ptr eptr, mbgl::PremultipliedImage&& result) {
         if (eptr) {
             error = std::move(eptr);
             uv_async_send(async);
diff --git a/platform/node/src/node_map.hpp b/platform/node/src/node_map.hpp
index 455d38e957..23cd3c9fab 100644
--- a/platform/node/src/node_map.hpp
+++ b/platform/node/src/node_map.hpp
@@ -49,7 +49,7 @@ public:
     std::unique_ptr<mbgl::Map> map;
 
     std::exception_ptr error;
-    mbgl::UnassociatedImage image;
+    mbgl::PremultipliedImage image;
     std::unique_ptr<Nan::Callback> callback;
 
     // Async for delivering the notifications of render completion.
diff --git a/src/mbgl/map/view.cpp b/src/mbgl/map/view.cpp
index fa8b7584cc..efbe0f1672 100644
--- a/src/mbgl/map/view.cpp
+++ b/src/mbgl/map/view.cpp
@@ -10,7 +10,7 @@ void View::initialize(Map *map_) {
     map = map_;
 }
 
-UnassociatedImage View::readStillImage() {
+PremultipliedImage View::readStillImage() {
     return {};
 }
 
diff --git a/src/mbgl/util/premultiply.cpp b/src/mbgl/util/premultiply.cpp
index 3ee5a1e129..8a62012251 100644
--- a/src/mbgl/util/premultiply.cpp
+++ b/src/mbgl/util/premultiply.cpp
@@ -6,14 +6,14 @@ namespace mbgl {
 namespace util {
 
 PremultipliedImage premultiply(UnassociatedImage&& src) {
-   PremultipliedImage dst;
+    PremultipliedImage dst;
 
-   dst.width = src.width;
-   dst.height = src.height;
-   dst.data = std::move(src.data);
+    dst.width = src.width;
+    dst.height = src.height;
+    dst.data = std::move(src.data);
 
-   uint8_t* data = dst.data.get();
-   for (size_t i = 0; i < dst.size(); i += 4) {
+    uint8_t* data = dst.data.get();
+    for (size_t i = 0; i < dst.size(); i += 4) {
         uint8_t& r = data[i + 0];
         uint8_t& g = data[i + 1];
         uint8_t& b = data[i + 2];
@@ -26,5 +26,28 @@ PremultipliedImage premultiply(UnassociatedImage&& src) {
     return std::move(dst);
 }
 
+UnassociatedImage unpremultiply(PremultipliedImage&& src) {
+    UnassociatedImage dst;
+
+    dst.width = src.width;
+    dst.height = src.height;
+    dst.data = std::move(src.data);
+
+    uint8_t* data = dst.data.get();
+    for (size_t i = 0; i < dst.size(); i += 4) {
+        uint8_t& r = data[i + 0];
+        uint8_t& g = data[i + 1];
+        uint8_t& b = data[i + 2];
+        uint8_t& a = data[i + 3];
+        if (a) {
+            r = (255 * r + (a / 2)) / a;
+            g = (255 * g + (a / 2)) / a;
+            b = (255 * b + (a / 2)) / a;
+        }
+    }
+
+    return std::move(dst);
+}
+
 }
 }
diff --git a/src/mbgl/util/premultiply.hpp b/src/mbgl/util/premultiply.hpp
index 9d9dcc6468..8d87c157c8 100644
--- a/src/mbgl/util/premultiply.hpp
+++ b/src/mbgl/util/premultiply.hpp
@@ -7,6 +7,7 @@ namespace mbgl {
 namespace util {
 
 PremultipliedImage premultiply(UnassociatedImage&&);
+UnassociatedImage unpremultiply(PremultipliedImage&&);
 
 }
 }
diff --git a/test/api/annotations.cpp b/test/api/annotations.cpp
index c4fea02079..ae9c804730 100644
--- a/test/api/annotations.cpp
+++ b/test/api/annotations.cpp
@@ -14,16 +14,16 @@
 
 using namespace mbgl;
 
-UnassociatedImage render(Map& map) {
-    std::promise<UnassociatedImage> promise;
-    map.renderStill([&](std::exception_ptr, UnassociatedImage&& image) {
+PremultipliedImage render(Map& map) {
+    std::promise<PremultipliedImage> promise;
+    map.renderStill([&](std::exception_ptr, PremultipliedImage&& image) {
         promise.set_value(std::move(image));
     });
     return std::move(promise.get_future().get());
 }
 
 void checkRendering(Map& map, const char * name) {
-    UnassociatedImage actual = render(map);
+    PremultipliedImage actual = render(map);
     test::checkImage(std::string("test/fixtures/annotations/") + name + "/",
                      actual, 0.0002, 0.1);
 }
diff --git a/test/api/api_misuse.cpp b/test/api/api_misuse.cpp
index 4d5fe11042..538f09a040 100644
--- a/test/api/api_misuse.cpp
+++ b/test/api/api_misuse.cpp
@@ -45,7 +45,7 @@ TEST(API, RenderWithoutStyle) {
     Map map(view, fileSource, MapMode::Still);
 
     std::promise<std::exception_ptr> promise;
-    map.renderStill([&promise](std::exception_ptr error, UnassociatedImage&&) {
+    map.renderStill([&promise](std::exception_ptr error, PremultipliedImage&&) {
         promise.set_value(error);
     });
 
diff --git a/test/api/repeated_render.cpp b/test/api/repeated_render.cpp
index 79349c67e7..77e4569dbf 100644
--- a/test/api/repeated_render.cpp
+++ b/test/api/repeated_render.cpp
@@ -25,8 +25,8 @@ TEST(API, RepeatedRender) {
 
     {
         map.setStyleJSON(style, "");
-        std::promise<UnassociatedImage> promise;
-        map.renderStill([&promise](std::exception_ptr, UnassociatedImage&& image) {
+        std::promise<PremultipliedImage> promise;
+        map.renderStill([&promise](std::exception_ptr, PremultipliedImage&& image) {
             promise.set_value(std::move(image));
         });
         auto result = std::move(promise.get_future().get());
@@ -37,8 +37,8 @@ TEST(API, RepeatedRender) {
 
     {
         map.setStyleJSON(style, "TEST_DATA/suite");
-        std::promise<UnassociatedImage> promise;
-        map.renderStill([&promise](std::exception_ptr, UnassociatedImage&& image) {
+        std::promise<PremultipliedImage> promise;
+        map.renderStill([&promise](std::exception_ptr, PremultipliedImage&& image) {
             promise.set_value(std::move(image));
         });
         auto result = std::move(promise.get_future().get());
diff --git a/test/fixtures/util.cpp b/test/fixtures/util.cpp
index aa6371a4c7..ebd530eaac 100644
--- a/test/fixtures/util.cpp
+++ b/test/fixtures/util.cpp
@@ -85,45 +85,17 @@ uint64_t crc64(const std::string& str) {
     return crc64(str.data(), str.size());
 }
 
-UnassociatedImage unpremultiply(const PremultipliedImage& src) {
-    UnassociatedImage dst { src.width, src.height };
-    std::copy(src.data.get(), src.data.get() + src.size(), dst.data.get());
-
-    uint8_t* data = dst.data.get();
-    for (size_t i = 0; i < dst.size(); i += 4) {
-        uint8_t& r = data[i + 0];
-        uint8_t& g = data[i + 1];
-        uint8_t& b = data[i + 2];
-        uint8_t& a = data[i + 3];
-        if (a) {
-            r = (255 * r + (a / 2)) / a;
-            g = (255 * g + (a / 2)) / a;
-            b = (255 * b + (a / 2)) / a;
-        }
-    }
-
-    return std::move(dst);
-}
-
 void checkImage(const std::string& base,
-                const UnassociatedImage& actual,
+                const PremultipliedImage& actual,
                 double imageThreshold,
                 double pixelThreshold) {
-    // TODO: the pixels produced by Map::renderStill are probably actually premultiplied,
-    // but Map::renderStill produces an UnassociatedImage. This probably should be fixed;
-    // here we just hack around it by copying the pixels to a PremultipliedImage (and
-    // un-premultiplying them when updating expected.png, since encodePNG wants
-    // un-premultiplied pixels).
-    PremultipliedImage actualActual { actual.width, actual.height };
-    std::copy(actual.data.get(), actual.data.get() + actual.size(), actualActual.data.get());
-
     if (getenv("UPDATE")) {
-        util::write_file(base + "/expected.png", encodePNG(unpremultiply(actualActual)));
+        util::write_file(base + "/expected.png", encodePNG(actual));
         return;
     }
 
     PremultipliedImage expected = decodeImage(util::read_file(base + "/expected.png"));
-    UnassociatedImage diff { expected.width, expected.height };
+    PremultipliedImage diff { expected.width, expected.height };
 
     ASSERT_EQ(expected.width, actual.width);
     ASSERT_EQ(expected.height, actual.height);
diff --git a/test/fixtures/util.hpp b/test/fixtures/util.hpp
index b369979e9c..16edfbace3 100644
--- a/test/fixtures/util.hpp
+++ b/test/fixtures/util.hpp
@@ -25,7 +25,7 @@ uint64_t crc64(const char*, size_t);
 uint64_t crc64(const std::string&);
 
 void checkImage(const std::string& base,
-                const UnassociatedImage& actual,
+                const PremultipliedImage& actual,
                 double imageThreshold = 0,
                 double pixelThreshold = 0);
 
diff --git a/test/miscellaneous/bilinear.cpp b/test/miscellaneous/bilinear.cpp
index 740025bbbc..3184e5739d 100644
--- a/test/miscellaneous/bilinear.cpp
+++ b/test/miscellaneous/bilinear.cpp
@@ -1,6 +1,7 @@
 #include "../fixtures/util.hpp"
 #include <mbgl/util/compression.hpp>
 #include <mbgl/util/scaling.hpp>
+#include <mbgl/util/premultiply.hpp>
 #include <mbgl/util/image.hpp>
 #include <mbgl/util/io.hpp>
 
@@ -43,7 +44,7 @@ TEST(Bilinear, Scaling) {
     util::bilinearScale(srcData, srcSize, { 252, 380, 12, 12 }, dstData, dstSize, { 18, 90, 24, 24 }, false);
 
     const std::string data { reinterpret_cast<char *>(dstData), dstSize.x * dstSize.y * sizeof(uint32_t) };
-    util::write_file("test/fixtures/sprites/atlas_actual.png", encodePNG(dst));
+    util::write_file("test/fixtures/sprites/atlas_actual.png", encodePNG(util::premultiply(std::move(dst))));
     util::write_file("test/fixtures/sprites/atlas_actual.bin", util::compress(data));
 
     const std::string reference = util::decompress(util::read_file("test/fixtures/sprites/atlas_reference.bin"));
diff --git a/test/miscellaneous/image.cpp b/test/miscellaneous/image.cpp
index 8a321583cb..2db62b96d3 100644
--- a/test/miscellaneous/image.cpp
+++ b/test/miscellaneous/image.cpp
@@ -7,7 +7,7 @@
 using namespace mbgl;
 
 TEST(Image, PNGRoundTrip) {
-    UnassociatedImage rgba { 1, 1 };
+    PremultipliedImage rgba { 1, 1 };
     rgba.data[0] = 128;
     rgba.data[1] = 0;
     rgba.data[2] = 0;
@@ -21,14 +21,14 @@ TEST(Image, PNGRoundTrip) {
 }
 
 TEST(Image, PNGRoundTripAlpha) {
-    UnassociatedImage rgba { 1, 1 };
+    PremultipliedImage rgba { 1, 1 };
     rgba.data[0] = 128;
     rgba.data[1] = 0;
     rgba.data[2] = 0;
     rgba.data[3] = 128;
 
     PremultipliedImage image = decodeImage(encodePNG(rgba));
-    EXPECT_EQ(64, image.data[0]);
+    EXPECT_EQ(128, image.data[0]);
     EXPECT_EQ(0, image.data[1]);
     EXPECT_EQ(0, image.data[2]);
     EXPECT_EQ(128, image.data[3]);
diff --git a/test/style/pending_resources.cpp b/test/style/pending_resources.cpp
index 58dc72937e..81452ffc7e 100644
--- a/test/style/pending_resources.cpp
+++ b/test/style/pending_resources.cpp
@@ -39,7 +39,7 @@ TEST_P(PendingResources, DeleteMapObjectWithPendingRequest) {
     const std::string style = util::read_file("test/fixtures/resources/style.json");
     map->setStyleJSON(style, ".");
 
-    map->renderStill([&endTest](std::exception_ptr, UnassociatedImage&&) {
+    map->renderStill([&endTest](std::exception_ptr, PremultipliedImage&&) {
         EXPECT_TRUE(false) << "Should never happen.";
     });