Fix rare scenario where map render surface size was wrong (#9293)

There was a rare situation where if the surface changed like this:
1. First set it to width: 100, height: 200
2. Resize it to width: 300, height: 400
3. At this point if the render thread was busy the resize was not
processed
4. Immediately set again width: 100, height: 200
5. At this point onSurfaceSizeChanged did not schedule a post to render
thread because the size is still 100,200
6. Render thread was freed and could process the resize in step 2 but
there's no task for step 4

Expected result would be that both resizes in step 2 and 4 should run.

cc @mapbox/maps-android
cc @mapbox/sdk-ci

GitOrigin-RevId: 1a940b42e0b49ece1d86b6e4146532688ae7454c

Author: jush

CHANGELOG.md

@@ -7,7 +7,10 @@ Mapbox welcomes participation and contributions from everyone.
 # main
 
 ## Features ✨ and improvements 🏁
-* Introduce experimental `queryRenderedRasterValues` API for querying the rendered raster array value at a point on the map. 
+* Introduce experimental `queryRenderedRasterValues` API for querying the rendered raster array value at a point on the map.
+
+## Bug fixes 🐞
+* Fix rare scenario where map render surface size was wrong.
 
 # 11.18.0
 

maps-sdk/src/main/java/com/mapbox/maps/renderer/MapboxRenderThread.kt

@@ -69,7 +69,10 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
   internal var surface: Surface? = null
   @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
   internal var eglSurface: EGLSurface
+
+  @RenderThread
   private var width: Int = 0
+  @RenderThread
   private var height: Int = 0
 
   private val widgetRenderer: MapboxWidgetRenderer
@@ -80,7 +83,6 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
   @Volatile
   @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
   internal var awaitingNextVsync = false
-  private var sizeChanged = false
   @Volatile
   @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
   internal var paused = false
@@ -119,9 +121,10 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
    * Render thread should be treated as valid (prepared to render a map) when both flags are true.
    * Getter is thread-safe as this flag could be accessed from any thread.
    */
-  private val renderThreadPrepared get() = renderThreadPreparedLock.withLock {
-    eglContextMadeCurrent && nativeRenderCreated
-  }
+  private val renderThreadPrepared
+    get() = renderThreadPreparedLock.withLock {
+      eglContextMadeCurrent && nativeRenderCreated
+    }
   private var eglContextCreated = false
   private var renderNotSupported = false
 
@@ -223,7 +226,7 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
    * Keep a runnable to [prepareRenderFrame] to avoid creating a new one on every frame.
    */
   private val prepareRenderFrameRunnable: Runnable = Runnable {
-      prepareRenderFrame()
+      prepareRenderFrame(width = null, height = null, creatingSurface = false)
   }
   private fun postPrepareRenderFrame(delayMillis: Long = 0L) {
     renderHandlerThread.postDelayed(
@@ -265,10 +268,6 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
                 nativeRenderCreated = true
                 mapboxRenderer.createRenderer()
                 logI(TAG, "Native renderer created.")
-                mapboxRenderer.onSurfaceChanged(
-                  width = width,
-                  height = height
-                )
               }
               return true
             }
@@ -336,12 +335,10 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
     return true
   }
 
-  private fun checkSurfaceSizeChanged() {
-    if (sizeChanged) {
+  @RenderThread
+  private fun notifyRenderersSizeChanged(width: Int, height: Int) {
       mapboxRenderer.onSurfaceChanged(width = width, height = height)
       widgetRenderer.onSurfaceChanged(width = width, height = height)
-      sizeChanged = false
-    }
   }
 
   private fun checkWidgetRender() {
@@ -470,18 +467,34 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
       }
       eglContextCreated = false
       if (tryRecreate) {
-        setUpRenderThread(creatingSurface = true)
+        if (setUpRenderThread(creatingSurface = true)) {
+          notifyRenderersSizeChanged(width, height)
+        }
       } else {
         surface?.release()
       }
     }
   }
 
