From ebd2985f785e19f3d4976f2d803ac017b755d0d9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= <mail@kkaefer.com>
Date: Fri, 20 Mar 2015 18:33:29 +0100
Subject: [PATCH] make style updates a little more threadsafe

- we're now always loading and parsing styles in the map thread
- adds an atomic enum that stores the actual updates we need to perform
- adds a MapData object that mutexes access to variables used from both threads
---
 include/mbgl/map/map.hpp  |  25 ++++++--
 src/mbgl/map/map.cpp      | 130 ++++++++++++++++++++++++--------------
 src/mbgl/map/map_data.hpp |  36 +++++++++++
 3 files changed, 137 insertions(+), 54 deletions(-)
 create mode 100644 src/mbgl/map/map_data.hpp

diff --git a/include/mbgl/map/map.hpp b/include/mbgl/map/map.hpp
index 06ae39d87..9ea5c72f6 100644
--- a/include/mbgl/map/map.hpp
+++ b/include/mbgl/map/map.hpp
@@ -39,6 +39,7 @@ class LineAtlas;
 class Environment;
 class EnvironmentScope;
 class AnnotationManager;
+class MapData;
 
 class Map : private util::noncopyable {
     friend class View;
@@ -75,7 +76,12 @@ public:
     void render();
 
     // Notifies the Map thread that the state has changed and an update might be necessary.
-    void triggerUpdate();
+    using UpdateType = uint32_t;
+    enum class Update : UpdateType {
+        Nothing   = 0,
+        StyleInfo = 1 << 0,
+    };
+    void triggerUpdate(Update = Update::Nothing);
 
     // Triggers a render. Can be called from any thread.
     void triggerRender();
@@ -92,8 +98,8 @@ public:
 
     void setDefaultTransitionDuration(std::chrono::steady_clock::duration duration = std::chrono::steady_clock::duration::zero());
     std::chrono::steady_clock::duration getDefaultTransitionDuration();
-    void setStyleURL(const std::string &url);
-    void setStyleJSON(std::string newStyleJSON, const std::string &base = "");
+    void setStyleURL(const std::string& url);
+    void setStyleJSON(const std::string& json, const std::string& base = "");
     std::string getStyleJSON() const;
 
     // Transition
@@ -178,6 +184,13 @@ private:
     void updateSources();
     void updateSources(const util::ptr<StyleLayerGroup> &group);
 
+    // Triggered by triggerUpdate();
+    void update();
+
+    // Loads the style set in the data object. Called by Update::StyleInfo
+    void reloadStyle();
+    void loadStyleJSON(const std::string& json, const std::string& base);
+
     // Prepares a map render by updating the tiles we need for the current view, as well as updating
     // the stylesheet.
     void prepare();
@@ -234,8 +247,8 @@ private:
     std::unique_ptr<Painter> painter;
     util::ptr<AnnotationManager> annotationManager;
 
-    std::string styleURL;
-    std::string styleJSON = "";
+    const std::unique_ptr<MapData> data;
+
     std::vector<std::string> classes;
     std::string accessToken;
 
@@ -245,6 +258,8 @@ private:
     std::chrono::steady_clock::time_point animationTime = std::chrono::steady_clock::time_point::min();
 
     std::set<util::ptr<StyleSource>> activeSources;
+
+    std::atomic<UpdateType> updated;
 };
 
 }
diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp
index ae1cfd20f..57000e040 100644
--- a/src/mbgl/map/map.cpp
+++ b/src/mbgl/map/map.cpp
@@ -1,6 +1,7 @@
 #include <mbgl/map/map.hpp>
 #include <mbgl/map/environment.hpp>
 #include <mbgl/map/view.hpp>
+#include <mbgl/map/map_data.hpp>
 #include <mbgl/platform/platform.hpp>
 #include <mbgl/map/source.hpp>
 #include <mbgl/renderer/painter.hpp>
@@ -71,7 +72,8 @@ Map::Map(View& view_, FileSource& fileSource_)
       lineAtlas(util::make_unique<LineAtlas>(512, 512)),
       texturePool(std::make_shared<TexturePool>()),
       painter(util::make_unique<Painter>(*spriteAtlas, *glyphAtlas, *lineAtlas)),
