fix(Android): Render Target ANR (#12470) fc77e0e0ef

The synchronization point introduced when creating render targets may be causing causing ANRs. This fixes that by removing the need to synchronize.

We initially decided to create the render target on the command server since we need the sample count, which is only available with the OpenGL context being active. We also wanted to keep surface creation synchronous, so we made render target's runOnce block the main until done. It was assumed that it would be fast and done once early on.

But this was a dangerous assumption and lead to ANRs in practice. We want to avoid synchronization with the main thread as much as possible. Rather than make the whole surface creation deferred, we instead retain the sync API but only allocate a holder (or "handle" though not in the command queue sense). This is effectively a pointer-to-a-pointer which only allocates the true render target at the time of first use. This works so long as we assume that allocation to be cheap, which currently it is.

Because of this design, we no longer need the command queue to create the render target. The sample count is accessible at the deferred creation time. So the whole creation through the command queue can be shifted out and simply done in the render context bindings.

Adds a test to ensure the main thread is no longer blocked.

Co-authored-by: Erik <erik@rive.app>

Author: ErikUggeldahl

.rive_head

@@ -1 +1 @@
-3b74a521481e40e29c940665e26f074e7c5fca97
+fc77e0e0eff91fc2d1b554e8cb664ca59a0b1dbd

kotlin/src/androidTest/kotlin/app/rive/core/CommandQueueLifecycleTest.kt

@@ -45,11 +45,11 @@ class CommandQueueLifecycleTest : RiveAndroidTest() {
 
         assertTrue(
             releaseAttempted.await(2, TimeUnit.SECONDS),
-            "Command-server release callback did not run"
+            "Command server release callback did not run"
         )
         assertTrue(
             thrown.get() is IllegalStateException,
-            "Expected command-server release to throw IllegalStateException"
+            "Expected command server release to throw IllegalStateException"
         )
         assertFalse(
             commandQueue.isDisposed,

kotlin/src/androidTest/kotlin/app/rive/core/CommandQueueTestHelpers.kt

@@ -16,7 +16,7 @@ import kotlin.test.assertTrue
 /**
  * Runs [block] while a temporary background thread continuously polls this command queue.
  *
- * Android instrumentation tests that enqueue command-server work need polling even when the code
+ * Android instrumentation tests that enqueue command server work need polling even when the code
  * under test is not running inside the normal lifecycle-driven polling loop. Polling is stopped in
  * `finally` so failures inside [block] do not leak the helper thread.
  */
@@ -115,6 +115,7 @@ private suspend fun RiveWorker.loadRiveFileOrFail(@RawRes rawResourceId: Int): R
         is Result.Success -> result.value
         is Result.Error ->
             throw AssertionError("Failed to load Rive file: ${result.throwable.message}")
+
         is Result.Loading ->
             throw AssertionError("RiveFile.fromSource should not return Loading")
     }

kotlin/src/androidTest/kotlin/app/rive/core/RiveSurfaceLifecycleTest.kt

@@ -12,14 +12,22 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
 import app.rive.Fit
 import app.rive.RiveAndroidTest
 import app.rive.runtime.kotlin.test.R
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.async
+import kotlinx.coroutines.runBlocking
+import kotlinx.coroutines.withTimeoutOrNull
 import org.junit.runner.RunWith
 import java.util.concurrent.CountDownLatch
 import java.util.concurrent.TimeUnit
 import java.util.concurrent.atomic.AtomicBoolean
 import kotlin.test.Test
 import kotlin.test.assertFailsWith
 import kotlin.test.assertFalse
+import kotlin.test.assertNotNull
 import kotlin.test.assertTrue
+import kotlin.time.Duration.Companion.seconds
+
+private val TEST_TIMEOUT = 3.seconds.inWholeMilliseconds
 
 @RunWith(AndroidJUnit4::class)
 class RiveSurfaceLifecycleTest : RiveAndroidTest() {
@@ -30,10 +38,7 @@ class RiveSurfaceLifecycleTest : RiveAndroidTest() {
 
         surface.close()
 
-        assertTrue(
-            closeableSurface.awaitClosed(),
-            "Surface-backed resources were not closed before CommandQueue disposal completed"
-        )
+        assertClosed(closeableSurface)
     }
 
     @Test
@@ -55,10 +60,42 @@ class RiveSurfaceLifecycleTest : RiveAndroidTest() {
         surface.close()
 
         assertDisposed(commandQueue)
-        assertTrue(
-            closeableSurface.awaitClosed(),
-            "Surface-backed resources were not closed before CommandQueue disposal completed"
-        )
+        assertClosed(closeableSurface)
+    }
+
+    @Test
+    fun createRiveSurface_returnsImmediately() = runBlocking {
+        val blockEntered = CountDownLatch(1)
+        val blockMayExit = CountDownLatch(1)
+
+        // Block the command server before surface creation. Surface creation should only allocate
+        // the lazy render target holder and must not wait for this queued work to complete.
+        riveWorker.runOnCommandServer {
+            blockEntered.countDown()
+            blockMayExit.await()
+        }
+
+        val closeableSurface = LatchingImageReaderSurface()
+        try {
+            assertTrue(
+                blockEntered.await(TEST_TIMEOUT, TimeUnit.MILLISECONDS),
+                "Command server did not enter blocking test work"
+            )
+
+            val createSurface = async(Dispatchers.Default) {
+                riveWorker.createRiveSurface(closeableSurface)
+            }
+            assertNotNull(
+                withTimeoutOrNull(TEST_TIMEOUT) {
+                    createSurface.await()
+                },
+                "Surface creation did not complete before command server block was allowed to exit"
+            ).close()
+        } finally {
+            blockMayExit.countDown()
+        }
+
+        assertClosed(closeableSurface)
     }
 
     @Test
@@ -86,10 +123,7 @@ class RiveSurfaceLifecycleTest : RiveAndroidTest() {
         surface.close()
         gate.countDown()
 
-        assertTrue(
-            closeableSurface.awaitClosed(),
-            "Surface-backed resources were not closed before CommandQueue disposal completed"
-        )
+        assertClosed(closeableSurface)
         assertFalse(
             closeableSurface.awaitFrameAvailable(timeoutMillis = 200),
             "Expected draw queued before surface close to be canceled before producing a frame"
@@ -198,4 +232,11 @@ class RiveSurfaceLifecycleTest : RiveAndroidTest() {
 
         fun awaitClosed(): Boolean = closed.await(2, TimeUnit.SECONDS)
     }
+
+    private fun assertClosed(surface: LatchingImageReaderSurface) {
+        assertTrue(
+            surface.awaitClosed(),
+            "Surface-backed resources were not closed before CommandQueue disposal completed"
+        )
+    }
 }

kotlin/src/main/cpp/include/models/lazy_framebuffer_render_target_gl.hpp

@@ -0,0 +1,90 @@
+#pragma once
+
+#include <GLES3/gl3.h>
+#include <cstdint>
+#include <memory>
+
+#include "helpers/rive_log.hpp"
+#include "rive/renderer/gl/render_target_gl.hpp"
+
+namespace rive_android
+{
+
+/**
+ * Owns a GL framebuffer render target whose concrete GL resources are created
+ * on first use.
+ *
+ * Android surface creation can happen on the UI thread, while the Rive GL
+ * context is owned by the command server thread. This holder allows surface
+ * creation to return synchronously without blocking the main thread on command
+ * server backlog or GL work. It is returned to Kotlin as an opaque native
+ * handle, but it defers GL work until getOrCreate() runs on the command server
+ * after the target EGL surface has been made current.
+ *
+ * Creating the concrete render target is assumed to be cheap enough to perform
+ * lazily during the first draw.
+ *
+ * This holder is uniquely owned by RiveSurface disposal ordering and is deleted
+ * on the command server thread after pending draws for the same draw key have
+ * been canceled.
+ */
+class LazyFramebufferRenderTargetGL
+{
+public:
+    LazyFramebufferRenderTargetGL(uint32_t width, uint32_t height) :
+        m_width(width), m_height(height)
+    {}
+
+    LazyFramebufferRenderTargetGL(const LazyFramebufferRenderTargetGL&) =
+        delete;
+    LazyFramebufferRenderTargetGL& operator=(
+        const LazyFramebufferRenderTargetGL&) = delete;
+
+    /**
+     * Returns the concrete GL render target, creating it if necessary.
+     *
+     * Must be called on the command server thread with the intended EGL surface
+     * already current. The current framebuffer's sample count is captured on
+     * first creation and reused for the lifetime of this surface.
+     */
+    rive::gpu::RenderTargetGL* getOrCreate()
+    {
+        if (!m_renderTarget)
+        {
+            GLint actualSampleCount = 1;
+            glBindFramebuffer(GL_FRAMEBUFFER, 0);
+            glGetIntegerv(GL_SAMPLES, &actualSampleCount);
+            RiveLogD("RiveN/LazyRT",
+                     "Creating render target on command server "
+                     "(sample count: %d)",
+                     actualSampleCount);
+
+            m_renderTarget.reset(
+                new rive::gpu::FramebufferRenderTargetGL(m_width,
+                                                         m_height,
+                                                         0, // Framebuffer ID
+                                                         actualSampleCount));
+        }
+        return m_renderTarget.get();
+    }
+
+private:
+    struct RenderTargetUnref
+    {
+        void operator()(
+            rive::gpu::FramebufferRenderTargetGL* renderTarget) const
+        {
+            if (renderTarget != nullptr)
+            {
+                renderTarget->unref();
+            }
+        }
+    };
+
+    const uint32_t m_width;
+    const uint32_t m_height;
+    std::unique_ptr<rive::gpu::FramebufferRenderTargetGL, RenderTargetUnref>
+        m_renderTarget;
+};
+
+} // namespace rive_android