From 85db94782a6106f186348a0c35e4423cdda0e07e Mon Sep 17 00:00:00 2001 From: christosts Date: Wed, 24 Jan 2024 11:47:28 -0800 Subject: [PATCH] Rollback of https://github.com/androidx/media/commit/e364510937ac1f3c4eb14fc34d2d772a64645ced PiperOrigin-RevId: 601187997 --- ...Test.java => TransformerProgressTest.java} | 181 +++++++++++++++++- .../transformer/TransformerInternal.java | 93 ++++++--- 2 files changed, 237 insertions(+), 37 deletions(-) rename libraries/transformer/src/androidTest/java/androidx/media3/transformer/{TransformerTrimOptimizationProgressTest.java => TransformerProgressTest.java} (57%) diff --git a/libraries/transformer/src/androidTest/java/androidx/media3/transformer/TransformerTrimOptimizationProgressTest.java b/libraries/transformer/src/androidTest/java/androidx/media3/transformer/TransformerProgressTest.java similarity index 57% rename from libraries/transformer/src/androidTest/java/androidx/media3/transformer/TransformerTrimOptimizationProgressTest.java rename to libraries/transformer/src/androidTest/java/androidx/media3/transformer/TransformerProgressTest.java index 69ba5038a6..278f604553 100644 --- a/libraries/transformer/src/androidTest/java/androidx/media3/transformer/TransformerTrimOptimizationProgressTest.java +++ b/libraries/transformer/src/androidTest/java/androidx/media3/transformer/TransformerProgressTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2023 The Android Open Source Project + * Copyright 2024 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. @@ -15,6 +15,7 @@ */ package androidx.media3.transformer; +import static androidx.media3.common.util.Assertions.checkStateNotNull; import static androidx.media3.common.util.Util.isRunningOnEmulator; import static androidx.media3.transformer.AndroidTestUtil.MP4_TRIM_OPTIMIZATION_URI_STRING; import static androidx.media3.transformer.Transformer.PROGRESS_STATE_AVAILABLE; @@ -25,21 +26,33 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assume.assumeFalse; import static org.junit.Assume.assumeTrue; +import android.app.Instrumentation; import android.content.Context; +import androidx.media3.common.GlObjectsProvider; +import androidx.media3.common.GlTextureInfo; import androidx.media3.common.MediaItem; +import androidx.media3.common.VideoFrameProcessingException; import androidx.media3.common.util.NullableType; import androidx.media3.common.util.Util; +import androidx.media3.effect.BaseGlShaderProgram; +import androidx.media3.effect.Brightness; import androidx.media3.effect.DebugTraceUtil; +import androidx.media3.effect.GlEffect; +import androidx.media3.effect.GlShaderProgram; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; import androidx.test.platform.app.InstrumentationRegistry; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.util.concurrent.SettableFuture; import java.io.File; +import java.util.ArrayList; import java.util.Queue; import java.util.concurrent.ConcurrentLinkedDeque; +import java.util.concurrent.Executor; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import org.checkerframework.checker.nullness.qual.EnsuresNonNull; import org.checkerframework.checker.nullness.qual.MonotonicNonNull; import org.checkerframework.checker.nullness.qual.RequiresNonNull; @@ -49,12 +62,11 @@ import org.junit.Test; import org.junit.rules.TestName; import org.junit.runner.RunWith; -/** - * End-to-end instrumentation test for {@link Transformer#getProgress} when {@link - * Transformer.Builder#experimentalSetTrimOptimizationEnabled} is enabled. - */ +/** End-to-end instrumentation test for {@link Transformer#getProgress}. */ @RunWith(AndroidJUnit4.class) -public class TransformerTrimOptimizationProgressTest { +public class TransformerProgressTest { + private static final long DELAY_MS = 50; + @Rule public final TestName testName = new TestName(); private final Context context = ApplicationProvider.getApplicationContext(); @@ -67,6 +79,94 @@ public class TransformerTrimOptimizationProgressTest { testId = testName.getMethodName(); } + /** + * Tests that {@link Transformer#getProgress(ProgressHolder)} returns monotonically increasing + * updates. The test runs a transformation of a {@link Composition} using a custom video effect + * that adds delay in the video processing pipeline, ensuring that the the transformation takes + * long enough for the test thread to collect at least two progress updates. + */ + @Test + @RequiresNonNull("testId") + @SuppressWarnings("PreferJavaTimeOverload") + public void getProgress_monotonicallyIncreasingUpdates() throws InterruptedException { + AtomicBoolean completed = new AtomicBoolean(); + AtomicReference exportResultRef = new AtomicReference<>(); + AtomicReference exportExceptionRef = new AtomicReference<>(); + Transformer.Listener listener = + new Transformer.Listener() { + @Override + public void onCompleted(Composition composition, ExportResult exportResult) { + exportResultRef.set(exportResult); + completed.set(true); + } + + @Override + public void onError( + Composition composition, ExportResult exportResult, ExportException exportException) { + exportExceptionRef.set(exportException); + exportResultRef.set(exportResult); + completed.set(true); + } + }; + Instrumentation instrumentation = InstrumentationRegistry.getInstrumentation(); + AtomicReference<@NullableType Transformer> transformerRef = new AtomicReference<>(); + AtomicReference<@NullableType Exception> exceptionRef = new AtomicReference<>(); + instrumentation.runOnMainSync( + () -> { + try { + // A composition with a custom effect that's slow: puts the video processing thread to + // sleep on every received frame. + Composition composition = + new Composition.Builder( + new EditedMediaItemSequence( + new EditedMediaItem.Builder( + MediaItem.fromUri(AndroidTestUtil.MP4_ASSET_URI_STRING)) + .setEffects( + new Effects( + /* audioProcessors= */ ImmutableList.of(), + /* videoEffects= */ ImmutableList.of( + new DelayEffect(/* delayMs= */ DELAY_MS)))) + .build())) + .build(); + File outputVideoFile = + AndroidTestUtil.createExternalCacheFile( + context, /* fileName= */ testId + "-output.mp4"); + Transformer transformer = new Transformer.Builder(context).build(); + transformer.addListener(listener); + transformer.start(composition, outputVideoFile.getPath()); + transformerRef.set(transformer); + } catch (Exception e) { + exceptionRef.set(e); + } + }); + + assertThat(exceptionRef.get()).isNull(); + + ArrayList progresses = new ArrayList<>(); + while (!completed.get()) { + instrumentation.runOnMainSync( + () -> { + Transformer transformer = checkStateNotNull(transformerRef.get()); + ProgressHolder progressHolder = new ProgressHolder(); + if (transformer.getProgress(progressHolder) == PROGRESS_STATE_AVAILABLE + && (progresses.isEmpty() + || Iterables.getLast(progresses) != progressHolder.progress)) { + progresses.add(progressHolder.progress); + } + }); + + Thread.sleep(DELAY_MS); + } + + assertThat(exportExceptionRef.get()).isNull(); + // Transformer.getProgress() should be able to retrieve at least 2 progress updates since the + // delay effect stalls the video processing for each video frame. + assertThat(progresses.size()).isAtLeast(2); + assertThat(progresses).isInOrder(); + assertThat(Iterables.getFirst(progresses, /* defaultValue= */ -1)).isAtLeast(0); + assertThat(Iterables.getLast(progresses)).isAtMost(100); + } + @Test @RequiresNonNull("testId") public void getProgress_trimOptimizationEnabledAndApplied_givesIncreasingPercentages() @@ -140,6 +240,7 @@ public class TransformerTrimOptimizationProgressTest { assertThat(transformerExceptionFuture.get()).isNull(); assertThat(progresses).isInOrder(); + // TODO - b/322145448 Make tests more deterministic and produce at least one progress output. if (!progresses.isEmpty()) { // The progress list could be empty if the export ends before any progress can be retrieved. assertThat(Iterables.getFirst(progresses, /* defaultValue= */ -1)).isAtLeast(0); @@ -241,4 +342,72 @@ public class TransformerTrimOptimizationProgressTest { assertThat(transformerExceptionFuture.get()).isNull(); assertThat(foundInconsistentState.get()).isFalse(); } + + /** A {@link GlEffect} that adds delay in the video pipeline by putting the thread to sleep. */ + private static final class DelayEffect implements GlEffect { + public final long delayMs; + + public DelayEffect(long delayMs) { + this.delayMs = delayMs; + } + + @Override + public GlShaderProgram toGlShaderProgram(Context context, boolean useHdr) + throws VideoFrameProcessingException { + // Wrapping Brightness's GlShaderProgram for convenience. All existing BaseGlShaderProgram + // implementations are final and can't be extended. + BaseGlShaderProgram brightnessShaderGlProgram = + new Brightness(1.0f).toGlShaderProgram(context, useHdr); + return new GlShaderProgram() { + @Override + public void setInputListener(InputListener inputListener) { + brightnessShaderGlProgram.setInputListener(inputListener); + } + + @Override + public void setOutputListener(OutputListener outputListener) { + brightnessShaderGlProgram.setOutputListener(outputListener); + } + + @Override + public void setErrorListener(Executor executor, ErrorListener errorListener) { + brightnessShaderGlProgram.setErrorListener(executor, errorListener); + } + + @Override + public void queueInputFrame( + GlObjectsProvider glObjectsProvider, + GlTextureInfo inputTexture, + long presentationTimeUs) { + try { + Thread.sleep(delayMs); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + brightnessShaderGlProgram.queueInputFrame( + glObjectsProvider, inputTexture, presentationTimeUs); + } + + @Override + public void releaseOutputFrame(GlTextureInfo outputTexture) { + brightnessShaderGlProgram.releaseOutputFrame(outputTexture); + } + + @Override + public void signalEndOfCurrentInputStream() { + brightnessShaderGlProgram.signalEndOfCurrentInputStream(); + } + + @Override + public void flush() { + brightnessShaderGlProgram.flush(); + } + + @Override + public void release() throws VideoFrameProcessingException { + brightnessShaderGlProgram.release(); + } + }; + } + } } diff --git a/libraries/transformer/src/main/java/androidx/media3/transformer/TransformerInternal.java b/libraries/transformer/src/main/java/androidx/media3/transformer/TransformerInternal.java index 69f566a56f..4c860f79fe 100644 --- a/libraries/transformer/src/main/java/androidx/media3/transformer/TransformerInternal.java +++ b/libraries/transformer/src/main/java/androidx/media3/transformer/TransformerInternal.java @@ -41,6 +41,7 @@ import android.util.Log; import android.util.SparseArray; import androidx.annotation.GuardedBy; import androidx.annotation.IntDef; +import androidx.annotation.IntRange; import androidx.annotation.Nullable; import androidx.media3.common.C; import androidx.media3.common.DebugViewProvider; @@ -100,7 +101,6 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; private static final int MSG_REGISTER_SAMPLE_EXPORTER = 1; private static final int MSG_DRAIN_EXPORTERS = 2; private static final int MSG_END = 3; - private static final int MSG_UPDATE_PROGRESS = 4; private static final String TAG = "TransformerInternal"; private static final int DRAIN_EXPORTERS_DELAY_MS = 10; @@ -129,8 +129,10 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; private final List sampleExporters; private final MuxerWrapper muxerWrapper; - private final ConditionVariable transformerConditionVariable; + private final ConditionVariable canceledConditionVariable; private final Object setMaxSequenceDurationUsLock; + private final Object progressLock; + private final ProgressHolder internalProgressHolder; private boolean isDrainingExporters; @@ -145,10 +147,19 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; /** * The current {@link Transformer.ProgressState}. * - *