-  private fun prepareRenderFrame(creatingSurface: Boolean = false) {
-    // no need to do anything if we're waiting for next VSYNC already;
+  /**
+   * Responsible to prepare for rendering and schedule a new frame render (synchronized with the
+   * display's refresh rate using [Choreographer]).
+   * There are 3 main reasons to request a new frame:
+   * 1. A new surface is available ([creatingSurface] flag is true and [width] and [height] are given).
+   * 2. The surface was resized (new [width] and [height] are given).
+   * 3. Normal render pass ([creatingSurface] flag is false and no sizes are given).
+   */
+  @RenderThread
+  private fun prepareRenderFrame(
+    width: Int?,
+    height: Int?,
+    creatingSurface: Boolean
+  ) {
+    // no need to do anything if we're already waiting for next VSYNC (`doFrame`);
     // however if Android has sent us new surface - we must proceed up to `setUpRenderThread` or
     // otherwise main thread will end up having deadlock
-    if (awaitingNextVsync && !creatingSurface) {
+    // We also should go through if we've new sizes to set
+    if (awaitingNextVsync && !creatingSurface && width == null && height == null) {
       return
     }
     // Check first if we have to stop rendering at all (even if there was no EGL config) and cleanup EGL.
@@ -515,19 +528,21 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
       }
     }
     checkWidgetRender()
-    checkSurfaceSizeChanged()
+    if (width != null && height != null && renderThreadPrepared) {
+      notifyRenderersSizeChanged(width, height)
+    }
     Choreographer.getInstance().postFrameCallback(this)
     awaitingNextVsync = true
   }
 
   @UiThread
   fun onSurfaceSizeChanged(width: Int, height: Int) {
-    if (this.width != width || this.height != height) {
-      renderHandlerThread.post {
+    renderHandlerThread.post {
+      if (this.width != width || this.height != height) {
         this@MapboxRenderThread.width = width
         this@MapboxRenderThread.height = height
-        sizeChanged = true
-        prepareRenderFrame()
+        // Schedule a new frame to draw map with the new size
+        prepareRenderFrame(width = width, height = height, creatingSurface = false)
       }
     }
   }
@@ -586,8 +601,8 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
     }
     this.width = width
     this.height = height
-    widgetRenderer.onSurfaceChanged(width = width, height = height)
-    prepareRenderFrame(creatingSurface = true)
+    // Make sure we initialize renderer and schedule next frame to draw map.
+    prepareRenderFrame(width, height, creatingSurface = true)
   }
 
   @UiThread
@@ -604,6 +619,8 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
       surfaceProcessingLock.withLock {
         if (renderHandlerThread.isRunning) {
           renderHandlerThread.post {
+            this.width = width
+            this.height = height
             processAndroidSurface(surface, width, height)
           }
           logI(TAG, "onSurfaceCreated: waiting Android surface to be processed...")

maps-sdk/src/main/java/com/mapbox/maps/renderer/RenderThread.kt

@@ -11,6 +11,7 @@ package com.mapbox.maps.renderer
   AnnotationTarget.FUNCTION,
   AnnotationTarget.PROPERTY_GETTER,
   AnnotationTarget.PROPERTY_SETTER,
+  AnnotationTarget.PROPERTY,
   AnnotationTarget.CONSTRUCTOR,
   AnnotationTarget.CLASS,
 )

maps-sdk/src/test/java/com/mapbox/maps/renderer/MapboxRenderThreadTest.kt

@@ -200,6 +200,23 @@ class MapboxRenderThreadTest {
     }
   }
 
+  @Test
+  fun onSurfaceSizeChangedWhileBusyTest() {
+    val mapboxRenderer: MapboxRenderer = mockk(relaxUnitFun = true)
+    initRenderThread(mapboxRenderer)
+    provideValidSurface()
+    pauseHandler()
+    mapboxRenderThread.onSurfaceSizeChanged(2, 2)
+    mapboxRenderThread.onSurfaceSizeChanged(1, 1)
+    idleHandler()
+    verifyOrder {
+      mapboxRenderer.createRenderer()
+      mapboxRenderer.onSurfaceChanged(1, 1)
+      mapboxRenderer.onSurfaceChanged(2, 2)
+      mapboxRenderer.onSurfaceChanged(1, 1)
+    }
+  }
+
   @Test
   fun onSurfaceSizeChangedSameSizeTest() {
     initRenderThread()