From fc07ce056ae2f6c036faa622fae29b8cf0f4babd Mon Sep 17 00:00:00 2001 From: samrobinson Date: Tue, 24 Sep 2024 09:20:02 -0700 Subject: [PATCH] Add safer gap based checks to Transformer API boundary points. PiperOrigin-RevId: 678278666 --- .../media3/transformer/CompositionPlayer.java | 1 + .../media3/transformer/EditedMediaItem.java | 8 ++ .../transformer/TransformerInternal.java | 8 ++ .../media3/transformer/TransformerUtil.java | 3 + .../VideoFrameProcessingWrapper.java | 2 + .../transformer/SequenceExportTest.java | 90 +++++++++++++++++++ 6 files changed, 112 insertions(+) diff --git a/libraries/transformer/src/main/java/androidx/media3/transformer/CompositionPlayer.java b/libraries/transformer/src/main/java/androidx/media3/transformer/CompositionPlayer.java index 50a2a30660..afc8f11ee4 100644 --- a/libraries/transformer/src/main/java/androidx/media3/transformer/CompositionPlayer.java +++ b/libraries/transformer/src/main/java/androidx/media3/transformer/CompositionPlayer.java @@ -337,6 +337,7 @@ public final class CompositionPlayer extends SimpleBasePlayer public void setComposition(Composition composition) { verifyApplicationThread(); checkArgument(!composition.sequences.isEmpty()); + checkArgument(!composition.hasGaps()); checkState(this.composition == null); composition = deactivateSpeedAdjustingVideoEffects(composition); diff --git a/libraries/transformer/src/main/java/androidx/media3/transformer/EditedMediaItem.java b/libraries/transformer/src/main/java/androidx/media3/transformer/EditedMediaItem.java index f75846def5..dbd584af46 100644 --- a/libraries/transformer/src/main/java/androidx/media3/transformer/EditedMediaItem.java +++ b/libraries/transformer/src/main/java/androidx/media3/transformer/EditedMediaItem.java @@ -287,6 +287,10 @@ public final class EditedMediaItem { int frameRate, Effects effects) { checkState(!removeAudio || !removeVideo, "Audio and video cannot both be removed"); + if (isGap(mediaItem)) { + checkArgument(durationUs != C.TIME_UNSET); + checkArgument(!removeAudio && !flattenForSlowMotion && effects.audioProcessors.isEmpty()); + } this.mediaItem = mediaItem; this.removeAudio = removeAudio; this.removeVideo = removeVideo; @@ -349,6 +353,10 @@ public final class EditedMediaItem { * EditedMediaItemSequence.Builder#addGap(long) gap}. */ /* package */ boolean isGap() { + return isGap(mediaItem); + } + + private static boolean isGap(MediaItem mediaItem) { return Objects.equals(mediaItem.mediaId, GAP_MEDIA_ID); } } 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 244c3bc3e7..c66da422da 100644 --- a/libraries/transformer/src/main/java/androidx/media3/transformer/TransformerInternal.java +++ b/libraries/transformer/src/main/java/androidx/media3/transformer/TransformerInternal.java @@ -605,6 +605,11 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; @AssetLoader.SupportedOutputTypes int supportedOutputTypes) { @C.TrackType int trackType = getProcessedTrackType(firstAssetLoaderInputFormat.sampleMimeType); + + checkArgument( + trackType != TRACK_TYPE_VIDEO || !composition.sequences.get(sequenceIndex).hasGaps(), + "Gaps in video sequences are not supported."); + synchronized (assetLoaderLock) { assetLoaderInputTracker.registerTrack(sequenceIndex, firstAssetLoaderInputFormat); if (assetLoaderInputTracker.hasRegisteredAllTracks()) { @@ -749,6 +754,9 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; @GuardedBy("assetLoaderLock") private void createEncodedSampleExporter(@C.TrackType int trackType) { checkState(assetLoaderInputTracker.getSampleExporter(trackType) == null); + checkArgument( + trackType != TRACK_TYPE_AUDIO || !composition.sequences.get(sequenceIndex).hasGaps(), + "Gaps can not be transmuxed."); assetLoaderInputTracker.registerSampleExporter( trackType, new EncodedSampleExporter( diff --git a/libraries/transformer/src/main/java/androidx/media3/transformer/TransformerUtil.java b/libraries/transformer/src/main/java/androidx/media3/transformer/TransformerUtil.java index ec7c5b9247..641541ddfd 100644 --- a/libraries/transformer/src/main/java/androidx/media3/transformer/TransformerUtil.java +++ b/libraries/transformer/src/main/java/androidx/media3/transformer/TransformerUtil.java @@ -18,6 +18,7 @@ package androidx.media3.transformer; import static androidx.media3.common.ColorInfo.SDR_BT709_LIMITED; import static androidx.media3.common.ColorInfo.isTransferHdr; +import static androidx.media3.common.util.Assertions.checkArgument; import static androidx.media3.transformer.Composition.HDR_MODE_KEEP_HDR; import static androidx.media3.transformer.Composition.HDR_MODE_TONE_MAP_HDR_TO_SDR_USING_OPEN_GL; import static androidx.media3.transformer.EncoderUtil.getSupportedEncodersForHdrEditing; @@ -81,6 +82,8 @@ public final class TransformerUtil { MuxerWrapper muxerWrapper) { if (composition.sequences.size() > 1 || composition.sequences.get(sequenceIndex).editedMediaItems.size() > 1) { + checkArgument( + !composition.hasGaps() || !composition.transmuxAudio, "Gaps can not be transmuxed."); return !composition.transmuxAudio; } if (composition.hasGaps()) { diff --git a/libraries/transformer/src/main/java/androidx/media3/transformer/VideoFrameProcessingWrapper.java b/libraries/transformer/src/main/java/androidx/media3/transformer/VideoFrameProcessingWrapper.java index 2832bb2ff9..b3a8b26f77 100644 --- a/libraries/transformer/src/main/java/androidx/media3/transformer/VideoFrameProcessingWrapper.java +++ b/libraries/transformer/src/main/java/androidx/media3/transformer/VideoFrameProcessingWrapper.java @@ -19,6 +19,7 @@ package androidx.media3.transformer; import static androidx.media3.common.VideoFrameProcessor.INPUT_TYPE_BITMAP; import static androidx.media3.common.VideoFrameProcessor.INPUT_TYPE_SURFACE; import static androidx.media3.common.VideoFrameProcessor.INPUT_TYPE_TEXTURE_ID; +import static androidx.media3.common.util.Assertions.checkArgument; import static androidx.media3.common.util.Assertions.checkNotNull; import android.graphics.Bitmap; @@ -60,6 +61,7 @@ import java.util.concurrent.atomic.AtomicLong; long durationUs, @Nullable Format decodedFormat, boolean isLast) { + checkArgument(!editedMediaItem.isGap()); boolean isSurfaceAssetLoaderMediaItem = isMediaItemForSurfaceAssetLoader(editedMediaItem); durationUs = editedMediaItem.getDurationAfterEffectsApplied(durationUs); if (decodedFormat != null) { diff --git a/libraries/transformer/src/test/java/androidx/media3/transformer/SequenceExportTest.java b/libraries/transformer/src/test/java/androidx/media3/transformer/SequenceExportTest.java index 120b87c176..c91f2e51d4 100644 --- a/libraries/transformer/src/test/java/androidx/media3/transformer/SequenceExportTest.java +++ b/libraries/transformer/src/test/java/androidx/media3/transformer/SequenceExportTest.java @@ -32,8 +32,10 @@ import static androidx.media3.transformer.TestUtil.getDumpFileName; import static androidx.media3.transformer.TestUtil.getSequenceDumpFilePath; import static androidx.media3.transformer.TestUtil.removeEncodersAndDecoders; import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; import android.content.Context; +import androidx.annotation.Nullable; import androidx.media3.common.MediaItem; import androidx.media3.common.MimeTypes; import androidx.media3.common.audio.SonicAudioProcessor; @@ -44,6 +46,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.common.collect.ImmutableList; import org.junit.After; import org.junit.Before; +import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; @@ -499,6 +502,81 @@ public final class SequenceExportTest { "silenceHighPitch")); } + @Test + public void transmuxAudio_itemGap_throws() throws Exception { + Transformer transformer = + createTransformerBuilder(new DefaultMuxer.Factory(), /* enableFallback= */ false).build(); + EditedMediaItem audioItem = + new EditedMediaItem.Builder(MediaItem.fromUri(ASSET_URI_PREFIX + FILE_AUDIO_RAW)).build(); + EditedMediaItemSequence sequence = + new EditedMediaItemSequence.Builder().addItem(audioItem).addGap(500_000).build(); + Composition composition = new Composition.Builder(sequence).setTransmuxAudio(true).build(); + + transformer.start(composition, outputDir.newFile().getPath()); + + ExportException exception = + assertThrows(ExportException.class, () -> TransformerTestRunner.runLooper(transformer)); + assertThat(getRootCause(exception)).hasMessageThat().isEqualTo("Gaps can not be transmuxed."); + } + + // TODO - b/369154363: Enable test after shouldTranscode inconsistency is resolved. + @Ignore + @Test + public void transmuxAudio_gapItem_throws() throws Exception { + Transformer transformer = + createTransformerBuilder(new DefaultMuxer.Factory(), /* enableFallback= */ false).build(); + EditedMediaItem audioItem = + new EditedMediaItem.Builder(MediaItem.fromUri(ASSET_URI_PREFIX + FILE_AUDIO_RAW)) + .setRemoveVideo(true) + .build(); + EditedMediaItemSequence sequence = + new EditedMediaItemSequence.Builder().addGap(500_000).addItem(audioItem).build(); + Composition composition = new Composition.Builder(sequence).setTransmuxAudio(true).build(); + + transformer.start(composition, outputDir.newFile().getPath()); + + ExportException exception = + assertThrows(ExportException.class, () -> TransformerTestRunner.runLooper(transformer)); + assertThat(getRootCause(exception)).hasMessageThat().isEqualTo("Gaps can not be transmuxed."); + } + + @Test + public void start_videoGap_throws() throws Exception { + Transformer transformer = + createTransformerBuilder(new DefaultMuxer.Factory(), /* enableFallback= */ false).build(); + EditedMediaItem audioVideoItem = + new EditedMediaItem.Builder(MediaItem.fromUri(ASSET_URI_PREFIX + FILE_AUDIO_RAW_VIDEO)) + .build(); + EditedMediaItemSequence sequence = + new EditedMediaItemSequence.Builder().addItem(audioVideoItem).addGap(500_000).build(); + + transformer.start(new Composition.Builder(sequence).build(), outputDir.newFile().getPath()); + + ExportException exception = + assertThrows(ExportException.class, () -> TransformerTestRunner.runLooper(transformer)); + assertThat(getRootCause(exception)) + .hasMessageThat() + .isEqualTo("Gaps in video sequences are not supported."); + } + + @Test + public void start_gapVideo_throws() throws Exception { + Transformer transformer = + createTransformerBuilder(new DefaultMuxer.Factory(), /* enableFallback= */ false).build(); + EditedMediaItem audioVideoItem = + new EditedMediaItem.Builder(MediaItem.fromUri(ASSET_URI_PREFIX + FILE_AUDIO_RAW_VIDEO)) + .build(); + EditedMediaItemSequence sequence = + new EditedMediaItemSequence.Builder().addGap(500_000).addItem(audioVideoItem).build(); + Composition composition = new Composition.Builder(sequence).build(); + + transformer.start(composition, outputDir.newFile().getPath()); + + // Transformer throws because the first item in the sequence (the gap) does not have a video + // track. + assertThrows(ExportException.class, () -> TransformerTestRunner.runLooper(transformer)); + } + @Test public void start_gapGap_completesSuccessfully() throws Exception { CapturingMuxer.Factory muxerFactory = new CapturingMuxer.Factory(/* handleAudioAsPcm= */ true); @@ -915,4 +993,16 @@ public final class SequenceExportTest { assertThat(exportResult.sampleRate).isEqualTo(48_000); assertThat(exportResult.channelCount).isEqualTo(2); } + + private Throwable getRootCause(Throwable throwable) { + @Nullable Throwable node = throwable; + @Nullable Throwable nodeCause; + do { + nodeCause = node.getCause(); + if (nodeCause != null) { + node = nodeCause; + } + } while (nodeCause != null); + return node; + } }