Merge pull request #2223 from tangrams/android-nativemap-lock

Synchronize access to shared NativeMap

core/src/map.cpp

@@ -37,9 +37,9 @@ namespace Tangram {
 struct CameraEase {
     struct {
         glm::dvec2 pos;
-        float zoom;
-        float rotation;
-        float tilt;
+        float zoom = 0;
+        float rotation = 0;
+        float tilt = 0;
     } start, end;
 };
 
@@ -58,16 +58,14 @@ class Map::Impl {
     explicit Impl(Platform& _platform) :
         platform(_platform),
         inputHandler(view),
-        scene(std::make_shared<Scene>(_platform)) {}
+        scene(std::make_unique<Scene>(_platform)) {}
 
     void setPixelScale(float _pixelsPerPoint);
     SceneID loadScene(SceneOptions&& _sceneOptions);
     SceneID loadSceneAsync(SceneOptions&& _sceneOptions);
     void syncClientTileSources(bool _firstUpdate);
     bool updateCameraEase(float _dt);
 
-    std::mutex sceneMutex;
-
     Platform& platform;
     RenderState renderState;
     JobQueue jobQueue;
@@ -78,8 +76,7 @@ class Map::Impl {
 
     std::unique_ptr<Ease> ease;
 
-    std::shared_ptr<Scene> scene;
-    std::condition_variable blockUntilSceneReady;
+    std::unique_ptr<Scene> scene;
 
     std::unique_ptr<FrameBuffer> selectionBuffer = std::make_unique<FrameBuffer>(0, 0);
 
@@ -105,7 +102,7 @@ static std::bitset<9> g_flags = 0;
 
 Map::Map(std::unique_ptr<Platform> _platform) : platform(std::move(_platform)) {
     LOGTOInit();
-    impl.reset(new Impl(*platform));
+    impl = std::make_unique<Impl>(*platform);
 }
 
 Map::~Map() {
@@ -116,11 +113,10 @@ Map::~Map() {
     // In any case after shutdown Platform may not call back into Map!
     platform->shutdown();
 
-    // The unique_ptr to Impl will be automatically destroyed when Map is destroyed.
+    // Impl will be automatically destroyed by unique_ptr, but threads owned by AsyncWorker and
+    // Scene need to be destroyed before JobQueue stops.
     impl->asyncWorker.reset();
-
     impl->scene.reset();
-    impl->blockUntilSceneReady.notify_all();
 
     // Make sure other threads are stopped before calling stop()!
     // All jobs will be executed immediately on add() afterwards.
@@ -142,7 +138,7 @@ SceneID Map::loadScene(SceneOptions&& _sceneOptions, bool _async) {
 SceneID Map::Impl::loadScene(SceneOptions&& _sceneOptions) {
 
     // NB: This also disposes old scene which might be blocking
-    scene = std::make_shared<Scene>(platform, std::move(_sceneOptions));
+    scene = std::make_unique<Scene>(platform, std::move(_sceneOptions));
 
     scene->load();
 
@@ -155,8 +151,10 @@ SceneID Map::Impl::loadScene(SceneOptions&& _sceneOptions) {
 
 SceneID Map::Impl::loadSceneAsync(SceneOptions&& _sceneOptions) {
 
-    // Avoid to keep loading old scene and tiles
-    auto oldScene = std::move(scene);
+    // Move the previous scene into a shared_ptr so that it can be captured in a std::function
+    // (unique_ptr can't be captured because std::function is copyable).
+    std::shared_ptr<Scene> oldScene = std::move(scene);
+
     oldScene->cancelTasks();
 
     // Add callback for tile prefetching
@@ -169,9 +167,12 @@ SceneID Map::Impl::loadSceneAsync(SceneOptions&& _sceneOptions) {
         platform.requestRender();
     };
 
-    scene = std::make_shared<Scene>(platform, std::move(_sceneOptions), prefetchCallback);
+    scene = std::make_unique<Scene>(platform, std::move(_sceneOptions), prefetchCallback);
 
-    asyncWorker->enqueue([this, newScene = scene]() {
+    // This async task gets a raw pointer to the new scene and the following task takes ownership of the shared_ptr to
+    // the old scene. Tasks in the async queue are executed one at a time in FIFO order, so even if another scene starts
+    // to load before this loading task finishes, the current scene won't be freed until after this task finishes.
+    asyncWorker->enqueue([this, newScene = scene.get()]() {
         newScene->load();
 
         if (onSceneReady) {
@@ -181,9 +182,11 @@ SceneID Map::Impl::loadSceneAsync(SceneOptions&& _sceneOptions) {
         platform.requestRender();
     });
 
-    // Disposing TileWorker is blocking: Do this async just in case
-    asyncWorker->enqueue([s = std::move(oldScene)]() mutable {
-        LOG("ASYNC DISPOSE OF OLD SCENE - %d", s.use_count() == 1);
+    asyncWorker->enqueue([disposingScene = std::move(oldScene)]() mutable {
+        if (disposingScene.use_count() != 1) {
+            LOGE("Incorrect use count for old scene pointer: %d. Scene may be leaked!", disposingScene.use_count());
+        }
+        disposingScene.reset();
     });
 
     return scene->id;

platforms/android/tangram/src/main/cpp/NativeMap.cpp

@@ -11,7 +11,7 @@ AndroidMap* androidMapFromJava(JNIEnv* env, jobject nativeMapObject) {
     static jclass nativeMapClass = env->FindClass("com/mapzen/tangram/NativeMap");
     static jfieldID nativePointerFID = env->GetFieldID(nativeMapClass, "nativePointer", "J");
     jlong nativePointer = env->GetLongField(nativeMapObject, nativePointerFID);
-    assert(nativePointer > 0);
+    assert(nativePointer != 0);
     return reinterpret_cast<AndroidMap*>(nativePointer);
 }
 
@@ -117,15 +117,11 @@ void NATIVE_METHOD(resize)(JNIEnv* env, jobject obj, jint width, jint height) {
     map->resize(width, height);
 }
 
-jint NATIVE_METHOD(update)(JNIEnv* env, jobject obj, jfloat dt) {
+jint NATIVE_METHOD(render)(JNIEnv* env, jobject obj, jfloat dt) {
     auto* map = androidMapFromJava(env, obj);
     auto result = map->update(dt);
-    return static_cast<jint>(result.flags);
-}
-
-void NATIVE_METHOD(render)(JNIEnv* env, jobject obj) {
-    auto* map = androidMapFromJava(env, obj);
     map->render();
+    return static_cast<jint>(result.flags);
 }
 
 void NATIVE_METHOD(captureSnapshot)(JNIEnv* env, jobject obj, jintArray buffer) {
@@ -505,6 +501,8 @@ jlong NATIVE_METHOD(addClientDataSource)(JNIEnv* env, jobject obj,
 void NATIVE_METHOD(removeClientDataSource)(JNIEnv* env, jobject obj, jlong sourcePtr) {
     auto* map = androidMapFromJava(env, obj);
     auto* clientDataSource = reinterpret_cast<ClientDataSource*>(sourcePtr);
+    // Map holds the only reference of the shared_ptr to the ClientDataSource, so removing the
+    // reference in Map will also implicitly free the ClientDataSource.
     map->removeTileSource(*clientDataSource);
 }
 

platforms/android/tangram/src/main/java/com/mapzen/tangram/MapController.java

@@ -204,19 +204,32 @@ void UIThreadInit(@NonNull final GLViewHolder viewHolder, @Nullable final HttpHa
      * Responsible to dispose internals of MapController during Map teardown.
      * If client code extends MapController and overrides this method, then it must call super.dispose()
      */
-    protected synchronized void dispose() {
-
+    protected void dispose() {
         Log.v(BuildConfig.TAG, "Start MapController dispose");
         nativeMap.shutdown();
         Log.v(BuildConfig.TAG, "Remaining HTTP requests: " + httpRequestHandles.size());
 
         Log.v(BuildConfig.TAG, "MapData instances to remove: " + clientTileSources.size());
         for (MapData mapData : clientTileSources.values()) {
-            mapData.remove();
+            nativeMap.removeClientDataSource(mapData.pointer);
+            mapData.dispose();
         }
         clientTileSources.clear();
         markers.clear();
 
+        // NativeMap dispose makes GL calls to free GPU resources so we _should_ call it on the
+        // render thread, where the GL context is active. However, it is possible for the render
+        // thread associated with GLSurfaceView to be stopped without running all of the queued
+        // events. This would cause the native memory to be leaked.
+        //
+        // To avoid this, we dispose the NativeMap on the UI thread. The GL calls produce an
+        // error in logcat, but otherwise have no effect. The GPU resources are eventually freed
+        // when the SurfaceView is destroyed, so nothing is leaked.
+        Log.v(BuildConfig.TAG, "Dispose NativeMap");
+        NativeMap disposingNativeMap = nativeMap;
+        nativeMap = null;
+        disposingNativeMap.dispose();
+
         // Dispose all listener and callbacks associated with mapController
         // This will help prevent leaks of references from the client code, possibly used in these
         // listener/callbacks
@@ -228,21 +241,7 @@ protected synchronized void dispose() {
         markerPickListener = null;
         cameraAnimationCallback = null;
 
-        // Prevent any calls to native functions - except dispose.
-        final NativeMap disposingNativeMap = nativeMap;
-        nativeMap = null;
-
-        // NOTE: It is possible for the MapView held by a ViewGroup to be removed, calling detachFromWindow which
-        // stops the Render Thread associated with GLSurfaceView, possibly resulting in leaks from render thread
-        // queue. Because of the above, destruction lifecycle of the GLSurfaceView will not be triggered.
-        // To avoid the above scenario, nativeDispose is called from the UIThread.
-        //
-        // Since all gl resources will be freed when GLSurfaceView is deleted this is safe until
-        // we support sharing gl contexts.
-        Log.v(BuildConfig.TAG, "Dispose NativeMap");
-        disposingNativeMap.dispose();
         Log.v(BuildConfig.TAG, "Finish MapController dispose");
-
     }
 
     /**
@@ -685,6 +684,7 @@ void removeDataLayer(@NonNull final MapData mapData) {
             throw new RuntimeException("Tried to remove a MapData that was already disposed");
         }
         nativeMap.removeClientDataSource(mapData.pointer);
+        mapData.dispose();
     }
 
     /**

platforms/android/tangram/src/main/java/com/mapzen/tangram/MapData.java

@@ -76,7 +76,9 @@ public void remove() {
             return;
         }
         map.removeDataLayer(this);
+    }
 
+    void dispose() {
         mapController = null;
         pointer = 0;
     }

platforms/android/tangram/src/main/java/com/mapzen/tangram/MapRenderer.java

@@ -10,12 +10,13 @@
 
 import androidx.annotation.NonNull;
 
+import com.mapzen.tangram.viewholder.GLViewHolder;
+
 class MapRenderer implements GLSurfaceView.Renderer {
 
     MapRenderer(MapController mapController, Handler uiThreadHandler) {
         this.uiThreadHandler = uiThreadHandler;
         this.map = mapController;
-        this.nativeMap = mapController.nativeMap;
     }
 
     // GLSurfaceView.Renderer methods
@@ -34,19 +35,18 @@ public void onDrawFrame(final GL10 gl) {
         final float delta = (newTime - time) / 1000000000.0f;
         time = newTime;
 
-        boolean mapViewComplete;
-        boolean isCameraEasing;
-        boolean isAnimating;
+        if (map.nativeMap == null)
+        {
+            // Stop here if nativeMap is null. This can happen during Activity/Fragment teardown
+            // when MapController has been disposed but the View hasn't been destroyed yet.
+            return;
+        }
 
-        synchronized(map) {
-            int state = nativeMap.update(delta);
+        int state = map.nativeMap.render(delta);
 
-            nativeMap.render();
-
-            mapViewComplete = (state == VIEW_COMPLETE);
-            isCameraEasing = (state & VIEW_CHANGING) != 0;
-            isAnimating = (state & VIEW_ANIMATING) != 0;
-        }
+        boolean mapViewComplete = (state == VIEW_COMPLETE);
+        boolean isCameraEasing = (state & VIEW_CHANGING) != 0;
+        boolean isAnimating = (state & VIEW_ANIMATING) != 0;
 
         if (isCameraEasing) {
             if (!isPrevCameraEasing) {
@@ -87,16 +87,12 @@ public void run() {
 
     @Override
     public void onSurfaceChanged(final GL10 gl, final int width, final int height) {
-        synchronized (map) {
-            nativeMap.resize(width, height);
-        }
+        map.nativeMap.resize(width, height);
     }
 
     @Override
     public void onSurfaceCreated(final GL10 gl, final EGLConfig config) {
-        synchronized (map) {
-            nativeMap.setupGL();
-        }
+        map.nativeMap.setupGL();
     }
 
     void captureFrame(MapController.FrameCaptureCallback callback, boolean waitForCompleteView) {
@@ -106,17 +102,21 @@ void captureFrame(MapController.FrameCaptureCallback callback, boolean waitForCo
 
     @NonNull
     private Bitmap capture() {
-        View view = map.getGLViewHolder().getView();
+        GLViewHolder viewHolder = map.getGLViewHolder();
+        if (viewHolder == null) {
+            throw new IllegalStateException("MapController GLViewHolder is null");
+        }
+
+        View view = viewHolder.getView();
 
         final int w = view.getWidth();
         final int h = view.getHeight();
 
         final int[] b = new int[w * h];
         final int[] bt = new int[w * h];
 
-        synchronized (map) {
-            nativeMap.captureSnapshot(b);
-        }
+        map.nativeMap.captureSnapshot(b);
+
         for (int i = 0; i < h; i++) {
             for (int j = 0; j < w; j++) {
                 final int pix = b[i * w + j];
@@ -132,7 +132,6 @@ private Bitmap capture() {
 
     private final Handler uiThreadHandler;
     private final MapController map;
-    private final NativeMap nativeMap;
     private long time = System.nanoTime();
     private boolean isPrevCameraEasing = false;
     private boolean isPrevMapViewComplete = false;