From ff6537d69b8644f664b8dae05680f28765821ff3 Mon Sep 17 00:00:00 2001 From: claincly Date: Mon, 31 Mar 2025 04:20:45 -0700 Subject: [PATCH] Fix a bug that video effects are added twice The tests passed because of an issue in chaining. The chaining listener allows self-looping, i.e. the producer and the consumer of a frame could be the same instance. Like an effect chain of `a -> a -> b` This didn't fail any test before, because the chaining is rectified when connecting a to b, but it should have failed when connecting a to a. PiperOrigin-RevId: 742215700 --- .../ChainingGlShaderProgramListener.java | 5 +++++ .../video/PlaybackVideoGraphWrapper.java | 21 ++----------------- 2 files changed, 7 insertions(+), 19 deletions(-) diff --git a/libraries/effect/src/main/java/androidx/media3/effect/ChainingGlShaderProgramListener.java b/libraries/effect/src/main/java/androidx/media3/effect/ChainingGlShaderProgramListener.java index 8fdd7d7edf..8d84215dc3 100644 --- a/libraries/effect/src/main/java/androidx/media3/effect/ChainingGlShaderProgramListener.java +++ b/libraries/effect/src/main/java/androidx/media3/effect/ChainingGlShaderProgramListener.java @@ -15,6 +15,8 @@ */ package androidx.media3.effect; +import static androidx.media3.common.util.Assertions.checkArgument; + import androidx.media3.common.GlObjectsProvider; import androidx.media3.common.GlTextureInfo; import androidx.media3.effect.GlShaderProgram.InputListener; @@ -51,6 +53,9 @@ import androidx.media3.effect.GlShaderProgram.OutputListener; GlShaderProgram producingGlShaderProgram, GlShaderProgram consumingGlShaderProgram, VideoFrameProcessingTaskExecutor videoFrameProcessingTaskExecutor) { + checkArgument( + producingGlShaderProgram != consumingGlShaderProgram, + "Creating a self loop in the chain: " + producingGlShaderProgram); this.producingGlShaderProgram = producingGlShaderProgram; frameConsumptionManager = new FrameConsumptionManager( diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/video/PlaybackVideoGraphWrapper.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/video/PlaybackVideoGraphWrapper.java index b4f375b7fc..5d5e784164 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/video/PlaybackVideoGraphWrapper.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/video/PlaybackVideoGraphWrapper.java @@ -782,7 +782,7 @@ public final class PlaybackVideoGraphWrapper implements VideoSinkProvider, Video @FirstFrameReleaseInstruction int firstFrameReleaseInstruction, List videoEffects) { checkState(isInitialized()); - setPendingVideoEffects(videoEffects); + this.videoEffects = ImmutableList.copyOf(videoEffects); this.inputType = inputType; this.inputFormat = format; finalBufferPresentationTimeUs = C.TIME_UNSET; @@ -863,7 +863,7 @@ public final class PlaybackVideoGraphWrapper implements VideoSinkProvider, Video if (this.videoEffects.equals(videoEffects)) { return; } - setPendingVideoEffects(videoEffects); + this.videoEffects = ImmutableList.copyOf(videoEffects); if (inputFormat != null) { registerInputStream(inputFormat); } @@ -999,23 +999,6 @@ public final class PlaybackVideoGraphWrapper implements VideoSinkProvider, Video // Private methods - /** - * Sets the pending video effects. - * - *

Effects are pending until a new input stream is registered. - */ - private void setPendingVideoEffects(List newVideoEffects) { - if (videoGraphFactory.supportsMultipleInputs()) { - this.videoEffects = ImmutableList.copyOf(newVideoEffects); - } else { - this.videoEffects = - new ImmutableList.Builder() - .addAll(newVideoEffects) - .addAll(compositionEffects) - .build(); - } - } - private void registerInputStream(Format inputFormat) { Format adjustedInputFormat = inputFormat