Rollback of e364510937

PiperOrigin-RevId: 601187997

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}. */
- * End-to-end instrumentation test for {@link Transformer#getProgress} when {@link
- * Transformer.Builder#experimentalSetTrimOptimizationEnabled} is enabled.
- */
 @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<ExportResult> exportResultRef = new AtomicReference<>();
+    AtomicReference<ExportException> 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<Integer> 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();
+        }
+      };
+    }
+  }
 }

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<SampleExporter> 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}.
    *
-   * <p>Modified on the internal thread. Accessed on the application thread (in {@link
+   * <p>Accessed and modified on the application and internal thread.
-   * #getProgress}).
    */
-  private volatile @Transformer.ProgressState int progressState;
+  @GuardedBy("progressLock")
+  private @Transformer.ProgressState int progressState;
+
+  /**
+   * The current progress value, from 0 to 100.
+   *
+   * <p>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();
+    synchronized (progressLock) {
-    // TODO: figure out why calling clock.onThreadBlocked() here makes the tests fail.
+      if (progressState == PROGRESS_STATE_AVAILABLE) {
-    transformerConditionVariable.blockUninterruptible();
+        progressHolder.progress = progressValue;
-    transformerConditionVariable.close();
+      }
-    return progressState;
+      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();
+    canceledConditionVariable.blockUninterruptible();
-    transformerConditionVariable.close();
+    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);
+      internalProgressHolder.progress = 0;
-      if (progressState != PROGRESS_STATE_AVAILABLE) {
+      @Transformer.ProgressState
-        transformerConditionVariable.open();
+      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;
+    synchronized (progressLock) {
-    transformerConditionVariable.open();
+      progressState = PROGRESS_STATE_AVAILABLE;
+      progressValue = progressSum / progressCount;
+    }
   }
 
   private final class SequenceAssetLoaderListener implements AssetLoader.Listener {