[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.
mapbox · Jan 20, 2016 · 26faa6a · 26faa6a
1 parent 9b62661
commit 26faa6a
Show file tree

Hide file tree

Showing 9 changed files with 39 additions and 43 deletions.
diff --git 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
@@ -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
@@ -2346,8 +2346,8 @@ - (void)installAnnotationImage:(MGLAnnotationImage *)annotationImage
 
     // add sprite
     auto cSpriteImage = std::make_shared<mbgl::SpriteImage>(
-        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
@@ -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<mbgl::SpriteImage>((uint16_t)rep.size.width,
-                                                            (uint16_t)rep.size.height,
+    auto cSpriteImage = std::make_shared<mbgl::SpriteImage>((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
@@ -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
@@ -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<const uint32_t*>(image.data.get());
     auto dstData = reinterpret_cast<uint32_t*>(const_cast<char*>(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
@@ -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
@@ -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
@@ -13,9 +13,9 @@ using namespace mbgl;
 TEST(SpriteStore, SpriteStore) {
     FixtureLog log;
 
-    const auto sprite1 = std::make_shared<SpriteImage>(8, 8, 2, std::string(16 * 16 * 4, '\0'));
-    const auto sprite2 = std::make_shared<SpriteImage>(8, 8, 2, std::string(16 * 16 * 4, '\0'));
-    const auto sprite3 = std::make_shared<SpriteImage>(8, 8, 2, std::string(16 * 16 * 4, '\0'));
+    const auto sprite1 = std::make_shared<SpriteImage>(16, 16, 2, std::string(16 * 16 * 4, '\0'));
+    const auto sprite2 = std::make_shared<SpriteImage>(16, 16, 2, std::string(16 * 16 * 4, '\0'));
+    const auto sprite3 = std::make_shared<SpriteImage>(16, 16, 2, std::string(16 * 16 * 4, '\0'));
 
     using Sprites = std::map<std::string, std::shared_ptr<const SpriteImage>>;
     SpriteStore store(1);
@@ -90,8 +90,8 @@ TEST(SpriteStore, OtherPixelRatio) {
 }
 
 TEST(SpriteStore, Multiple) {
-    const auto sprite1 = std::make_shared<SpriteImage>(8, 8, 2, std::string(16 * 16 * 4, '\0'));
-    const auto sprite2 = std::make_shared<SpriteImage>(8, 8, 2, std::string(16 * 16 * 4, '\0'));
+    const auto sprite1 = std::make_shared<SpriteImage>(16, 16, 2, std::string(16 * 16 * 4, '\0'));
+    const auto sprite2 = std::make_shared<SpriteImage>(16, 16, 2, std::string(16 * 16 * 4, '\0'));
 
     using Sprites = std::map<std::string, std::shared_ptr<const SpriteImage>>;
     SpriteStore store(1);
@@ -109,8 +109,8 @@ TEST(SpriteStore, Multiple) {
 TEST(SpriteStore, Replace) {
     FixtureLog log;
 
-    const auto sprite1 = std::make_shared<SpriteImage>(8, 8, 2, std::string(16 * 16 * 4, '\0'));
-    const auto sprite2 = std::make_shared<SpriteImage>(8, 8, 2, std::string(16 * 16 * 4, '\0'));
+    const auto sprite1 = std::make_shared<SpriteImage>(16, 16, 2, std::string(16 * 16 * 4, '\0'));
+    const auto sprite2 = std::make_shared<SpriteImage>(16, 16, 2, std::string(16 * 16 * 4, '\0'));
 
     using Sprites = std::map<std::string, std::shared_ptr<const SpriteImage>>;
     SpriteStore store(1);
@@ -126,8 +126,8 @@ TEST(SpriteStore, Replace) {
 TEST(SpriteStore, ReplaceWithDifferentDimensions) {
     FixtureLog log;
 
-    const auto sprite1 = std::make_shared<SpriteImage>(8, 8, 2, std::string(16 * 16 * 4, '\0'));
-    const auto sprite2 = std::make_shared<SpriteImage>(9, 9, 2, std::string(18 * 18 * 4, '\0'));
+    const auto sprite1 = std::make_shared<SpriteImage>(16, 16, 2, std::string(16 * 16 * 4, '\0'));
+    const auto sprite2 = std::make_shared<SpriteImage>(18, 18, 2, std::string(18 * 18 * 4, '\0'));
 
     using Sprites = std::map<std::string, std::shared_ptr<const SpriteImage>>;
     SpriteStore store(1);