From 0780a21b3f998bad880c5e8e324eae8a19c806ba Mon Sep 17 00:00:00 2001
From: andrewlewis <andrewlewis@google.com>
Date: Fri, 24 Feb 2023 13:03:35 +0000
Subject: [PATCH] Continue releasing programs on failure

Currently if releasing a shader program throws we don't release the GL
context, which could leak resources, and any errors are silently dropped
as we suppress notifications during releasing.

Improve resource cleanup and debuggability of errors from custom effects
by continuing to release shaders on failure (for runtime and
`VideoFrameProcessingException`s) and always clean up the GL context.

Note: this doesn't help with the case where releasing a custom shader
blocks for a long time, causing releasing the frame processor to
time out.
PiperOrigin-RevId: 512042501
---
 .../effect/DefaultVideoFrameProcessor.java    | 25 ++++++++++++++-----
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/libraries/effect/src/main/java/androidx/media3/effect/DefaultVideoFrameProcessor.java b/libraries/effect/src/main/java/androidx/media3/effect/DefaultVideoFrameProcessor.java
index 114ab77b2b..335e72ce87 100644
--- a/libraries/effect/src/main/java/androidx/media3/effect/DefaultVideoFrameProcessor.java
+++ b/libraries/effect/src/main/java/androidx/media3/effect/DefaultVideoFrameProcessor.java
@@ -41,6 +41,7 @@ import androidx.media3.common.SurfaceInfo;
 import androidx.media3.common.VideoFrameProcessingException;
 import androidx.media3.common.VideoFrameProcessor;
 import androidx.media3.common.util.GlUtil;
+import androidx.media3.common.util.Log;
 import androidx.media3.common.util.UnstableApi;
 import androidx.media3.common.util.Util;
 import com.google.common.collect.ImmutableList;
@@ -61,7 +62,7 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull;
 public final class DefaultVideoFrameProcessor implements VideoFrameProcessor {
 
   /** A factory for {@link DefaultVideoFrameProcessor} instances. */
-  public static class Factory implements VideoFrameProcessor.Factory {
+  public static final class Factory implements VideoFrameProcessor.Factory {
     /**
      * {@inheritDoc}
      *
@@ -350,6 +351,8 @@ public final class DefaultVideoFrameProcessor implements VideoFrameProcessor {
     }
   }
 
+  private static final String TAG = "DefaultFrameProcessor";
+
   private static final String THREAD_NAME = "Effect:GlThread";
   private static final long RELEASE_WAIT_TIME_MS = 100;
 
@@ -548,11 +551,21 @@ public final class DefaultVideoFrameProcessor implements VideoFrameProcessor {
    * <p>This method must be called on the {@linkplain #THREAD_NAME background thread}.
    */
   @WorkerThread
-  private void releaseShaderProgramsAndDestroyGlContext()
-      throws GlUtil.GlException, VideoFrameProcessingException {
-    for (int i = 0; i < allShaderPrograms.size(); i++) {
-      allShaderPrograms.get(i).release();
+  private void releaseShaderProgramsAndDestroyGlContext() {
+    try {
+      for (int i = 0; i < allShaderPrograms.size(); i++) {
+        try {
+          allShaderPrograms.get(i).release();
+        } catch (Exception e) {
+          Log.e(TAG, "Error releasing shader program", e);
+        }
+      }
+    } finally {
+      try {
+        GlUtil.destroyEglContext(eglDisplay, eglContext);
+      } catch (GlUtil.GlException e) {
+        Log.e(TAG, "Error releasing GL context", e);
+      }
     }
-    GlUtil.destroyEglContext(eglDisplay, eglContext);
   }
 }