From beccfaba3bb1a80a4e659c24d097a5e684077556 Mon Sep 17 00:00:00 2001 From: andrewlewis Date: Wed, 17 Nov 2021 19:23:20 +0000 Subject: [PATCH] Avoid buffers used by OpenGL getting GC'ed The `GlUtil` wrappers for attributes/uniforms allocate buffers that are passed into OpenGL, and it seems that the distorted output was caused by these buffers being garbage collected. The issue was difficult to reproduce manually. Add a test that does repeated transcodes in a loop (marked `@Ignore` for now because it runs for a long time and requires network access so may be flaky) to make it easier to run many transcodes and look for inconsistent output. For now the consistency check is just based on the file size which is very likely to change if the actual video frames change. Verified that the test fails before, and passes once the attributes and uniforms are stored in fields as in this CL. PiperOrigin-RevId: 410581706 --- library/transformer/build.gradle | 1 + .../transformer/AndroidTestUtil.java | 70 ++++++++++++++++++- .../RepeatedTranscodeTransformationTest.java | 59 ++++++++++++++++ .../transformer/OpenGlFrameEditor.java | 28 +++++++- 4 files changed, 153 insertions(+), 5 deletions(-) create mode 100644 library/transformer/src/androidTest/java/com/google/android/exoplayer2/transformer/RepeatedTranscodeTransformationTest.java diff --git a/library/transformer/build.gradle b/library/transformer/build.gradle index 28742d1f6f..a06a70a320 100644 --- a/library/transformer/build.gradle +++ b/library/transformer/build.gradle @@ -48,6 +48,7 @@ dependencies { testImplementation 'com.google.truth:truth:' + truthVersion androidTestImplementation 'junit:junit:' + junitVersion androidTestImplementation 'androidx.test:runner:' + androidxTestRunnerVersion + androidTestImplementation 'com.google.truth:truth:' + truthVersion androidTestImplementation project(modulePrefix + 'testutils') } diff --git a/library/transformer/src/androidTest/java/com/google/android/exoplayer2/transformer/AndroidTestUtil.java b/library/transformer/src/androidTest/java/com/google/android/exoplayer2/transformer/AndroidTestUtil.java index 1afb81f667..fb45925a95 100644 --- a/library/transformer/src/androidTest/java/com/google/android/exoplayer2/transformer/AndroidTestUtil.java +++ b/library/transformer/src/androidTest/java/com/google/android/exoplayer2/transformer/AndroidTestUtil.java @@ -27,14 +27,25 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicReference; import org.checkerframework.checker.nullness.compatqual.NullableType; -/** Utility methods for instrumentation tests. */ +/** Utilities for instrumentation tests. */ /* package */ final class AndroidTestUtil { public static final String MP4_ASSET_URI = "asset:///media/mp4/sample.mp4"; public static final String SEF_ASSET_URI = "asset:///media/mp4/sample_sef_slow_motion.mp4"; + public static final String REMOTE_MP4_10_SECONDS_URI_STRING = + "https://storage.googleapis.com/exoplayer-test-media-1/mp4/android-screens-10s.mp4"; + + /** Information about the result of successfully running a transformer. */ + public static final class TransformationResult { + public long outputSizeBytes; + + private TransformationResult(long outputSizeBytes) { + this.outputSizeBytes = outputSizeBytes; + } + } /** Transforms the {@code uriString} with the {@link Transformer}. */ - public static void runTransformer(Context context, Transformer transformer, String uriString) - throws Exception { + public static TransformationResult runTransformer( + Context context, Transformer transformer, String uriString) throws Exception { AtomicReference<@NullableType Exception> exceptionReference = new AtomicReference<>(); CountDownLatch countDownLatch = new CountDownLatch(1); @@ -74,6 +85,59 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; if (exception != null) { throw exception; } + long outputSizeBytes = externalCacheFile.length(); + return new TransformationResult(outputSizeBytes); + } finally { + externalCacheFile.delete(); + } + } + + // TODO(internal b/202131097): Deduplicate with the other overload when TranscodingTransformer is + // merged into Transformer. + /** Transforms the {@code uriString} with the {@link TranscodingTransformer}. */ + public static TransformationResult runTransformer( + Context context, TranscodingTransformer transformer, String uriString) throws Exception { + AtomicReference<@NullableType Exception> exceptionReference = new AtomicReference<>(); + CountDownLatch countDownLatch = new CountDownLatch(1); + + TranscodingTransformer testTransformer = + transformer + .buildUpon() + .setListener( + new TranscodingTransformer.Listener() { + @Override + public void onTransformationCompleted(MediaItem inputMediaItem) { + countDownLatch.countDown(); + } + + @Override + public void onTransformationError(MediaItem inputMediaItem, Exception exception) { + exceptionReference.set(exception); + countDownLatch.countDown(); + } + }) + .build(); + + Uri uri = Uri.parse(uriString); + File externalCacheFile = createExternalCacheFile(uri, context); + try { + InstrumentationRegistry.getInstrumentation() + .runOnMainSync( + () -> { + try { + testTransformer.startTransformation( + MediaItem.fromUri(uri), externalCacheFile.getAbsolutePath()); + } catch (IOException e) { + exceptionReference.set(e); + } + }); + countDownLatch.await(); + @Nullable Exception exception = exceptionReference.get(); + if (exception != null) { + throw exception; + } + long outputSizeBytes = externalCacheFile.length(); + return new TransformationResult(outputSizeBytes); } finally { externalCacheFile.delete(); } diff --git a/library/transformer/src/androidTest/java/com/google/android/exoplayer2/transformer/RepeatedTranscodeTransformationTest.java b/library/transformer/src/androidTest/java/com/google/android/exoplayer2/transformer/RepeatedTranscodeTransformationTest.java new file mode 100644 index 0000000000..37799043f4 --- /dev/null +++ b/library/transformer/src/androidTest/java/com/google/android/exoplayer2/transformer/RepeatedTranscodeTransformationTest.java @@ -0,0 +1,59 @@ +/* + * Copyright 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.google.android.exoplayer2.transformer; + +import static com.google.android.exoplayer2.transformer.AndroidTestUtil.runTransformer; +import static com.google.common.truth.Truth.assertWithMessage; + +import android.content.Context; +import androidx.test.core.app.ApplicationProvider; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.google.android.exoplayer2.C; +import com.google.android.exoplayer2.util.MimeTypes; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** Tests repeated transcoding operations (as a stress test and to help reproduce flakiness). */ +@RunWith(AndroidJUnit4.class) +public final class RepeatedTranscodeTransformationTest { + + private static final int TRANSCODE_COUNT = 10; + + @Test + public void repeatedTranscode_givesConsistentLengthOutput() throws Exception { + Context context = ApplicationProvider.getApplicationContext(); + TranscodingTransformer transcodingTransformer = + new TranscodingTransformer.Builder() + .setVideoMimeType(MimeTypes.VIDEO_H265) + .setContext(context) + .build(); + + long previousOutputSizeBytes = C.LENGTH_UNSET; + for (int i = 0; i < TRANSCODE_COUNT; i++) { + // Use a long video in case an error occurs a while after the start of the video. + long outputSizeBytes = + runTransformer( + context, transcodingTransformer, AndroidTestUtil.REMOTE_MP4_10_SECONDS_URI_STRING) + .outputSizeBytes; + if (previousOutputSizeBytes != C.LENGTH_UNSET) { + assertWithMessage("Unexpected output size on transcode " + i + " out of " + TRANSCODE_COUNT) + .that(outputSizeBytes) + .isEqualTo(previousOutputSizeBytes); + } + previousOutputSizeBytes = outputSizeBytes; + } + } +} diff --git a/library/transformer/src/main/java/com/google/android/exoplayer2/transformer/OpenGlFrameEditor.java b/library/transformer/src/main/java/com/google/android/exoplayer2/transformer/OpenGlFrameEditor.java index 9579de53a9..cbf19da7fa 100644 --- a/library/transformer/src/main/java/com/google/android/exoplayer2/transformer/OpenGlFrameEditor.java +++ b/library/transformer/src/main/java/com/google/android/exoplayer2/transformer/OpenGlFrameEditor.java @@ -118,7 +118,14 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; } return new OpenGlFrameEditor( - eglDisplay, eglContext, eglSurface, textureId, checkNotNull(textureTransformUniform)); + eglDisplay, + eglContext, + eglSurface, + textureId, + checkNotNull(textureTransformUniform), + copyProgram, + copyAttributes, + copyUniforms); } // Predefined shader values. @@ -137,6 +144,17 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; private final Surface inputSurface; private final GlUtil.Uniform textureTransformUniform; + // TODO(internal: b/206631334): These fields ensure buffers passed to GL are not GC'ed. Implement + // a better way of doing this so they aren't just unused fields. + @SuppressWarnings("unused") + private final GlUtil.Program copyProgram; + + @SuppressWarnings("unused") + private final GlUtil.Attribute[] copyAttributes; + + @SuppressWarnings("unused") + private final GlUtil.Uniform[] copyUniforms; + private volatile boolean hasInputData; private OpenGlFrameEditor( @@ -144,12 +162,18 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; EGLContext eglContext, EGLSurface eglSurface, int textureId, - GlUtil.Uniform textureTransformUniform) { + GlUtil.Uniform textureTransformUniform, + GlUtil.Program copyProgram, + GlUtil.Attribute[] copyAttributes, + GlUtil.Uniform[] copyUniforms) { this.eglDisplay = eglDisplay; this.eglContext = eglContext; this.eglSurface = eglSurface; this.textureId = textureId; this.textureTransformUniform = textureTransformUniform; + this.copyProgram = copyProgram; + this.copyAttributes = copyAttributes; + this.copyUniforms = copyUniforms; textureTransformMatrix = new float[16]; inputSurfaceTexture = new SurfaceTexture(textureId); inputSurfaceTexture.setOnFrameAvailableListener(surfaceTexture -> hasInputData = true);