[core] Make Image safer

Provide Image::copy, which handles copying rectangles from a source to a destination, with thorough bounds checking.

Also fixes an indexing error in SpriteAtlas, where the top row of pixels in a wrapped image was copied from the wrong source row.

8 files changed, 59 insertions, 99 deletions

diff --git a/src/mbgl/renderer/frame_history.cpp b/src/mbgl/renderer/frame_history.cpp
index 1ee53d87b2..a933a9004a 100644
--- a/src/mbgl/renderer/frame_history.cpp
+++ b/src/mbgl/renderer/frame_history.cpp
@@ -8,7 +8,7 @@ namespace mbgl {
 
 FrameHistory::FrameHistory() {
     changeOpacities.fill(0);
-    std::fill(opacities.data.get(), opacities.data.get() + opacities.bytes(), 0);
+    opacities.fill(0);
 }
 
 void FrameHistory::record(const TimePoint& now, float zoom, const Duration& duration) {
diff --git a/src/mbgl/renderer/frame_history.hpp b/src/mbgl/renderer/frame_history.hpp
index fffbd113ed..f2b11f5f41 100644
--- a/src/mbgl/renderer/frame_history.hpp
+++ b/src/mbgl/renderer/frame_history.hpp
@@ -26,7 +26,7 @@ public:
 private:
     std::array<TimePoint, 256> changeTimes;
     std::array<uint8_t, 256> changeOpacities;
-    const AlphaImage opacities{ { 256, 1 } };
+    AlphaImage opacities{ { 256, 1 } };
 
     int16_t previousZoomIndex = 0;
     TimePoint previousTime;
diff --git a/src/mbgl/sprite/sprite_atlas.cpp b/src/mbgl/sprite/sprite_atlas.cpp
index ea055ce5ec..61c074a942 100644
--- a/src/mbgl/sprite/sprite_atlas.cpp
+++ b/src/mbgl/sprite/sprite_atlas.cpp
@@ -231,52 +231,32 @@ optional<SpriteAtlasPosition> SpriteAtlas::getPosition(const std::string& name,
     };
 }
 
-void copyBitmap(const uint32_t *src, const uint32_t srcStride, const uint32_t srcX, const uint32_t srcY,
-        uint32_t *const dst, const uint32_t dstStride, const uint32_t dstX, const uint32_t dstY, int dstSize,
-        const int width, const int height, const SpritePatternMode mode) {
-
-    int srcI = srcY * srcStride + srcX;
-    int dstI = dstY * dstStride + dstX;
-    int x, y;
-
-    if (mode == SpritePatternMode::Repeating) {
-        // add 1 pixel wrapped padding on each side of the image
-        dstI -= dstStride;
-        for (y = -1; y <= height; y++, srcI = ((y + height) % height + srcY) * srcStride + srcX, dstI += dstStride) {
-            for (x = -1; x <= width; x++) {
-                const int dstIndex = (dstI + x + dstSize) % dstSize;
-                dst[dstIndex] = src[srcI + ((x + width) % width)];
-            }
-        }
-
-    } else {
-        for (y = 0; y < height; y++, srcI += srcStride, dstI += dstStride) {
-            for (x = 0; x < width; x++) {
-                const int dstIndex = (dstI + x + dstSize) % dstSize;
-                dst[dstIndex] = src[srcI + x];
-            }
-        }
-    }
-}
-
 void SpriteAtlas::copy(const Holder& holder, const SpritePatternMode mode) {
     if (!image.valid()) {
         image = PremultipliedImage({ static_cast<uint32_t>(std::ceil(size.width * pixelRatio)),
                                      static_cast<uint32_t>(std::ceil(size.height * pixelRatio)) });
-        std::fill(image.data.get(), image.data.get() + image.bytes(), 0);
+        image.fill(0);
+    }
+
+    if (!holder.spriteImage->image.valid()) {
+        return;
     }
 
-    const uint32_t* srcData =
-        reinterpret_cast<const uint32_t*>(holder.spriteImage->image.data.get());
-    if (!srcData) return;
-    uint32_t* const dstData = reinterpret_cast<uint32_t*>(image.data.get());
+    const uint32_t padding = 1;
+    const uint32_t x = (holder.pos.x + padding) * pixelRatio;
+    const uint32_t y = (holder.pos.y + padding) * pixelRatio;
+    const uint32_t w = holder.spriteImage->image.size.width;
+    const uint32_t h = holder.spriteImage->image.size.height;
 
-    const int padding = 1;
+    PremultipliedImage::copy(holder.spriteImage->image, image, { 0, 0 }, { x, y }, { w, h });
 
-    copyBitmap(srcData, holder.spriteImage->image.size.width, 0, 0, dstData, image.size.width,
-               (holder.pos.x + padding) * pixelRatio, (holder.pos.y + padding) * pixelRatio,
-               image.size.width * image.size.height, holder.spriteImage->image.size.width,
-               holder.spriteImage->image.size.height, mode);
+    if (mode == SpritePatternMode::Repeating) {
+        // Add 1 pixel wrapped padding on each side of the image.
+        PremultipliedImage::copy(holder.spriteImage->image, image, { 0, h - 1 }, { x, y - 1 }, { w, 1 }); // T
+        PremultipliedImage::copy(holder.spriteImage->image, image, { 0,     0 }, { x, y + h }, { w, 1 }); // B
+        PremultipliedImage::copy(holder.spriteImage->image, image, { w - 1, 0 }, { x - 1, y }, { 1, h }); // L
+        PremultipliedImage::copy(holder.spriteImage->image, image, { 0,     0 }, { x + w, y }, { 1, h }); // R
+    }
 
     dirty = true;
 }
diff --git a/src/mbgl/sprite/sprite_parser.cpp b/src/mbgl/sprite/sprite_parser.cpp
index 9de8515e14..66b5ec0606 100644
--- a/src/mbgl/sprite/sprite_parser.cpp
+++ b/src/mbgl/sprite/sprite_parser.cpp
@@ -34,17 +34,8 @@ SpriteImagePtr createSpriteImage(const PremultipliedImage& image,
 
     PremultipliedImage dstImage({ width, height });
 
-    auto srcData = reinterpret_cast<const uint32_t*>(image.data.get());
-    auto dstData = reinterpret_cast<uint32_t*>(dstImage.data.get());
-
     // Copy from the source image into our individual sprite image
-    for (uint32_t y = 0; y < height; ++y) {
-        const auto dstRow = y * width;
-        const auto srcRow = (y + srcY) * image.size.width + srcX;
-        for (uint32_t x = 0; x < width; ++x) {
-            dstData[dstRow + x] = srcData[srcRow + x];
-        }
-    }
+    PremultipliedImage::copy(image, dstImage, { srcX, srcY }, { 0, 0 }, { width, height });
 
     return std::make_unique<const SpriteImage>(std::move(dstImage), ratio, sdf);
 }
diff --git a/src/mbgl/text/glyph.hpp b/src/mbgl/text/glyph.hpp
index c89d045dfc..78ea613cd3 100644
--- a/src/mbgl/text/glyph.hpp
+++ b/src/mbgl/text/glyph.hpp
@@ -3,6 +3,7 @@
 #include <mbgl/text/glyph_range.hpp>
 #include <mbgl/util/rect.hpp>
 #include <mbgl/util/traits.hpp>
+#include <mbgl/util/image.hpp>
 
 #include <cstdint>
 #include <vector>
@@ -89,7 +90,7 @@ public:
     uint32_t id = 0;
 
     // A signed distance field of the glyph with a border (see above).
-    std::string bitmap;
+    AlphaImage bitmap;
 
     // Glyph metrics
     GlyphMetrics metrics;
diff --git a/src/mbgl/text/glyph_atlas.cpp b/src/mbgl/text/glyph_atlas.cpp
index dc1fe2b1d9..88b319366d 100644
--- a/src/mbgl/text/glyph_atlas.cpp
+++ b/src/mbgl/text/glyph_atlas.cpp
@@ -124,48 +124,26 @@ Rect<uint16_t> GlyphAtlas::addGlyph(uintptr_t tileUID,
         return Rect<uint16_t>{ 0, 0, 0, 0 };
     }
 
-    uint16_t buffered_width = glyph.metrics.width + SDFGlyph::borderSize * 2;
-    uint16_t buffered_height = glyph.metrics.height + SDFGlyph::borderSize * 2;
-
-    // Guard against mismatches between the glyph bitmap size and the size mandated by
-    // its metrics.
-    if (size_t(buffered_width * buffered_height) != glyph.bitmap.size()) {
-        return Rect<uint16_t>{ 0, 0, 0, 0 };
-    }
-
     // Add a 1px border around every image.
-    const uint16_t padding = 1;
-    uint16_t pack_width = buffered_width + 2 * padding;
-    uint16_t pack_height = buffered_height + 2 * padding;
+    const uint32_t padding = 1;
+    uint16_t width = glyph.bitmap.size.width + 2 * padding;
+    uint16_t height = glyph.bitmap.size.height + 2 * padding;
 
     // Increase to next number divisible by 4, but at least 1.
     // This is so we can scale down the texture coordinates and pack them
     // into 2 bytes rather than 4 bytes.
-    pack_width += (4 - pack_width % 4);
-    pack_height += (4 - pack_height % 4);
+    width += (4 - width % 4);
+    height += (4 - height % 4);
 
-    Rect<uint16_t> rect = bin.allocate(pack_width, pack_height);
+    Rect<uint16_t> rect = bin.allocate(width, height);
     if (rect.w == 0) {
         Log::Error(Event::OpenGL, "glyph bitmap overflow");
         return rect;
     }
 
-    // Verify that binpack didn't produce a rect that goes beyond the size of the image.
-    // This should never happen.
-    assert(rect.x + rect.w <= image.size.width);
-    assert(rect.y + rect.h <= image.size.height);
-
     face.emplace(glyph.id, GlyphValue { rect, tileUID });
 
-    // Copy the bitmap
-    const uint8_t* source = reinterpret_cast<const uint8_t*>(glyph.bitmap.data());
-    for (uint32_t y = 0; y < buffered_height; y++) {
-        uint32_t y1 = image.size.width * (rect.y + y + padding) + rect.x + padding;
-        uint32_t y2 = buffered_width * y;
-        for (uint32_t x = 0; x < buffered_width; x++) {
-            image.data[y1 + x] = source[y2 + x];
-        }
-    }
+    AlphaImage::copy(glyph.bitmap, image, { 0, 0 }, { rect.x + padding, rect.y + padding }, glyph.bitmap.size);
 
     dirty = true;
 
diff --git a/src/mbgl/text/glyph_atlas.hpp b/src/mbgl/text/glyph_atlas.hpp
index af14aace5b..84f29aeb73 100644
--- a/src/mbgl/text/glyph_atlas.hpp
+++ b/src/mbgl/text/glyph_atlas.hpp
@@ -101,7 +101,7 @@ private:
     std::mutex mtx;
     BinPack<uint16_t> bin;
     std::unordered_map<FontStack, std::map<uint32_t, GlyphValue>, FontStackHash> index;
-    const AlphaImage image;
+    AlphaImage image;
     std::atomic<bool> dirty;
     mbgl::optional<gl::Texture> texture;
 };
diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index cdeac57984..5c57d278db 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -25,6 +25,7 @@ void parseGlyphPBF(GlyphSet& glyphSet, const GlyphRange& glyphRange, const std::
             auto glyph_pbf = fontstack_pbf.get_message();
 
             SDFGlyph glyph;
+            protozero::data_view glyphData;
 
             bool hasID = false, hasWidth = false, hasHeight = false, hasLeft = false,
                  hasTop = false, hasAdvance = false;
@@ -36,7 +37,7 @@ void parseGlyphPBF(GlyphSet& glyphSet, const GlyphRange& glyphRange, const std::
                     hasID = true;
                     break;
                 case 2: // bitmap
-                    glyph.bitmap = glyph_pbf.get_string();
+                    glyphData = glyph_pbf.get_view();
                     break;
                 case 3: // width
                     glyph.metrics.width = glyph_pbf.get_uint32();
@@ -64,26 +65,35 @@ void parseGlyphPBF(GlyphSet& glyphSet, const GlyphRange& glyphRange, const std::
                 }
             }
 
+            // Only treat this glyph as a correct glyph if it has all required fields. It also
+            // needs to satisfy a few metrics conditions that ensure that the glyph isn't bogus.
+            // All other glyphs are malformed.  We're also discarding all glyphs that are outside
+            // the expected glyph range.
+            if (!hasID || !hasWidth || !hasHeight || !hasLeft || !hasTop || !hasAdvance ||
+                glyph.metrics.width >= 256 || glyph.metrics.height >= 256 ||
+                glyph.metrics.left < -128 || glyph.metrics.left >= 128 ||
+                glyph.metrics.top < -128 || glyph.metrics.top >= 128 ||
+                glyph.metrics.advance >= 256 ||
+                glyph.id < glyphRange.first || glyph.id > glyphRange.second) {
+                continue;
+            }
+
             // If the area of width/height is non-zero, we need to adjust the expected size
             // with the implicit border size, otherwise we expect there to be no bitmap at all.
-            const uint32_t expectedBitmapSize =
-                glyph.metrics.width && glyph.metrics.height
-                    ? (glyph.metrics.width + 2 * SDFGlyph::borderSize) *
-                          (glyph.metrics.height + 2 * SDFGlyph::borderSize)
-                    : 0;
-
-            // Only treat this glyph as a correct glyph if it has all required fields, and if
-            // the bitmap has the correct length. It also needs to satisfy a few metrics conditions
-            // that ensure that the glyph isn't bogus. All other glyphs are malformed.
-            // We're also discarding all glyphs that are outside the expected glyph range.
-            if (hasID && hasWidth && hasHeight && hasLeft && hasTop && hasAdvance &&
-                glyph.metrics.width < 256 && glyph.metrics.height < 256 &&
-                glyph.metrics.left >= -128 && glyph.metrics.left < 128 &&
-                glyph.metrics.top >= -128 && glyph.metrics.top < 128 &&
-                glyph.metrics.advance < 256 && glyph.bitmap.size() == expectedBitmapSize &&
-                glyph.id >= glyphRange.first && glyph.id <= glyphRange.second) {
-                glyphSet.insert(glyph.id, std::move(glyph));
+            if (glyph.metrics.width && glyph.metrics.height) {
+                const Size size {
+                    glyph.metrics.width + 2 * SDFGlyph::borderSize,
+                    glyph.metrics.height + 2 * SDFGlyph::borderSize
+                };
+
+                if (size.area() != glyphData.size()) {
+                    continue;
+                }
+
+                glyph.bitmap = AlphaImage(size, reinterpret_cast<const uint8_t*>(glyphData.data()), glyphData.size());
             }
+
+            glyphSet.insert(glyph.id, std::move(glyph));
         }
     }
 }