Merge pull request #1016 from mapbox/1016-coalesce-opengl-object-deletion

Crash when deleting buffers

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