Remove Map::start/stop

Both Android and iOS now use pause/resume. The map thread is
always running, and be able to receive messages. When paused,
it will just refuse to render.

android/cpp/jni.cpp

@@ -256,20 +256,6 @@ void JNICALL nativeDestroySurface(JNIEnv *env, jobject obj, jlong nativeMapViewP
     nativeMapView->destroySurface();
 }
 
-void JNICALL nativeStart(JNIEnv *env, jobject obj, jlong nativeMapViewPtr) {
-    mbgl::Log::Debug(mbgl::Event::JNI, "nativeStart");
-    assert(nativeMapViewPtr != 0);
-    NativeMapView *nativeMapView = reinterpret_cast<NativeMapView *>(nativeMapViewPtr);
-    nativeMapView->start();
-}
-
-void JNICALL nativeStop(JNIEnv *env, jobject obj, jlong nativeMapViewPtr) {
-    mbgl::Log::Debug(mbgl::Event::JNI, "nativeStop");
-    assert(nativeMapViewPtr != 0);
-    NativeMapView *nativeMapView = reinterpret_cast<NativeMapView *>(nativeMapViewPtr);
-    nativeMapView->stop();
-}
-
 void JNICALL nativePause(JNIEnv *env, jobject obj, jlong nativeMapViewPtr) {
     mbgl::Log::Debug(mbgl::Event::JNI, "nativePause");
     assert(nativeMapViewPtr != 0);
@@ -932,8 +918,6 @@ extern "C" JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM *vm, void *reserved) {
         {"nativeCreateSurface", "(JLandroid/view/Surface;)V",
          reinterpret_cast<void *>(&nativeCreateSurface)},
         {"nativeDestroySurface", "(J)V", reinterpret_cast<void *>(&nativeDestroySurface)},
-        {"nativeStart", "(J)V", reinterpret_cast<void *>(&nativeStart)},
-        {"nativeStop", "(J)V", reinterpret_cast<void *>(&nativeStop)},
         {"nativePause", "(J)V", reinterpret_cast<void *>(&nativePause)},
         {"nativeResume", "(J)V", reinterpret_cast<void *>(&nativeResume)},
         {"nativeUpdate", "(J)V", reinterpret_cast<void *>(&nativeUpdate)},

android/cpp/native_map_view.cpp

@@ -60,7 +60,7 @@ NativeMapView::NativeMapView(JNIEnv *env, jobject obj_)
     : mbgl::View(*this),
       fileCache(mbgl::android::cachePath + "/mbgl-cache.db"),
       fileSource(&fileCache),
-      map(*this, fileSource) {
+      map(*this, fileSource, MapMode::Continuous, true) {
     mbgl::Log::Debug(mbgl::Event::Android, "NativeMapView::NativeMapView");
 
     assert(env != nullptr);
@@ -577,23 +577,6 @@ EGLConfig NativeMapView::chooseConfig(const EGLConfig configs[], EGLint numConfi
     return configId;
 }
 
-void NativeMapView::start() {
-    mbgl::Log::Debug(mbgl::Event::Android, "NativeMapView::start");
-
-    if (display == EGL_NO_DISPLAY) {
-        initializeDisplay();
-    }
-
-    if (context == EGL_NO_CONTEXT) {
-        initializeContext();
-    }
-
-    assert(display != EGL_NO_DISPLAY);
-    assert(context != EGL_NO_CONTEXT);
-
-    map.start(true);
-}
-
 void loadExtensions() {
     const GLubyte *str = glGetString(GL_EXTENSIONS);
     if (str == nullptr) {
@@ -690,14 +673,6 @@ void loadExtensions() {
     }
 }
 
-void NativeMapView::stop() {
-    mbgl::Log::Debug(mbgl::Event::Android, "NativeMapView::stop");
-
-    if ((display != EGL_NO_DISPLAY) && (context != EGL_NO_CONTEXT)) {
-        map.stop();
-    }
-}
-
 void NativeMapView::pause(bool waitForPause) {
     mbgl::Log::Debug(mbgl::Event::Android, "NativeMapView::pause %s",
                      (waitForPause) ? "true" : "false");

android/java/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxgl/views/MapView.java

@@ -419,7 +419,6 @@ public class MapView extends SurfaceView {
 
         mNativeMapView.initializeDisplay();
         mNativeMapView.initializeContext();
-        mNativeMapView.start();
     }
 
     // Called when we need to save instance state
@@ -441,7 +440,6 @@ public class MapView extends SurfaceView {
     // Called when we need to clean up
     // Must be called from Activity onDestroy
     public void onDestroy() {
-        mNativeMapView.stop();
         mNativeMapView.terminateContext();
         mNativeMapView.terminateDisplay();
     }

android/java/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxgl/views/NativeMapView.java

@@ -73,14 +73,6 @@ class NativeMapView {
         nativeDestroySurface(mNativeMapViewPtr);
     }
 
-    public void start() {
-        nativeStart(mNativeMapViewPtr);
-    }
-
-    public void stop() {
-        nativeStop(mNativeMapViewPtr);
-    }
-
     public void pause() {
         nativePause(mNativeMapViewPtr);
     }
@@ -386,10 +378,6 @@ class NativeMapView {
 
     private native void nativeDestroySurface(long nativeMapViewPtr);
 
-    private native void nativeStart(long nativeMapViewPtr);
-
-    private native void nativeStop(long nativeMapViewPtr);
-
     private native void nativePause(long nativeMapViewPtr);
 
     private native void nativeResume(long nativeMapViewPtr);

include/mbgl/android/native_map_view.hpp

@@ -39,9 +39,6 @@ public:
     void createSurface(ANativeWindow *window);
     void destroySurface();
 
-    void start();
-    void stop();
-
     void resume();
     void pause(bool waitForPause = false);
 

include/mbgl/map/map.hpp

@@ -35,19 +35,11 @@ class Map : private util::noncopyable {
     friend class View;
 
 public:
-    explicit Map(View&, FileSource&);
+    explicit Map(View&, FileSource&,
+                 MapMode mode = MapMode::Continuous,
+                 bool startPaused = false);
     ~Map();
 
-    // Start the map render thread. It is asynchronous.
-    void start(bool startPaused = false, MapMode mode = MapMode::Continuous);
-    inline void start(MapMode renderMode) { start(false, renderMode); }
-
-    // Stop the map render thread. This call will block until the map rendering thread stopped.
-    // The optional callback function will be invoked repeatedly until the map thread is stopped.
-    // The callback function should wait until it is woken up again by view.notify(), otherwise
-    // this will be a busy waiting loop.
-    void stop(std::function<void ()> callback = std::function<void ()>());
-
     // Pauses the render thread. The render thread will stop running but will not be terminated and will not lose state until resumed.
     void pause(bool waitForPause = false);
 
@@ -149,7 +141,6 @@ private:
 
     const std::unique_ptr<Environment> env;
     std::unique_ptr<EnvironmentScope> scope;
-    View &view;
     const std::unique_ptr<MapData> data;
     std::unique_ptr<util::Thread<MapContext>> context;
 };

include/mbgl/map/mode.hpp

@@ -6,7 +6,6 @@
 namespace mbgl {
 
 enum class MapMode : uint8_t {
-    None, // we're not doing any processing
     Continuous, // continually updating map
     Still, // a once-off still image
 };

include/mbgl/platform/default/glfw_view.hpp

@@ -25,7 +25,7 @@ public:
     static void onMouseClick(GLFWwindow *window, int button, int action, int modifiers);
     static void onMouseMove(GLFWwindow *window, double x, double y);
 
-    int run();
+    void run();
     void fps();
 
 public:

linux/main.cpp

@@ -90,7 +90,7 @@ int main(int argc, char *argv[]) {
 
     map.setStyleURL(style);
 
-    int ret = view->run();
+    view->run();
 
     // Save settings
     mbgl::LatLng latLng = map.getLatLng();
@@ -101,5 +101,5 @@ int main(int argc, char *argv[]) {
     settings.debug = map.getDebug();
     settings.save();
 
-    return ret;
+    return 0;
 }

macosx/main.mm

@@ -133,7 +133,7 @@ int main() {
     // Load style
     map.setStyleURL("asset://styles/bright-v7.json");
 
-    int ret = view.run();
+    view.run();
 
     [reachability stopNotifier];
 
@@ -146,5 +146,5 @@ int main() {
     settings.debug = map.getDebug();
     settings.save();
 
-    return ret;
+    return 0;
 }

platform/default/glfw_view.cpp

@@ -6,24 +6,15 @@
 
 pthread_once_t loadGLExtensions = PTHREAD_ONCE_INIT;
 
-GLFWView::GLFWView(bool fullscreen_) : fullscreen(fullscreen_) {
-#ifdef NVIDIA
-    glDiscardFramebufferEXT = reinterpret_cast<PFNGLDISCARDFRAMEBUFFEREXTPROC>(glfwGetProcAddress("glDiscardFramebufferEXT"));
-#endif
-}
-
-GLFWView::~GLFWView() {
-    glfwDestroyWindow(window);
-    glfwTerminate();
-}
-
 void glfwError(int error, const char *description) {
     mbgl::Log::Error(mbgl::Event::OpenGL, "GLFW error (%i): %s", error, description);
     assert(false);
 }
 
-void GLFWView::initialize(mbgl::Map *map_) {
-    View::initialize(map_);
+GLFWView::GLFWView(bool fullscreen_) : fullscreen(fullscreen_) {
+#ifdef NVIDIA
+    glDiscardFramebufferEXT = reinterpret_cast<PFNGLDISCARDFRAMEBUFFEREXTPROC>(glfwGetProcAddress("glDiscardFramebufferEXT"));
+#endif
 
     glfwSetErrorCallback(glfwError);
 
@@ -65,10 +56,6 @@ void GLFWView::initialize(mbgl::Map *map_) {
     glfwMakeContextCurrent(window);
     glfwSwapInterval(1);
 
-    int width, height;
-    glfwGetWindowSize(window, &width, &height);
-    onResize(window, width, height);
-
     glfwSetCursorPosCallback(window, onMouseMove);
     glfwSetMouseButtonCallback(window, onMouseClick);
     glfwSetWindowSizeCallback(window, onResize);
@@ -162,6 +149,19 @@ void GLFWView::initialize(mbgl::Map *map_) {
     glfwMakeContextCurrent(nullptr);
 }
 
+GLFWView::~GLFWView() {
+    glfwDestroyWindow(window);
+    glfwTerminate();
+}
+
+void GLFWView::initialize(mbgl::Map *map_) {
+    View::initialize(map_);
+
+    int width, height;
+    glfwGetWindowSize(window, &width, &height);
+    onResize(window, width, height);
+}
+
 void GLFWView::onKey(GLFWwindow *window, int key, int /*scancode*/, int action, int mods) {
     GLFWView *view = reinterpret_cast<GLFWView *>(glfwGetWindowUserPointer(window));
 
@@ -266,18 +266,10 @@ void GLFWView::onMouseMove(GLFWwindow *window, double x, double y) {
     view->lastY = y;
 }
 
-int GLFWView::run() {
-    map->start();
-
+void GLFWView::run() {
     while (!glfwWindowShouldClose(window)) {
         glfwWaitEvents();
     }
-
-    map->stop([]() {
-        glfwWaitEvents();
-    });
-
-    return 0;
 }
 
 void GLFWView::activate() {

platform/ios/MGLMapView.mm

@@ -277,7 +277,9 @@ mbgl::DefaultFileSource *mbglFileSource = nullptr;
     mbglView = new MBGLView(self);
     mbglFileCache  = new mbgl::SQLiteCache(defaultCacheDatabase());
     mbglFileSource = new mbgl::DefaultFileSource(mbglFileCache);
-    mbglMap = new mbgl::Map(*mbglView, *mbglFileSource);
+
+    // Start paused on the IB canvas
+    mbglMap = new mbgl::Map(*mbglView, *mbglFileSource, mbgl::MapMode::Continuous, _isTargetingInterfaceBuilder);
     mbglMap->resize(self.bounds.size.width, self.bounds.size.height, _glView.contentScaleFactor);
 
     // Notify map object when network reachability status changes.
@@ -380,12 +382,6 @@ mbgl::DefaultFileSource *mbglFileSource = nullptr;
     _regionChangeDelegateQueue = [NSOperationQueue new];
     _regionChangeDelegateQueue.maxConcurrentOperationCount = 1;
 
-    // start the main loop, but not on the IB canvas
-    if ( ! _isTargetingInterfaceBuilder)
-    {
-        mbglMap->start();
-    }
-
     // metrics: map load event
     const mbgl::LatLng latLng = mbglMap->getLatLng();
     const double zoom = mbglMap->getZoom();
@@ -669,7 +665,7 @@ mbgl::DefaultFileSource *mbglFileSource = nullptr;
     self.glSnapshotView.image = self.glView.snapshot;
     self.glSnapshotView.hidden = NO;
 
-    mbglMap->stop();
+    mbglMap->pause();
 
     [self.glView deleteDrawable];
 }
@@ -680,7 +676,7 @@ mbgl::DefaultFileSource *mbglFileSource = nullptr;
 
     [self.glView bindDrawable];
 
-    mbglMap->start();
+    mbglMap->resume();
 }
 
 - (void)tintColorDidChange

src/mbgl/map/map.cpp

@@ -33,60 +33,22 @@
 
 namespace mbgl {
 
-Map::Map(View& view_, FileSource& fileSource_)
-    : env(util::make_unique<Environment>(fileSource_)),
+Map::Map(View& view, FileSource& fileSource, MapMode mode, bool startPaused)
+    : env(util::make_unique<Environment>(fileSource)),
       scope(util::make_unique<EnvironmentScope>(*env, ThreadType::Main, "Main")),
-      view(view_),
-      data(util::make_unique<MapData>(view_))
+      data(util::make_unique<MapData>(view, mode)),
+      context(util::make_unique<util::Thread<MapContext>>("Map", *env, view, *data, startPaused))
 {
     view.initialize(this);
 }
 
 Map::~Map() {
-    if (data->mode != MapMode::None) {
-        stop();
-    }
-}
-
-void Map::start(bool startPaused, MapMode renderMode) {
-    assert(Environment::currentlyOn(ThreadType::Main));
-    assert(data->mode == MapMode::None);
-
-    // When starting map rendering in another thread, we perform async/continuously
-    // updated rendering. Only in these cases, we attach the async handlers.
-    data->mode = renderMode;
-
-    context = util::make_unique<util::Thread<MapContext>>("Map", *env, view, *data, startPaused);
-    triggerUpdate();
-}
-
-void Map::stop(std::function<void ()> cb) {
-    assert(Environment::currentlyOn(ThreadType::Main));
-    assert(data->mode != MapMode::None);
-
     resume();
-
-    if (cb) {
-        // Wait until the render thread stopped. We are using this construct instead of plainly
-        // relying on the thread_join because the system might need to run things in the current
-        // thread that is required for the render thread to terminate correctly. This is for example
-        // the case with Cocoa's NSURLRequest. Otherwise, we will eventually deadlock because this
-        // thread (== main thread) is blocked. The callback function should use an efficient waiting
-        // function to avoid a busy waiting loop.
-        context->pumpingStop(cb);
-    }
-
-    // If a callback function was provided, this should return immediately because the thread has
-    // already finished executing.
-    context.reset();
-
-    data->mode = MapMode::None;
 }
 
 void Map::pause(bool waitForPause) {
     assert(Environment::currentlyOn(ThreadType::Main));
     assert(data->mode == MapMode::Continuous);
-    assert(context);
 
     std::unique_lock<std::mutex> lockPause(data->mutexPause);
     context->invoke(&MapContext::pause);
@@ -98,15 +60,12 @@ void Map::pause(bool waitForPause) {
 
 void Map::resume() {
     assert(Environment::currentlyOn(ThreadType::Main));
-    assert(data->mode != MapMode::None);
-    assert(context);
 
     data->condResume.notify_all();
 }
 
 void Map::renderStill(StillImageCallback fn) {
     assert(Environment::currentlyOn(ThreadType::Main));
-    assert(context);
 
     if (data->mode != MapMode::Still) {
         throw util::Exception("Map is not in still image render mode");
@@ -122,20 +81,17 @@ void Map::renderStill(StillImageCallback fn) {
 
 void Map::renderSync() {
     assert(Environment::currentlyOn(ThreadType::Main));
-    assert(context);
 
     context->invokeSync(&MapContext::render);
 }
 
 void Map::renderAsync() {
     assert(Environment::currentlyOn(ThreadType::Main));
-    assert(context);
 
     context->invoke(&MapContext::render);
 }
 
 void Map::update() {
-    assert(context);
     triggerUpdate();
 }
 
@@ -346,7 +302,6 @@ void Map::setDefaultPointAnnotationSymbol(const std::string& symbol) {
 
 double Map::getTopOffsetPixelsForAnnotationSymbol(const std::string& symbol) {
     assert(Environment::currentlyOn(ThreadType::Main));
-    assert(context);
     return context->invokeSync<double>(&MapContext::getTopOffsetPixelsForAnnotationSymbol, symbol);
 }
 
@@ -357,9 +312,7 @@ uint32_t Map::addPointAnnotation(const LatLng& point, const std::string& symbol)
 std::vector<uint32_t> Map::addPointAnnotations(const std::vector<LatLng>& points, const std::vector<std::string>& symbols) {
     assert(Environment::currentlyOn(ThreadType::Main));
     auto result = data->annotationManager.addPointAnnotations(points, symbols, *data);
-    if (context) {
-        context->invoke(&MapContext::updateAnnotationTiles, result.first);
-    }
+    context->invoke(&MapContext::updateAnnotationTiles, result.first);
     return result.second;
 }
 
@@ -371,9 +324,7 @@ void Map::removeAnnotation(uint32_t annotation) {
 void Map::removeAnnotations(const std::vector<uint32_t>& annotations) {
     assert(Environment::currentlyOn(ThreadType::Main));
     auto result = data->annotationManager.removeAnnotations(annotations, *data);
-    if (context) {
-        context->invoke(&MapContext::updateAnnotationTiles, result);
-    }
+    context->invoke(&MapContext::updateAnnotationTiles, result);
 }
 
 std::vector<uint32_t> Map::getAnnotationsInBounds(const LatLngBounds& bounds) {
@@ -441,22 +392,17 @@ Duration Map::getDefaultTransitionDuration() {
 }
 
 void Map::setSourceTileCacheSize(size_t size) {
-    assert(context);
     context->invoke(&MapContext::setSourceTileCacheSize, size);
 }
 
 void Map::onLowMemory() {
-    if (context) {
-        context->invoke(&MapContext::onLowMemory);
-    }
+    context->invoke(&MapContext::onLowMemory);
 }
 
 #pragma mark - Private
 
 void Map::triggerUpdate(Update update_) {
-    if (context) {
-        context->invoke(&MapContext::triggerUpdate, update_);
-    }
+    context->invoke(&MapContext::triggerUpdate, update_);
 }
 
 }

src/mbgl/map/map_context.cpp

@@ -45,7 +45,6 @@ MapContext::MapContext(uv_loop_t* loop, Environment& env_, View& view_, MapData&
       painter(util::make_unique<Painter>(*spriteAtlas, *glyphAtlas, *lineAtlas))
 {
     assert(Environment::currentlyOn(ThreadType::Map));
-    assert(data.mode != MapMode::None);
 
     asyncUpdate->unref();
 

src/mbgl/map/map_data.hpp

@@ -30,7 +30,7 @@ class MapData {
     using Lock = std::lock_guard<std::mutex>;
 
 public:
-    inline MapData(View& view) : transform(view) {
+    inline MapData(View& view, MapMode mode_) : transform(view), mode(mode_) {
         setAnimationTime(TimePoint::min());
         setDefaultTransitionDuration(Duration::zero());
     }
@@ -100,7 +100,7 @@ public:
 public:
     Transform transform;
     AnnotationManager annotationManager;
-    MapMode mode = MapMode::None;
+    const MapMode mode;
 
 private:
     mutable std::mutex mtx;

src/mbgl/util/thread.hpp

@@ -89,7 +89,6 @@ private:
     std::promise<void> joinable;
 
     std::thread thread;
-    std::atomic_bool joined;
 
     Object* object;
     RunLoop* loop;
@@ -97,8 +96,7 @@ private:
 
 template <class Object>
 template <class... Args>
-Thread<Object>::Thread(const std::string& name, Args&&... args)
-    : joined(false) {
+Thread<Object>::Thread(const std::string& name, Args&&... args) {
     // Note: We're using std::tuple<> to store the arguments because GCC 4.9 has a bug
     // when expanding parameters packs captured in lambdas.
     std::tuple<Args...> params = std::forward_as_tuple(::std::forward<Args>(args)...);
@@ -137,7 +135,6 @@ void Thread<Object>::run(P&& params, index_sequence<I...>) {
     loop_.run();
 
     joinable.get_future().get();
-    joined = true;
 }
 
 template <class Object>
@@ -147,15 +144,6 @@ Thread<Object>::~Thread() {
     thread.join();
 }
 
-template <class Object>
-void Thread<Object>::pumpingStop(std::function<void ()> cb) {
-    loop->stop();
-    joinable.set_value();
-    while (!joined) {
-        cb();
-    }
-}
-
 }
 }
 

test/api/repeated_render.cpp

@@ -22,9 +22,7 @@ TEST(API, RepeatedRender) {
 
     Log::setObserver(util::make_unique<FixtureLogObserver>());
 
-    Map map(view, fileSource);
-
-    map.start(MapMode::Still);
+    Map map(view, fileSource, MapMode::Still);
 
     {
         view.resize(128, 512, 1);
@@ -54,8 +52,6 @@ TEST(API, RepeatedRender) {
         util::write_file("test/fixtures/api/2.png", png);
     }
 
-    map.stop();
-
     auto observer = Log::removeObserver();
     auto flo = dynamic_cast<FixtureLogObserver*>(observer.get());
     auto unchecked = flo->unchecked();

test/api/set_style.cpp

@@ -16,13 +16,10 @@ TEST(API, SetStyle) {
 
     Log::setObserver(util::make_unique<FixtureLogObserver>());
 
-    Map map(view, fileSource);
-
-    map.start(MapMode::Still);
-
-    map.setStyleJSON("invalid", "test/suite");
-
-    map.stop();
+    {
+        Map map(view, fileSource, MapMode::Still);
+        map.setStyleJSON("invalid", "test/suite");
+    }
 
     auto observer = Log::removeObserver();
     auto flo = dynamic_cast<FixtureLogObserver*>(observer.get());

test/headless/headless.cpp

@@ -144,11 +144,9 @@ TEST_P(HeadlessTest, render) {
 
         HeadlessView view(display, width, height, pixelRatio);
         DefaultFileSource fileSource(nullptr);
-        Map map(view, fileSource);
+        Map map(view, fileSource, MapMode::Still);
 
         map.resize(width, height, pixelRatio);
-        map.start(MapMode::Still);
-
         map.setClasses(classes);
         map.setStyleJSON(style, "test/suite");
         map.setLatLngZoom(mbgl::LatLng(latitude, longitude), zoom);
@@ -163,8 +161,6 @@ TEST_P(HeadlessTest, render) {
         });
 
         promise.get_future().get();
-
-        map.stop();
     }
 }