Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Harden AndroidRendererFrontend::reduceMemoryUse #14622

Merged
merged 2 commits into from
May 10, 2019
Merged

Harden AndroidRendererFrontend::reduceMemoryUse #14622

merged 2 commits into from
May 10, 2019

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented May 9, 2019

Only allow invoking OnLowMemory with an active renderer.
Closes #14603

@tobrun tobrun added the Android Mapbox Maps SDK for Android label May 9, 2019
@tobrun tobrun added this to the release-nectar milestone May 9, 2019
@tobrun tobrun requested a review from LukasPaczos May 9, 2019 11:20
@tobrun tobrun self-assigned this May 9, 2019
@LukasPaczos
Copy link
Contributor

I'm not sure this check is enough. Check out this diff, which is still going to crash when we launch the map:

diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapView.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapView.java
index b04b1f827..fde151882 100644
--- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapView.java
+++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapView.java
@@ -295,7 +295,7 @@ public class MapView extends FrameLayout implements NativeMapView.ViewCallback {
     } else {
       GLSurfaceView glSurfaceView = new GLSurfaceView(getContext());
       glSurfaceView.setZOrderMediaOverlay(mapboxMapOptions.getRenderSurfaceOnTop());
-      mapRenderer = new GLSurfaceViewMapRenderer(getContext(), glSurfaceView, localFontFamily) {
+      mapRenderer = new GLSurfaceViewMapRenderer(this, getContext(), glSurfaceView, localFontFamily) {
         @Override
         public void onSurfaceCreated(GL10 gl, EGLConfig config) {
           MapView.this.onSurfaceCreated();
diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/renderer/glsurfaceview/GLSurfaceViewMapRenderer.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/renderer/glsurfaceview/GLSurfaceViewMapRenderer.java
index aa36dace7..85b01589e 100644
--- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/renderer/glsurfaceview/GLSurfaceViewMapRenderer.java
+++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/renderer/glsurfaceview/GLSurfaceViewMapRenderer.java
@@ -4,6 +4,8 @@ import android.content.Context;
 import android.opengl.GLSurfaceView;
 import android.support.annotation.NonNull;
 import android.view.SurfaceHolder;
+
+import com.mapbox.mapboxsdk.maps.MapView;
 import com.mapbox.mapboxsdk.maps.renderer.MapRenderer;
 import com.mapbox.mapboxsdk.maps.renderer.egl.EGLConfigChooser;
 
@@ -23,7 +25,7 @@ public class GLSurfaceViewMapRenderer extends MapRenderer implements GLSurfaceVi
   @NonNull
   private final GLSurfaceView glSurfaceView;
 
-  public GLSurfaceViewMapRenderer(Context context,
+  public GLSurfaceViewMapRenderer(final MapView mapView, Context context,
                                   GLSurfaceView glSurfaceView,
                                   String localIdeographFontFamily) {
     super(context, localIdeographFontFamily);
@@ -39,6 +41,7 @@ public class GLSurfaceViewMapRenderer extends MapRenderer implements GLSurfaceVi
       public void surfaceCreated(SurfaceHolder holder) {
         super.surfaceCreated(holder);
         hasSurface = true;
+        mapView.onLowMemory();
       }
 
       @Override

The pointer is initialized on GL thread in MapRenderer#onSurfaceCreated and from quick testing, it seems like SurfaceHolderCallbackAdapter#surfaceCreated that sets the flag is called consistently sooner than MapRenderer#onSurfaceCreated.

@LukasPaczos
Copy link
Contributor

Can that also mean that we are misusing the MapRenderer#hasSurface flag? It's not thread safe and we are accessing it from multiple threads in GLSurfaceViewMapRenderer#requestRender.

…reated by validating if mapboxMap isn't null when invoking MapView#onLowMemory
@tobrun
Copy link
Member Author

tobrun commented May 10, 2019

1c8f2c9

re. threadsafity, replaced hasSurface with AtomicBoolean to ensure thread safety. lmk if you feel another construct would be more appropriate here.

re. not sure this check is enough, yes you are correct we need to make sure that the renderer is actually created and that only happens after setting the hasSurface call and on the main thread. Adding an additional check to MapView#onLowMemory fixes that, MapboxMap will always be created after the renderer.

@tobrun tobrun merged commit 1b8eb9a into master May 10, 2019
@tobrun tobrun deleted the tvn-onlowmem branch May 10, 2019 12:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling onLowMemory before initialization
2 participants