From 26faa6a5ade54c0a423aab84106876dc59be868f Mon Sep 17 00:00:00 2001 From: Ansis Brammanis Date: Thu, 14 Jan 2016 17:11:25 -0800 Subject: [PATCH] [core][ios][osx][android] fix icons with non-integer width/height ref #3031 ref #2198 For example, an icon that has: - a pixel width of 10 - a pixel ratio of 3 - a scaled with of 3.333 is now supported. --- CHANGELOG.md | 3 +++ .../com/mapbox/mapboxsdk/views/MapView.java | 4 ++-- platform/ios/src/MGLMapView.mm | 4 ++-- platform/osx/src/MGLMapView.mm | 4 ++-- src/mbgl/sprite/sprite_image.cpp | 12 +++++----- src/mbgl/sprite/sprite_parser.cpp | 23 +++++++------------ test/sprite/sprite_image.cpp | 8 +++---- test/sprite/sprite_parser.cpp | 6 ++--- test/sprite/sprite_store.cpp | 18 +++++++-------- 9 files changed, 39 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f0b07198c9b..d684e7e8b26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Android master +* Fixed crash caused by annotation image with non-integer width or height ([#3031](https://github.com/mapbox/mapbox-gl-native/issues/3031)) + ## 3.0.0 * Added Camera API ([#3244](https://github.com/mapbox/mapbox-gl-native/issues/3244)) @@ -53,6 +55,7 @@ Known issues: - New API to provide a custom callout view to the map for annotations. ([#3456](https://github.com/mapbox/mapbox-gl-native/pull/3456)) - Made telemetry on/off setting available in-app. ([#3445](https://github.com/mapbox/mapbox-gl-native/pull/3445)) - Fixed an issue with users not being counted by Mapbox if they had disabled telemetry. ([#3495](https://github.com/mapbox/mapbox-gl-native/pull/3495)) +- Fixed crash caused by MGLAnnotationImage with non-integer width or height ([#2198](https://github.com/mapbox/mapbox-gl-native/issues/2198)) ## iOS 3.0.1 diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/views/MapView.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/views/MapView.java index 451dbfdcf55..9f6a6757152 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/views/MapView.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/views/MapView.java @@ -2314,8 +2314,8 @@ private void loadIcon(Icon icon) { mNativeMapView.addAnnotationIcon( id, - (int) (bitmap.getWidth() / scale), - (int) (bitmap.getHeight() / scale), + bitmap.getWidth(), + bitmap.getHeight(), scale, buffer.array()); } diff --git a/platform/ios/src/MGLMapView.mm b/platform/ios/src/MGLMapView.mm index 5b1ccd16280..3e63cd4b768 100644 --- a/platform/ios/src/MGLMapView.mm +++ b/platform/ios/src/MGLMapView.mm @@ -2346,8 +2346,8 @@ - (void)installAnnotationImage:(MGLAnnotationImage *)annotationImage // add sprite auto cSpriteImage = std::make_shared( - uint16_t(annotationImage.image.size.width), - uint16_t(annotationImage.image.size.height), + uint16_t(width), + uint16_t(height), float(annotationImage.image.scale), std::move(pixels)); diff --git a/platform/osx/src/MGLMapView.mm b/platform/osx/src/MGLMapView.mm index 302a9e8c449..596b909fa28 100644 --- a/platform/osx/src/MGLMapView.mm +++ b/platform/osx/src/MGLMapView.mm @@ -1590,8 +1590,8 @@ - (void)installAnnotationImage:(MGLAnnotationImage *)annotationImage { // Get the image’s raw pixel data as an RGBA buffer. std::string pixelString((const char *)rep.bitmapData, rep.pixelsWide * rep.pixelsHigh * 4 /* RGBA */); - auto cSpriteImage = std::make_shared((uint16_t)rep.size.width, - (uint16_t)rep.size.height, + auto cSpriteImage = std::make_shared((uint16_t)rep.pixelsWide, + (uint16_t)rep.pixelsHigh, (float)(rep.pixelsWide / size.width), std::move(pixelString)); NSString *symbolName = [MGLAnnotationSpritePrefix stringByAppendingString:annotationImage.reuseIdentifier]; diff --git a/src/mbgl/sprite/sprite_image.cpp b/src/mbgl/sprite/sprite_image.cpp index d5e4a7828e5..e55d676b34e 100644 --- a/src/mbgl/sprite/sprite_image.cpp +++ b/src/mbgl/sprite/sprite_image.cpp @@ -6,16 +6,16 @@ namespace mbgl { -SpriteImage::SpriteImage(const uint16_t width_, - const uint16_t height_, +SpriteImage::SpriteImage(const uint16_t pixelWidth_, + const uint16_t pixelHeight_, const float pixelRatio_, std::string&& data_, bool sdf_) - : width(width_), - height(height_), + : width(std::ceil(pixelWidth_ / pixelRatio_)), + height(std::ceil(pixelHeight_ / pixelRatio_)), pixelRatio(pixelRatio_), - pixelWidth(std::ceil(width * pixelRatio)), - pixelHeight(std::ceil(height * pixelRatio)), + pixelWidth(pixelWidth_), + pixelHeight(pixelHeight_), data(std::move(data_)), sdf(sdf_) { const size_t size = pixelWidth * pixelHeight * 4; diff --git a/src/mbgl/sprite/sprite_parser.cpp b/src/mbgl/sprite/sprite_parser.cpp index 1aa95310c5b..6942d009f34 100644 --- a/src/mbgl/sprite/sprite_parser.cpp +++ b/src/mbgl/sprite/sprite_parser.cpp @@ -15,37 +15,30 @@ namespace mbgl { SpriteImagePtr createSpriteImage(const PremultipliedImage& image, const uint16_t srcX, const uint16_t srcY, - const uint16_t srcWidth, - const uint16_t srcHeight, + const uint16_t width, + const uint16_t height, const double ratio, const bool sdf) { // Disallow invalid parameter configurations. - if (srcWidth == 0 || srcHeight == 0 || ratio <= 0 || ratio > 10 || srcWidth > 1024 || - srcHeight > 1024) { + if (width == 0 || height == 0 || ratio <= 0 || ratio > 10 || width > 1024 || + height > 1024) { Log::Warning(Event::Sprite, "Can't create sprite with invalid metrics"); return nullptr; } - const uint16_t width = std::ceil(double(srcWidth) / ratio); - const uint16_t dstWidth = std::ceil(width * ratio); - assert(dstWidth >= srcWidth); - const uint16_t height = std::ceil(double(srcHeight) / ratio); - const uint16_t dstHeight = std::ceil(height * ratio); - assert(dstHeight >= srcHeight); - - std::string data(dstWidth * dstHeight * 4, '\0'); + std::string data(width * height * 4, '\0'); auto srcData = reinterpret_cast(image.data.get()); auto dstData = reinterpret_cast(const_cast(data.data())); - const int32_t maxX = std::min(uint32_t(image.width), uint32_t(srcWidth + srcX)) - srcX; + const int32_t maxX = std::min(uint32_t(image.width), uint32_t(width + srcX)) - srcX; assert(maxX <= int32_t(image.width)); - const int32_t maxY = std::min(uint32_t(image.height), uint32_t(srcHeight + srcY)) - srcY; + const int32_t maxY = std::min(uint32_t(image.height), uint32_t(height + srcY)) - srcY; assert(maxY <= int32_t(image.height)); // Copy from the source image into our individual sprite image for (uint16_t y = 0; y < maxY; ++y) { - const auto dstRow = y * dstWidth; + const auto dstRow = y * width; const auto srcRow = (y + srcY) * image.width + srcX; for (uint16_t x = 0; x < maxX; ++x) { dstData[dstRow + x] = srcData[srcRow + x]; diff --git a/test/sprite/sprite_image.cpp b/test/sprite/sprite_image.cpp index 8bc88fcc847..1ed4e56ff6c 100644 --- a/test/sprite/sprite_image.cpp +++ b/test/sprite/sprite_image.cpp @@ -28,7 +28,7 @@ TEST(Sprite, SpriteImageZeroRatio) { SpriteImage(16, 16, 0, ""); FAIL() << "Expected exception"; } catch (util::SpriteImageException& ex) { - EXPECT_STREQ("Sprite image dimensions may not be zero", ex.what()); + EXPECT_STREQ("Sprite image pixel count mismatch", ex.what()); } } @@ -43,7 +43,7 @@ TEST(Sprite, SpriteImageMismatchedData) { TEST(Sprite, SpriteImage) { std::string pixels(32 * 24 * 4, '\0'); - SpriteImage sprite(16, 12, 2, std::move(pixels)); + SpriteImage sprite(32, 24, 2, std::move(pixels)); EXPECT_EQ(16, sprite.width); EXPECT_EQ(32, sprite.pixelWidth); EXPECT_EQ(12, sprite.height); @@ -54,8 +54,8 @@ TEST(Sprite, SpriteImage) { TEST(Sprite, SpriteImageFractionalRatio) { std::string pixels(20 * 12 * 4, '\0'); - SpriteImage sprite(13, 8, 1.5, std::move(pixels)); - EXPECT_EQ(13, sprite.width); + SpriteImage sprite(20, 12, 1.5, std::move(pixels)); + EXPECT_EQ(14, sprite.width); EXPECT_EQ(20, sprite.pixelWidth); EXPECT_EQ(8, sprite.height); EXPECT_EQ(12, sprite.pixelHeight); diff --git a/test/sprite/sprite_parser.cpp b/test/sprite/sprite_parser.cpp index 0eb298b23e0..9eb19a8095a 100644 --- a/test/sprite/sprite_parser.cpp +++ b/test/sprite/sprite_parser.cpp @@ -106,10 +106,10 @@ TEST(Sprite, SpriteImageCreation1_5x) { ASSERT_TRUE(sprite2.get()); EXPECT_EQ(24, sprite2->width); EXPECT_EQ(24, sprite2->height); - EXPECT_EQ(36, sprite2->pixelWidth); - EXPECT_EQ(36, sprite2->pixelHeight); + EXPECT_EQ(35, sprite2->pixelWidth); + EXPECT_EQ(35, sprite2->pixelHeight); EXPECT_EQ(1.5, sprite2->pixelRatio); - EXPECT_EQ(0x134A530C742DD141u, test::crc64(sprite2->data)); + EXPECT_EQ(14312995667113444493u, test::crc64(sprite2->data)); } TEST(Sprite, SpriteParsing) { diff --git a/test/sprite/sprite_store.cpp b/test/sprite/sprite_store.cpp index 8f9aebbe67f..5056b89cb74 100644 --- a/test/sprite/sprite_store.cpp +++ b/test/sprite/sprite_store.cpp @@ -13,9 +13,9 @@ using namespace mbgl; TEST(SpriteStore, SpriteStore) { FixtureLog log; - const auto sprite1 = std::make_shared(8, 8, 2, std::string(16 * 16 * 4, '\0')); - const auto sprite2 = std::make_shared(8, 8, 2, std::string(16 * 16 * 4, '\0')); - const auto sprite3 = std::make_shared(8, 8, 2, std::string(16 * 16 * 4, '\0')); + const auto sprite1 = std::make_shared(16, 16, 2, std::string(16 * 16 * 4, '\0')); + const auto sprite2 = std::make_shared(16, 16, 2, std::string(16 * 16 * 4, '\0')); + const auto sprite3 = std::make_shared(16, 16, 2, std::string(16 * 16 * 4, '\0')); using Sprites = std::map>; SpriteStore store(1); @@ -90,8 +90,8 @@ TEST(SpriteStore, OtherPixelRatio) { } TEST(SpriteStore, Multiple) { - const auto sprite1 = std::make_shared(8, 8, 2, std::string(16 * 16 * 4, '\0')); - const auto sprite2 = std::make_shared(8, 8, 2, std::string(16 * 16 * 4, '\0')); + const auto sprite1 = std::make_shared(16, 16, 2, std::string(16 * 16 * 4, '\0')); + const auto sprite2 = std::make_shared(16, 16, 2, std::string(16 * 16 * 4, '\0')); using Sprites = std::map>; SpriteStore store(1); @@ -109,8 +109,8 @@ TEST(SpriteStore, Multiple) { TEST(SpriteStore, Replace) { FixtureLog log; - const auto sprite1 = std::make_shared(8, 8, 2, std::string(16 * 16 * 4, '\0')); - const auto sprite2 = std::make_shared(8, 8, 2, std::string(16 * 16 * 4, '\0')); + const auto sprite1 = std::make_shared(16, 16, 2, std::string(16 * 16 * 4, '\0')); + const auto sprite2 = std::make_shared(16, 16, 2, std::string(16 * 16 * 4, '\0')); using Sprites = std::map>; SpriteStore store(1); @@ -126,8 +126,8 @@ TEST(SpriteStore, Replace) { TEST(SpriteStore, ReplaceWithDifferentDimensions) { FixtureLog log; - const auto sprite1 = std::make_shared(8, 8, 2, std::string(16 * 16 * 4, '\0')); - const auto sprite2 = std::make_shared(9, 9, 2, std::string(18 * 18 * 4, '\0')); + const auto sprite1 = std::make_shared(16, 16, 2, std::string(16 * 16 * 4, '\0')); + const auto sprite2 = std::make_shared(18, 18, 2, std::string(18 * 18 * 4, '\0')); using Sprites = std::map>; SpriteStore store(1);