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

@@ -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;

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;
         }
     }

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) {

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;
 }

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);
     }
 }
 

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);
 }

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

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);

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>&) {

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();
 }