From 61fa43619334ac68c66d45d38aaf681f2a21fa96 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= <mail@kkaefer.com>
Date: Wed, 18 Mar 2015 15:37:06 +0100
Subject: [PATCH] coalesce buffer deletions and run them in bulk during
 rendering

On iOS, the main thread does framebuffer operations just before it requests a rerender of the view contents. This means that GL operations on the Map thread will interfere with that and cause a crash. While most GL operations happen throughout rendering (which is synchronized with the main thread), deletion of shared_ptrs release in turn their OpenGL objects. This may happen at any time and isn't synced with rendering.

This patch changes this so instead of deleting all objects immediately, we are collecting all abandoned IDs and delete them in bulk once the main thread has given us permission to use OpenGL calls (i.e. during render()).
---
 include/mbgl/map/map.hpp           | 10 +++---
 src/mbgl/geometry/buffer.hpp       |  3 +-
 src/mbgl/geometry/line_atlas.cpp   |  6 ++--
 src/mbgl/geometry/sprite_atlas.cpp |  4 +--
 src/mbgl/geometry/vao.cpp          |  3 +-
 src/mbgl/map/environment.cpp       | 55 ++++++++++++++++++++++++++++--
 src/mbgl/map/environment.hpp       | 42 ++++++++++++++++++-----
 src/mbgl/map/map.cpp               | 23 +++++++++++--
 src/mbgl/map/tile_data.cpp         |  2 +-
 src/mbgl/util/texture_pool.cpp     | 15 +++-----
 10 files changed, 127 insertions(+), 36 deletions(-)

diff --git a/include/mbgl/map/map.hpp b/include/mbgl/map/map.hpp
index 86c89f769..06ae39d87 100644
--- a/include/mbgl/map/map.hpp
+++ b/include/mbgl/map/map.hpp
@@ -37,6 +37,7 @@ class GlyphAtlas;
 class SpriteAtlas;
 class LineAtlas;
 class Environment;