Modified on the internal thread. Accessed on the application thread (in {@link - * #getProgress}). + *

Accessed and modified on the application and internal thread. */ - private volatile @Transformer.ProgressState int progressState; + @GuardedBy("progressLock") + private @Transformer.ProgressState int progressState; + + /** + * The current progress value, from 0 to 100. + * + *

Accessed and modified on the application and internal thread. + */ + @GuardedBy("progressLock") + @IntRange(from = 0, to = 100) + private int progressValue; /** * The boolean tracking if this component has been released. @@ -215,8 +226,11 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; compositionHasLoopingSequence = nonLoopingSequencesWithNonFinalDuration != composition.sequences.size(); setMaxSequenceDurationUsLock = new Object(); + canceledConditionVariable = new ConditionVariable(); + progressLock = new Object(); + internalProgressHolder = new ProgressHolder(); sampleExporters = new ArrayList<>(); - transformerConditionVariable = new ConditionVariable(); + // It's safe to use "this" because we don't send a message before exiting the constructor. @SuppressWarnings("nullness:methodref.receiver.bound") HandlerWrapper internalHandler = @@ -227,18 +241,23 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; public void start() { verifyInternalThreadAlive(); internalHandler.sendEmptyMessage(MSG_START); + synchronized (progressLock) { + progressState = Transformer.PROGRESS_STATE_WAITING_FOR_AVAILABILITY; + progressValue = 0; + } } public @Transformer.ProgressState int getProgress(ProgressHolder progressHolder) { if (released) { return PROGRESS_STATE_NOT_STARTED; } - verifyInternalThreadAlive(); - internalHandler.obtainMessage(MSG_UPDATE_PROGRESS, progressHolder).sendToTarget(); - // TODO: figure out why calling clock.onThreadBlocked() here makes the tests fail. - transformerConditionVariable.blockUninterruptible(); - transformerConditionVariable.close(); - return progressState; + + synchronized (progressLock) { + if (progressState == PROGRESS_STATE_AVAILABLE) { + progressHolder.progress = progressValue; + } + return progressState; + } } public void cancel() { @@ -250,8 +269,8 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; .obtainMessage(MSG_END, END_REASON_CANCELLED, /* unused */ 0, /* exportException */ null) .sendToTarget(); clock.onThreadBlocked(); - transformerConditionVariable.blockUninterruptible(); - transformerConditionVariable.close(); + canceledConditionVariable.blockUninterruptible(); + canceledConditionVariable.close(); if (cancelException != null) { throw cancelException; } @@ -282,7 +301,7 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; // handled to report release timeouts and to unblock the transformer condition variable in case // of cancellation. Progress update messages must be handled to unblock the transformer // condition variable. - if (released && msg.what != MSG_END && msg.what != MSG_UPDATE_PROGRESS) { + if (released && msg.what != MSG_END) { return true; } try { @@ -299,9 +318,6 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; case MSG_END: endInternal(/* endReason= */ msg.arg1, /* exportException= */ (ExportException) msg.obj); break; - case MSG_UPDATE_PROGRESS: - updateProgressInternal(/* progressHolder= */ (ProgressHolder) msg.obj); - break; default: return false; } @@ -332,6 +348,8 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; while (sampleExporters.get(i).processData()) {} } + updateProgressInternal(); + if (!muxerWrapper.isEnded()) { internalHandler.sendEmptyMessageDelayed(MSG_DRAIN_EXPORTERS, DRAIN_EXPORTERS_DELAY_MS); } @@ -349,6 +367,11 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; boolean releasedPreviously = released; if (!released) { released = true; + synchronized (progressLock) { + progressState = PROGRESS_STATE_NOT_STARTED; + progressValue = 0; + } + // VideoSampleExporter can hold buffers from the asset loader's decoder in a surface texture, // so we release the VideoSampleExporter first to avoid releasing the codec while its buffers // are pending processing. @@ -391,12 +414,8 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; internalHandler.post(internalHandlerThread::quitSafely); } - // Update progress before opening variable to avoid getProgress returning an invalid combination - // of state and progress. - progressState = PROGRESS_STATE_NOT_STARTED; - transformerConditionVariable.open(); - if (forCancellation) { + canceledConditionVariable.open(); return; } @@ -437,25 +456,37 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; } } - private void updateProgressInternal(ProgressHolder progressHolder) { + private void updateProgressInternal() { + if (released) { + return; + } + int progressSum = 0; int progressCount = 0; - ProgressHolder individualProgressHolder = new ProgressHolder(); for (int i = 0; i < sequenceAssetLoaders.size(); i++) { if (composition.sequences.get(i).isLooping) { // Looping sequence progress is always unavailable. Skip it. continue; } - progressState = sequenceAssetLoaders.get(i).getProgress(individualProgressHolder); - if (progressState != PROGRESS_STATE_AVAILABLE) { - transformerConditionVariable.open(); + internalProgressHolder.progress = 0; + @Transformer.ProgressState + int assetLoaderProgressState = + sequenceAssetLoaders.get(i).getProgress(internalProgressHolder); + if (assetLoaderProgressState != PROGRESS_STATE_AVAILABLE) { + // TODO - b/322136131 : Check for inconsistent state transitions. + synchronized (progressLock) { + progressState = assetLoaderProgressState; + progressValue = 0; + } return; } - progressSum += individualProgressHolder.progress; + progressSum += internalProgressHolder.progress; progressCount++; } - progressHolder.progress = progressSum / progressCount; - transformerConditionVariable.open(); + synchronized (progressLock) { + progressState = PROGRESS_STATE_AVAILABLE; + progressValue = progressSum / progressCount; + } } private final class SequenceAssetLoaderListener implements AssetLoader.Listener {