From be51913b81cf3a542feba17d37016443ee27c3fc Mon Sep 17 00:00:00 2001 From: kimvde Date: Wed, 12 Feb 2025 03:19:06 -0800 Subject: [PATCH] Refactor GlObjectsProvider release logic When possible, make it the caller's responsibility to release the GlObjectsProvider. It's good practice that the class creating an object also releases it. It avoids releasing the same object (here, the GlObjectsProvider) multiple times, which was causing DefaultVideoCompositorPixelTest to fail. PiperOrigin-RevId: 725983417 --- RELEASENOTES.md | 3 ++ .../media3/effect/DefaultVideoCompositor.java | 11 ++----- .../effect/DefaultVideoFrameProcessor.java | 30 +++++++++++++------ .../effect/MultipleInputVideoGraph.java | 15 +++++----- .../DefaultVideoCompositorPixelTest.java | 16 ++++++++-- 5 files changed, 48 insertions(+), 27 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 2de6f2acae..a91885df09 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -10,6 +10,9 @@ * Add `MediaProjectionAssetLoader`, which provides media from a `MediaProjection` for screen recording. * Add `#getInputFormat()` to `Codec` interface. + * Shift the responsibility to release the `GlObjectsProvider` onto the + caller in `DefaultVideoFrameProcessor` and `DefaultVideoCompositor` when + possible. * Track Selection: * Extractors: * DataSource: diff --git a/libraries/effect/src/main/java/androidx/media3/effect/DefaultVideoCompositor.java b/libraries/effect/src/main/java/androidx/media3/effect/DefaultVideoCompositor.java index 1814262602..ed70566027 100644 --- a/libraries/effect/src/main/java/androidx/media3/effect/DefaultVideoCompositor.java +++ b/libraries/effect/src/main/java/androidx/media3/effect/DefaultVideoCompositor.java @@ -105,8 +105,9 @@ public final class DefaultVideoCompositor implements VideoCompositor { /** * Creates an instance. * - *

The {@link ExecutorService} must be {@linkplain ExecutorService#shutdown shut down} by the - * caller. + *

It's the caller's responsibility to {@linkplain GlObjectsProvider#release(EGLDisplay) + * release} the {@link GlObjectsProvider} on the {@link ExecutorService}'s thread, and to + * {@linkplain ExecutorService#shutdown shut down} the {@link ExecutorService}. */ public DefaultVideoCompositor( Context context, @@ -409,12 +410,6 @@ public final class DefaultVideoCompositor implements VideoCompositor { GlUtil.destroyEglSurface(eglDisplay, placeholderEglSurface); } catch (GlUtil.GlException e) { Log.e(TAG, "Error releasing GL resources", e); - } finally { - try { - glObjectsProvider.release(checkNotNull(eglDisplay)); - } catch (GlUtil.GlException e) { - Log.e(TAG, "Error releasing GL objects", e); - } } } 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 b68a964876..aefc55b53e 100644 --- a/libraries/effect/src/main/java/androidx/media3/effect/DefaultVideoFrameProcessor.java +++ b/libraries/effect/src/main/java/androidx/media3/effect/DefaultVideoFrameProcessor.java @@ -235,6 +235,10 @@ public final class DefaultVideoFrameProcessor implements VideoFrameProcessor { * Sets the {@link GlObjectsProvider}. * *

The default value is a {@link DefaultGlObjectsProvider}. + * + *

If both the {@link GlObjectsProvider} and the {@link ExecutorService} are set, it's the + * caller's responsibility to release the {@link GlObjectsProvider} on the {@link + * ExecutorService}'s thread. */ @CanIgnoreReturnValue public Builder setGlObjectsProvider(GlObjectsProvider glObjectsProvider) { @@ -408,16 +412,16 @@ public final class DefaultVideoFrameProcessor implements VideoFrameProcessor { throws VideoFrameProcessingException { // TODO(b/261188041) Add tests to verify the Listener is invoked on the given Executor. - boolean shouldShutdownExecutorService = executorService == null; ExecutorService instanceExecutorService = executorService == null ? Util.newSingleThreadExecutor(THREAD_NAME) : executorService; - + boolean shouldShutdownExecutorService = executorService == null; VideoFrameProcessingTaskExecutor videoFrameProcessingTaskExecutor = new VideoFrameProcessingTaskExecutor( instanceExecutorService, shouldShutdownExecutorService, listener::onError); - GlObjectsProvider glObjectsProvider = - this.glObjectsProvider == null ? new DefaultGlObjectsProvider() : this.glObjectsProvider; + boolean shouldReleaseGlObjectsProvider = glObjectsProvider == null || executorService == null; + GlObjectsProvider instanceGlObjectsProvider = + glObjectsProvider == null ? new DefaultGlObjectsProvider() : glObjectsProvider; Future defaultVideoFrameProcessorFuture = instanceExecutorService.submit( @@ -431,7 +435,8 @@ public final class DefaultVideoFrameProcessor implements VideoFrameProcessor { videoFrameProcessingTaskExecutor, listenerExecutor, listener, - glObjectsProvider, + instanceGlObjectsProvider, + shouldReleaseGlObjectsProvider, textureOutputListener, textureOutputCapacity, repeatLastRegisteredFrame, @@ -453,6 +458,7 @@ public final class DefaultVideoFrameProcessor implements VideoFrameProcessor { private final Context context; private final GlObjectsProvider glObjectsProvider; + private final boolean shouldReleaseGlObjectsProvider; private final EGLDisplay eglDisplay; private final InputSwitcher inputSwitcher; private final VideoFrameProcessingTaskExecutor videoFrameProcessingTaskExecutor; @@ -493,6 +499,7 @@ public final class DefaultVideoFrameProcessor implements VideoFrameProcessor { private DefaultVideoFrameProcessor( Context context, GlObjectsProvider glObjectsProvider, + boolean shouldReleaseGlObjectsProvider, EGLDisplay eglDisplay, InputSwitcher inputSwitcher, VideoFrameProcessingTaskExecutor videoFrameProcessingTaskExecutor, @@ -504,6 +511,7 @@ public final class DefaultVideoFrameProcessor implements VideoFrameProcessor { DebugViewProvider debugViewProvider) { this.context = context; this.glObjectsProvider = glObjectsProvider; + this.shouldReleaseGlObjectsProvider = shouldReleaseGlObjectsProvider; this.eglDisplay = eglDisplay; this.inputSwitcher = inputSwitcher; this.videoFrameProcessingTaskExecutor = videoFrameProcessingTaskExecutor; @@ -833,6 +841,7 @@ public final class DefaultVideoFrameProcessor implements VideoFrameProcessor { Executor videoFrameProcessorListenerExecutor, Listener listener, GlObjectsProvider glObjectsProvider, + boolean shouldReleaseGlObjectsProvider, @Nullable GlTextureProducer.Listener textureOutputListener, int textureOutputCapacity, boolean repeatLastRegisteredFrame, @@ -890,6 +899,7 @@ public final class DefaultVideoFrameProcessor implements VideoFrameProcessor { return new DefaultVideoFrameProcessor( context, glObjectsProvider, + shouldReleaseGlObjectsProvider, eglDisplay, inputSwitcher, videoFrameProcessingTaskExecutor, @@ -1139,10 +1149,12 @@ public final class DefaultVideoFrameProcessor implements VideoFrameProcessor { Log.e(TAG, "Error releasing shader program", e); } } finally { - try { - glObjectsProvider.release(eglDisplay); - } catch (GlUtil.GlException e) { - Log.e(TAG, "Error releasing GL objects", e); + if (shouldReleaseGlObjectsProvider) { + try { + glObjectsProvider.release(eglDisplay); + } catch (GlUtil.GlException e) { + Log.e(TAG, "Error releasing GL objects", e); + } } } } diff --git a/libraries/effect/src/main/java/androidx/media3/effect/MultipleInputVideoGraph.java b/libraries/effect/src/main/java/androidx/media3/effect/MultipleInputVideoGraph.java index 38160ee382..ed61e73046 100644 --- a/libraries/effect/src/main/java/androidx/media3/effect/MultipleInputVideoGraph.java +++ b/libraries/effect/src/main/java/androidx/media3/effect/MultipleInputVideoGraph.java @@ -76,7 +76,7 @@ public abstract class MultipleInputVideoGraph implements VideoGraph { private final Context context; private final ColorInfo outputColorInfo; - private final SingleContextGlObjectsProvider glObjectsProvider; + private final GlObjectsProvider glObjectsProvider; private final DebugViewProvider debugViewProvider; private final VideoGraph.Listener listener; private final Executor listenerExecutor; @@ -310,12 +310,9 @@ public abstract class MultipleInputVideoGraph implements VideoGraph { sharedExecutorService.submit( () -> { try { - // The eglContext is not released by any of the frame processors. - if (glObjectsProvider.singleEglContext != null) { - destroyEglContext(getDefaultEglDisplay(), glObjectsProvider.singleEglContext); - } + glObjectsProvider.release(getDefaultEglDisplay()); } catch (Exception e) { - Log.e(TAG, "Error releasing GL context", e); + Log.e(TAG, "Error releasing GlObjectsProvider", e); } }); @@ -520,8 +517,10 @@ public abstract class MultipleInputVideoGraph implements VideoGraph { } @Override - public void release(EGLDisplay eglDisplay) { - // The eglContext is released in the VideoGraph after all VideoFrameProcessors are released. + public void release(EGLDisplay eglDisplay) throws GlException { + if (singleEglContext != null) { + destroyEglContext(eglDisplay, singleEglContext); + } } } } diff --git a/libraries/transformer/src/androidTest/java/androidx/media3/transformer/DefaultVideoCompositorPixelTest.java b/libraries/transformer/src/androidTest/java/androidx/media3/transformer/DefaultVideoCompositorPixelTest.java index 7a367ddd9f..50a138d986 100644 --- a/libraries/transformer/src/androidTest/java/androidx/media3/transformer/DefaultVideoCompositorPixelTest.java +++ b/libraries/transformer/src/androidTest/java/androidx/media3/transformer/DefaultVideoCompositorPixelTest.java @@ -48,6 +48,7 @@ import androidx.media3.common.OverlaySettings; import androidx.media3.common.VideoCompositorSettings; import androidx.media3.common.VideoFrameProcessingException; import androidx.media3.common.util.GlUtil; +import androidx.media3.common.util.Log; import androidx.media3.common.util.Size; import androidx.media3.common.util.Util; import androidx.media3.effect.AlphaScale; @@ -74,6 +75,7 @@ import java.util.List; import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicReference; import org.checkerframework.checker.nullness.qual.MonotonicNonNull; import org.junit.After; @@ -94,6 +96,7 @@ public final class DefaultVideoCompositorPixelTest { @Rule public final TestName testName = new TestName(); + private static final String TAG = "DefaultVideoCompositorPixelTest"; private static final String ORIGINAL_PNG_ASSET_PATH = "media/png/media3test_srgb.png"; private static final String TEST_DIRECTORY = "test-generated-goldens/CompositorTestTimestamps/"; private static final ImmutableList> TWO_INPUT_COMPOSITOR_EFFECT_LISTS = @@ -623,6 +626,7 @@ public final class DefaultVideoCompositorPixelTest { private final List inputVideoFrameProcessorTestRunners; private final VideoCompositor videoCompositor; private final ExecutorService sharedExecutorService; + private final GlObjectsProvider glObjectsProvider; private final AtomicReference compositionException; private final CountDownLatch compositorEnded; private final String testId; @@ -660,7 +664,7 @@ public final class DefaultVideoCompositorPixelTest { this.testId = testId; timeoutMs = inputEffectLists.size() * VIDEO_FRAME_PROCESSING_WAIT_MS; sharedExecutorService = Util.newSingleThreadExecutor("Effect:Shared:GlThread"); - GlObjectsProvider glObjectsProvider = new DefaultGlObjectsProvider(); + glObjectsProvider = new DefaultGlObjectsProvider(); compositionException = new AtomicReference<>(); outputTimestampsToBitmaps = new LinkedHashMap<>(); @@ -800,7 +804,15 @@ public final class DefaultVideoCompositorPixelTest { inputVideoFrameProcessorTestRunners.get(i).release(); } videoCompositor.release(); - + Future unused = + sharedExecutorService.submit( + () -> { + try { + glObjectsProvider.release(GlUtil.getDefaultEglDisplay()); + } catch (Exception e) { + Log.e(TAG, "Error releasing GlObjectsProvider", e); + } + }); try { sharedExecutorService.shutdown(); if (!sharedExecutorService.awaitTermination(timeoutMs, MILLISECONDS)) {