+class EnvironmentScope;
 class AnnotationManager;
 
 class Map : private util::noncopyable {
@@ -192,6 +193,7 @@ private:
     Mode mode = Mode::None;
 
     const std::unique_ptr<Environment> env;
+    std::unique_ptr<EnvironmentScope> scope;
     View &view;
 
 private:
@@ -223,13 +225,13 @@ private:
     FileSource& fileSource;
 
     util::ptr<Style> style;
-    const std::unique_ptr<GlyphAtlas> glyphAtlas;
+    std::unique_ptr<GlyphAtlas> glyphAtlas;
     util::ptr<GlyphStore> glyphStore;
-    const std::unique_ptr<SpriteAtlas> spriteAtlas;
+    std::unique_ptr<SpriteAtlas> spriteAtlas;
     util::ptr<Sprite> sprite;
-    const std::unique_ptr<LineAtlas> lineAtlas;
+    std::unique_ptr<LineAtlas> lineAtlas;
     util::ptr<TexturePool> texturePool;
-    const std::unique_ptr<Painter> painter;
+    std::unique_ptr<Painter> painter;
     util::ptr<AnnotationManager> annotationManager;
 
     std::string styleURL;
diff --git a/src/mbgl/geometry/buffer.hpp b/src/mbgl/geometry/buffer.hpp
index 3649574bb..4198425ec 100644
--- a/src/mbgl/geometry/buffer.hpp
+++ b/src/mbgl/geometry/buffer.hpp
@@ -3,6 +3,7 @@
 
 #include <mbgl/platform/gl.hpp>
 #include <mbgl/util/noncopyable.hpp>
+#include <mbgl/map/environment.hpp>
 
 #include <cstdlib>
 #include <cassert>
@@ -21,7 +22,7 @@ public:
     ~Buffer() {
         cleanup();
         if (buffer != 0) {
-            MBGL_CHECK_ERROR(glDeleteBuffers(1, &buffer));
+            Environment::Get().abandonBuffer(buffer);
             buffer = 0;
         }
     }
diff --git a/src/mbgl/geometry/line_atlas.cpp b/src/mbgl/geometry/line_atlas.cpp
index 8be1a8d53..507cc7b08 100644
--- a/src/mbgl/geometry/line_atlas.cpp
+++ b/src/mbgl/geometry/line_atlas.cpp
@@ -1,3 +1,4 @@
+#include <mbgl/map/environment.hpp>
 #include <mbgl/geometry/line_atlas.hpp>
 #include <mbgl/platform/gl.hpp>
 #include <mbgl/platform/log.hpp>
@@ -18,9 +19,10 @@ LineAtlas::LineAtlas(uint16_t w, uint16_t h)
 LineAtlas::~LineAtlas() {
     std::lock_guard<std::recursive_mutex> lock(mtx);
 
-    MBGL_CHECK_ERROR(glDeleteTextures(1, &texture));
-    delete[] data;
+    Environment::Get().abandonTexture(texture);
     texture = 0;
+
+    delete[] data;
 }
 
 LinePatternPos LineAtlas::getDashPosition(const std::vector<float> &dasharray, bool round) {
diff --git a/src/mbgl/geometry/sprite_atlas.cpp b/src/mbgl/geometry/sprite_atlas.cpp
index dce772f2e..bf31c6e38 100644
--- a/src/mbgl/geometry/sprite_atlas.cpp
+++ b/src/mbgl/geometry/sprite_atlas.cpp
@@ -1,3 +1,4 @@
+#include <mbgl/map/environment.hpp>
 #include <mbgl/geometry/sprite_atlas.hpp>
 #include <mbgl/platform/gl.hpp>
 #include <mbgl/platform/log.hpp>
@@ -293,8 +294,7 @@ void SpriteAtlas::bind(bool linear) {
 
 SpriteAtlas::~SpriteAtlas() {
     std::lock_guard<std::recursive_mutex> lock(mtx);
-
-    MBGL_CHECK_ERROR(glDeleteTextures(1, &texture));
+    Environment::Get().abandonTexture(texture);
     texture = 0;
     ::operator delete(data), data = nullptr;
 }
diff --git a/src/mbgl/geometry/vao.cpp b/src/mbgl/geometry/vao.cpp
index 00976b4d5..fef74396a 100644
--- a/src/mbgl/geometry/vao.cpp
+++ b/src/mbgl/geometry/vao.cpp
@@ -1,6 +1,7 @@
 #include <mbgl/geometry/vao.hpp>
 #include <mbgl/platform/log.hpp>
 #include <mbgl/util/string.hpp>
+#include <mbgl/map/environment.hpp>
 
 namespace mbgl {
 
@@ -11,7 +12,7 @@ VertexArrayObject::~VertexArrayObject() {
     if (!gl::DeleteVertexArrays) return;
 
     if (vao) {
-        MBGL_CHECK_ERROR(gl::DeleteVertexArrays(1, &vao));
+        Environment::Get().abandonVAO(vao);
     }
 }
 
diff --git a/src/mbgl/map/environment.cpp b/src/mbgl/map/environment.cpp
index 1aea5aa0c..469790501 100644
--- a/src/mbgl/map/environment.cpp
+++ b/src/mbgl/map/environment.cpp
@@ -1,5 +1,6 @@
 #include <mbgl/map/environment.hpp>
 #include <mbgl/storage/file_source.hpp>
+#include <mbgl/platform/gl.hpp>
 
 #include <uv.h>
 
@@ -76,12 +77,12 @@ ThreadInfoStore threadInfoStore;
 
 } // namespace
 
-Environment::Scope::Scope(Environment& env, ThreadType type, const std::string& name)
+EnvironmentScope::EnvironmentScope(Environment& env, ThreadType type, const std::string& name)
     : id(std::this_thread::get_id()) {
     threadInfoStore.registerThread(&env, type, name);
 }
 
-Environment::Scope::~Scope() {
+EnvironmentScope::~EnvironmentScope() {
     assert(id == std::this_thread::get_id());
     threadInfoStore.unregisterThread();
 }
@@ -90,6 +91,12 @@ Environment::Environment(FileSource& fs)
     : id(makeEnvironmentID()), fileSource(fs), loop(uv_loop_new()) {
 }
 
+Environment::~Environment() {
+    assert(abandonedVAOs.empty());
+    assert(abandonedTextures.empty());
+    assert(abandonedBuffers.empty());
+}
+
 Environment& Environment::Get() {
     Environment* env = threadInfoStore.getThreadInfo().env;
     assert(env);
@@ -129,6 +136,50 @@ void Environment::cancelRequest(Request* req) {
     fileSource.cancel(req);
 }
 
+// #############################################################################################
+
+#pragma mark - OpenGL cleanup
+
+void Environment::abandonVAO(uint32_t vao) {
+    assert(currentlyOn(ThreadType::Map));
+    abandonedVAOs.emplace_back(vao);
+}
+
+void Environment::abandonBuffer(uint32_t buffer) {
+    assert(currentlyOn(ThreadType::Map));
+    abandonedBuffers.emplace_back(buffer);
+}
+
+void Environment::abandonTexture(uint32_t texture) {
+    assert(currentlyOn(ThreadType::Map));
+    abandonedTextures.emplace_back(texture);
+}
+
+// Actually remove the objects we marked as abandoned with the above methods.
+void Environment::performCleanup() {
+    assert(currentlyOn(ThreadType::Map));
+
+    if (!abandonedVAOs.empty()) {
+        MBGL_CHECK_ERROR(gl::DeleteVertexArrays(static_cast<GLsizei>(abandonedVAOs.size()),
+                                                abandonedVAOs.data()));
+        abandonedVAOs.clear();
+    }
+
+    if (!abandonedTextures.empty()) {
+        MBGL_CHECK_ERROR(glDeleteTextures(static_cast<GLsizei>(abandonedTextures.size()),
+                                          abandonedTextures.data()));
+        abandonedTextures.clear();
+    }
+
+    if (!abandonedBuffers.empty()) {
+        MBGL_CHECK_ERROR(glDeleteBuffers(static_cast<GLsizei>(abandonedBuffers.size()),
+                                         abandonedBuffers.data()));
+        abandonedBuffers.clear();
+    }
+}
+
+// #############################################################################################
+
 void Environment::terminate() {
     fileSource.abort(*this);
 }
diff --git a/src/mbgl/map/environment.hpp b/src/mbgl/map/environment.hpp
index b631abf13..1cd33a5e6 100644
--- a/src/mbgl/map/environment.hpp
+++ b/src/mbgl/map/environment.hpp
@@ -6,6 +6,7 @@
 
 #include <thread>
 #include <functional>
+#include <vector>
 
 typedef struct uv_loop_s uv_loop_t;
 
@@ -25,16 +26,8 @@ enum class ThreadType : uint8_t {
 
 class Environment final : private util::noncopyable {
 public:
-    class Scope final {
-    public:
-        Scope(Environment&, ThreadType, const std::string& name);
-        ~Scope();
-
-    private:
-        std::thread::id id;
-    };
-
     Environment(FileSource&);
+    ~Environment();
 
     static Environment& Get();
     static bool inScope();
@@ -42,10 +35,27 @@ public:
     static std::string threadName();
 
     unsigned getID() const;
+
+    // #############################################################################################
+
+    // File request APIs
     void requestAsync(const Resource&, std::function<void(const Response&)>);
     Request* request(const Resource&, std::function<void(const Response&)>);
     void cancelRequest(Request*);
 
+    // #############################################################################################
+
+    // Mark OpenGL objects for deletion
+    void abandonVAO(uint32_t vao);
+    void abandonBuffer(uint32_t buffer);
+    void abandonTexture(uint32_t texture);
+
+    // Actually remove the objects we marked as abandoned with the above methods.
+    // Only call this while the OpenGL context is exclusive to this thread.
+    void performCleanup();
+
+    // #############################################################################################
+
     // Request to terminate the environment.
     void terminate();
 
@@ -53,10 +63,24 @@ private:
     unsigned id;
     FileSource& fileSource;
 
+    // Stores OpenGL objects that we marked for deletion
+    std::vector<uint32_t> abandonedVAOs;
+    std::vector<uint32_t> abandonedBuffers;
+    std::vector<uint32_t> abandonedTextures;
+
 public:
     uv_loop_t* const loop;
 };
 
+class EnvironmentScope final {
+public:
+    EnvironmentScope(Environment&, ThreadType, const std::string& name);
+    ~EnvironmentScope();
+
+private:
+    std::thread::id id;
+};
+
 }
 
 #endif
diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp
index 22e6cbceb..ae1cfd20f 100644
--- a/src/mbgl/map/map.cpp
+++ b/src/mbgl/map/map.cpp
@@ -61,6 +61,7 @@ using namespace mbgl;
 
 Map::Map(View& view_, FileSource& fileSource_)
     : env(util::make_unique<Environment>(fileSource_)),
+      scope(util::make_unique<EnvironmentScope>(*env, ThreadType::Main, "Main")),
       view(view_),
       transform(view_),
       fileSource(fileSource_),
@@ -80,15 +81,28 @@ Map::~Map() {
         stop();
     }
 
+    // Extend the scope to include both Main and Map thread types to ease cleanup.
+    scope.reset();
+    scope = util::make_unique<EnvironmentScope>(
+        *env, static_cast<ThreadType>(static_cast<uint8_t>(ThreadType::Main) |
+                                      static_cast<uint8_t>(ThreadType::Map)),
+        "MapandMain");
+
     // Explicitly reset all pointers.
     activeSources.clear();
     sprite.reset();
     glyphStore.reset();
     style.reset();
-    texturePool.reset();
     workers.reset();
+    painter.reset();
+    annotationManager.reset();
+    lineAtlas.reset();
+    spriteAtlas.reset();
+    glyphAtlas.reset();
 
     uv_run(env->loop, UV_RUN_DEFAULT);
+
+    env->performCleanup();
 }
 
 uv::worker &Map::getWorker() {
@@ -113,7 +127,6 @@ void Map::start(bool startPaused) {
 
         // Remove all of these to make sure they are destructed in the correct thread.
         style.reset();
-        workers.reset();
         activeSources.clear();
 
         terminating = true;
@@ -233,7 +246,7 @@ void Map::run() {
         threadName += "andMain";
     }
 
-    Environment::Scope scope(*env, threadType, threadName);
+    EnvironmentScope mapScope(*env, threadType, threadName);
 
     if (mode == Mode::Continuous) {
         checkForPause();
@@ -749,6 +762,10 @@ void Map::prepare() {
 
 void Map::render() {
     assert(Environment::currentlyOn(ThreadType::Map));
+
+    // Cleanup OpenGL objects that we abandoned since the last render call.
+    env->performCleanup();
+
     assert(painter);
     painter->render(*style, activeSources,
                     state, animationTime);
diff --git a/src/mbgl/map/tile_data.cpp b/src/mbgl/map/tile_data.cpp
index aed182671..1832bd8f0 100644
--- a/src/mbgl/map/tile_data.cpp
+++ b/src/mbgl/map/tile_data.cpp
@@ -97,7 +97,7 @@ void TileData::reparse(uv::worker& worker, std::function<void()> callback)
     new uv::work<util::ptr<TileData>>(
         worker,
         [this](util::ptr<TileData>& tile) {
-            Environment::Scope scope(env, ThreadType::TileWorker, "TileWorker_" + tile->name);
+            EnvironmentScope scope(env, ThreadType::TileWorker, "TileWorker_" + tile->name);
             tile->parse();
         },
         [callback](util::ptr<TileData>&) {
diff --git a/src/mbgl/util/texture_pool.cpp b/src/mbgl/util/texture_pool.cpp
index 33bca01c0..845796f5d 100644
--- a/src/mbgl/util/texture_pool.cpp
+++ b/src/mbgl/util/texture_pool.cpp
@@ -1,4 +1,5 @@
 #include <mbgl/util/texture_pool.hpp>
+#include <mbgl/map/environment.hpp>
 
 #include <vector>
 
@@ -42,17 +43,9 @@ void TexturePool::removeTextureID(GLuint texture_id) {
 }
 
 void TexturePool::clearTextureIDs() {
-    std::vector<GLuint> ids_to_remove;
-    ids_to_remove.reserve(texture_ids.size());
-
-    for (std::set<GLuint>::iterator id_iterator = texture_ids.begin();
-         id_iterator != texture_ids.end(); ++id_iterator) {
-        ids_to_remove.push_back(*id_iterator);
+    auto& env = Environment::Get();
+    for (auto texture : texture_ids) {
+        env.abandonTexture(texture);
     }
-
-    if (!ids_to_remove.empty()) {
-        MBGL_CHECK_ERROR(glDeleteTextures((GLsizei)ids_to_remove.size(), &ids_to_remove[0]));
-    }
-
     texture_ids.clear();
 }