From 1d33790dc06a1127e55753d123d55e6ec6e89713 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Wed, 25 Nov 2015 13:49:12 -0800 Subject: [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. --- bin/render.cpp | 6 ++--- include/mbgl/map/map.hpp | 2 +- include/mbgl/map/view.hpp | 2 +- include/mbgl/platform/default/headless_view.hpp | 2 +- include/mbgl/util/image.hpp | 2 +- platform/darwin/image.mm | 4 +-- platform/default/headless_view.cpp | 4 +-- platform/default/image.cpp | 8 +++++- platform/node/src/node_map.cpp | 2 +- platform/node/src/node_map.hpp | 2 +- src/mbgl/map/view.cpp | 2 +- src/mbgl/util/premultiply.cpp | 35 ++++++++++++++++++++----- src/mbgl/util/premultiply.hpp | 1 + test/api/annotations.cpp | 8 +++--- test/api/api_misuse.cpp | 2 +- test/api/repeated_render.cpp | 8 +++--- test/fixtures/util.cpp | 34 +++--------------------- test/fixtures/util.hpp | 2 +- test/miscellaneous/bilinear.cpp | 3 ++- test/miscellaneous/image.cpp | 6 ++--- test/style/pending_resources.cpp | 2 +- 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 image(reinterpret_cast(as->data)); + std::unique_ptr image(reinterpret_cast(as->data)); uv_close(reinterpret_cast(as), [](uv_handle_t *handle) { delete reinterpret_cast(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; + using StillImageCallback = std::function; 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; // 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 #include +#include #include @@ -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 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 map; std::exception_ptr error; - mbgl::UnassociatedImage image; + mbgl::PremultipliedImage image; std::unique_ptr 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 promise; - map.renderStill([&](std::exception_ptr, UnassociatedImage&& image) { +PremultipliedImage render(Map& map) { + std::promise 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 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 promise; - map.renderStill([&promise](std::exception_ptr, UnassociatedImage&& image) { + std::promise 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 promise; - map.renderStill([&promise](std::exception_ptr, UnassociatedImage&& image) { + std::promise 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 #include +#include #include #include @@ -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(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."; }); -- cgit v1.2.1