-      annotationManager(util::make_unique<AnnotationManager>())
+      annotationManager(util::make_unique<AnnotationManager>()),
+      data(util::make_unique<MapData>())
 {
     view.initialize(this);
 }
@@ -138,11 +140,7 @@ void Map::start(bool startPaused) {
     });
 
     asyncUpdate = util::make_unique<uv::async>(env->loop, [this] {
-        assert(Environment::currentlyOn(ThreadType::Map));
-
-        if (state.hasSize()) {
-            prepare();
-        }
+        update();
     });
 
     asyncRender = util::make_unique<uv::async>(env->loop, [this] {
@@ -175,6 +173,8 @@ void Map::start(bool startPaused) {
         isStopped = true;
         view.notify();
     });
+
+    triggerUpdate();
 }
 
 void Map::stop(std::function<void ()> callback) {
@@ -252,7 +252,9 @@ void Map::run() {
         checkForPause();
     }
 
-    if (mode == Mode::Static && !style && styleURL.empty()) {
+    auto styleInfo = data->getStyleInfo();
+
+    if (mode == Mode::Static && !style && (styleInfo.url.empty() && styleInfo.json.empty())) {
         throw util::Exception("Style is not set");
     }
 
@@ -297,7 +299,9 @@ void Map::renderSync() {
     rendered = false;
 }
 
-void Map::triggerUpdate() {
+void Map::triggerUpdate(const Update u) {
+    updated |= static_cast<UpdateType>(u);
+
     if (mode == Mode::Static) {
         prepare();
     } else if (asyncUpdate) {
@@ -345,36 +349,27 @@ void Map::setup() {
 }
 
 void Map::setStyleURL(const std::string &url) {
-    // TODO: Make threadsafe.
+    assert(Environment::currentlyOn(ThreadType::Main));
 
-    styleURL = url;
-    if (mode == Mode::Continuous) {
-        stop();
-        start();
+    const size_t pos = url.rfind('/');
+    std::string base = "";
+    if (pos != std::string::npos) {
+        base = url.substr(0, pos + 1);
     }
+
+    data->setStyleInfo({ url, base, "" });
+    triggerUpdate(Update::StyleInfo);
 }
 
-void Map::setStyleJSON(std::string newStyleJSON, const std::string &base) {
-    // TODO: Make threadsafe.
-    styleJSON.swap(newStyleJSON);
-    sprite.reset();
-    if (!style) {
-        style = std::make_shared<Style>();
-    }
+void Map::setStyleJSON(const std::string& json, const std::string& base) {
+    assert(Environment::currentlyOn(ThreadType::Main));
 
-    style->base = base;
-    style->loadJSON((const uint8_t *)styleJSON.c_str());
-    style->cascadeClasses(classes);
-    style->setDefaultTransitionDuration(defaultTransitionDuration);
-
-    const std::string glyphURL = util::mapbox::normalizeGlyphsURL(style->glyph_url, getAccessToken());
-    glyphStore->setURL(glyphURL);
-
-    triggerUpdate();
+    data->setStyleInfo({ "", base, json });
+    triggerUpdate(Update::StyleInfo);
 }
 
 std::string Map::getStyleJSON() const {
-    return styleJSON;
+    return data->getStyleInfo().json;
 }
 
 util::ptr<Sprite> Map::getSprite() {
@@ -670,7 +665,9 @@ void Map::updateSources() {
     }
 
     // Then, reenable all of those that we actually use when drawing this layer.
-    updateSources(style->layers);
+    if (style) {
+        updateSources(style->layers);
+    }
 
     // Then, construct or destroy the actual source object, depending on enabled state.
     for (const auto& source : activeSources) {
@@ -715,26 +712,58 @@ void Map::updateTiles() {
     }
 }
 
-void Map::prepare() {
+void Map::update() {
     assert(Environment::currentlyOn(ThreadType::Map));
 
-    if (!style) {
-        style = std::make_shared<Style>();
+    if (state.hasSize()) {
+        prepare();
+    }
+}
 
-        env->request({ Resource::Kind::JSON, styleURL}, [&](const Response &res) {
+void Map::reloadStyle() {
+    assert(Environment::currentlyOn(ThreadType::Map));
+
+    style = std::make_shared<Style>();
+
+    const auto styleInfo = data->getStyleInfo();
+
+    if (!styleInfo.url.empty()) {
+        // We have a style URL
+        env->request({ Resource::Kind::JSON, styleInfo.url }, [&](const Response &res) {
             if (res.status == Response::Successful) {
-                // Calculate the base
-                const size_t pos = styleURL.rfind('/');
-                std::string base = "";
-                if (pos != std::string::npos) {
-                    base = styleURL.substr(0, pos + 1);
-                }
-
-                setStyleJSON(res.data, base);
+                loadStyleJSON(res.data, styleInfo.base);
             } else {
                 Log::Error(Event::Setup, "loading style failed: %s", res.message.c_str());
             }
         });
+    } else {
+        // We got JSON data directly.
+        loadStyleJSON(styleInfo.json, styleInfo.base);
+    }
+}
+
+void Map::loadStyleJSON(const std::string& json, const std::string& base) {
+    assert(Environment::currentlyOn(ThreadType::Map));
+
+    sprite.reset();
+    style = std::make_shared<Style>();
+    style->base = base;
+    style->loadJSON((const uint8_t *)json.c_str());
+    style->cascadeClasses(classes);
+    style->setDefaultTransitionDuration(defaultTransitionDuration);
+
+    const std::string glyphURL = util::mapbox::normalizeGlyphsURL(style->glyph_url, getAccessToken());
+    glyphStore->setURL(glyphURL);
+
+    triggerUpdate();
+}
+
+void Map::prepare() {
+    assert(Environment::currentlyOn(ThreadType::Map));
+
+    const auto u = updated.exchange(static_cast<UpdateType>(Update::Nothing));
+    if (u & static_cast<UpdateType>(Update::StyleInfo)) {
+        reloadStyle();
     }
 
     // Update transform transitions.
@@ -746,14 +775,17 @@ void Map::prepare() {
     state = transform.currentState();
 
     animationTime = std::chrono::steady_clock::now();
-    updateSources();
-    style->updateProperties(state.getNormalizedZoom(), animationTime);
 
-    // Allow the sprite atlas to potentially pull new sprite images if needed.
-    spriteAtlas->resize(state.getPixelRatio());
-    spriteAtlas->setSprite(getSprite());
+    if (style) {
+        updateSources();
+        style->updateProperties(state.getNormalizedZoom(), animationTime);
 
-    updateTiles();
+        // Allow the sprite atlas to potentially pull new sprite images if needed.
+        spriteAtlas->resize(state.getPixelRatio());
+        spriteAtlas->setSprite(getSprite());
+
+        updateTiles();
+    }
 
     if (mode == Mode::Continuous) {
         view.invalidate();
diff --git a/src/mbgl/map/map_data.hpp b/src/mbgl/map/map_data.hpp
new file mode 100644
index 000000000..98d2518a2
--- /dev/null
+++ b/src/mbgl/map/map_data.hpp
@@ -0,0 +1,36 @@
+#ifndef MBGL_MAP_MAP_DATA
+#define MBGL_MAP_MAP_DATA
+
+#include <string>
+#include <mutex>
+
+namespace mbgl {
+
+struct StyleInfo {
+    std::string url;
+    std::string base;
+    std::string json;
+};
+
+class MapData {
+    using Lock = std::lock_guard<std::mutex>;
+
+public:
+    inline StyleInfo getStyleInfo() const {
+        Lock lock(mtx);
+        return styleInfo;
+    }
+    inline void setStyleInfo(StyleInfo&& info) {
+        Lock lock(mtx);
+        styleInfo = info;
+    }
+
+private:
+    mutable std::mutex mtx;
+
+    StyleInfo styleInfo;
+};
+
+}
+
+#endif