From ff8db9a4fe0db2f646fb8b06ae4d39597b693e2b Mon Sep 17 00:00:00 2001 From: ibaker Date: Tue, 9 Mar 2021 14:37:36 +0000 Subject: [PATCH] Pass the end position of a stream to TextRenderer when marking it final In a period transition we pass the start time of the next period, for the final period we pass the duration of the period or timeline, if known. This means sideloaded subtitles now respect the end point of ClippingMediaSource and ensures that content subtitles aren't incorrectly displayed over mid-roll ads. When transitioning back into the subtitled content the subtitles still appear slightly before the video transitions, meaning the first subtitle of the content is shown with the last few frames of the ad. Resolving this in a way that doesn't break anything else requires a deeper investigation. Issue: #5317 Issue: #8456 #minor-release PiperOrigin-RevId: 361797118 --- RELEASENOTES.md | 13 +- .../exoplayer2/ClippedPlaybackTest.java | 155 ++++++++++++++++++ .../exoplayer2/ExoPlayerImplInternal.java | 26 ++- .../android/exoplayer2/text/TextRenderer.java | 27 +++ 4 files changed, 213 insertions(+), 8 deletions(-) create mode 100644 library/core/src/androidTest/java/com/google/android/exoplayer2/ClippedPlaybackTest.java diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 032ecb29a0..adfd097277 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -49,13 +49,20 @@ * Text * Parse SSA/ASS bold & italic info in `Style:` lines ([#8435](https://github.com/google/ExoPlayer/issues/8435)). +* Text: + * Don't display subtitles after the end position of the current media + period (if known). This ensures sideloaded subtitles respect the end + point of `ClippingMediaPeriod` and prevents content subtitles from + continuing to be displayed over mid-roll ads + ([#5317](https://github.com/google/ExoPlayer/issues/5317), + [#8456](https://github.com/google/ExoPlayer/issues/8456)). * MediaSession extension: Remove dependency to core module and rely on common only. The `TimelineQueueEditor` uses a new `MediaDescriptionConverter` for this purpose and does not rely on the `ConcatenatingMediaSource` anymore. * Cast extension: - * Fix `onPositionDiscontinuity` event so that it is not triggered with - reason `DISCONTINUITY_REASON_PERIOD_TRANSITION` after a seek to another - media item and so that it is not triggered after a timeline change. + * Fix `onPositionDiscontinuity` event so that it is not triggered with + reason `DISCONTINUITY_REASON_PERIOD_TRANSITION` after a seek to another + media item and so that it is not triggered after a timeline change. ### 2.13.2 (2021-02-25) diff --git a/library/core/src/androidTest/java/com/google/android/exoplayer2/ClippedPlaybackTest.java b/library/core/src/androidTest/java/com/google/android/exoplayer2/ClippedPlaybackTest.java new file mode 100644 index 0000000000..b7f05b0869 --- /dev/null +++ b/library/core/src/androidTest/java/com/google/android/exoplayer2/ClippedPlaybackTest.java @@ -0,0 +1,155 @@ +/* + * 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; + +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static com.google.common.truth.Truth.assertThat; + +import android.net.Uri; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.google.android.exoplayer2.source.ClippingMediaSource; +import com.google.android.exoplayer2.text.Cue; +import com.google.android.exoplayer2.text.TextOutput; +import com.google.android.exoplayer2.util.ConditionVariable; +import com.google.android.exoplayer2.util.MimeTypes; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** + * Instrumentation tests for playback of clipped items using {@link MediaItem#clippingProperties} or + * {@link ClippingMediaSource} directly. + */ +@RunWith(AndroidJUnit4.class) +public final class ClippedPlaybackTest { + + @Test + public void subtitlesRespectClipping_singlePeriod() throws Exception { + MediaItem mediaItem = + new MediaItem.Builder() + .setUri("asset:///media/mp4/sample.mp4") + .setSubtitles( + ImmutableList.of( + new MediaItem.Subtitle( + Uri.parse("asset:///media/webvtt/typical"), + MimeTypes.TEXT_VTT, + "en", + C.SELECTION_FLAG_DEFAULT))) + // Expect the clipping to affect both subtitles and video. + .setClipEndPositionMs(1000) + .build(); + AtomicReference player = new AtomicReference<>(); + CapturingTextOutput textOutput = new CapturingTextOutput(); + ConditionVariable playbackEnded = new ConditionVariable(); + getInstrumentation() + .runOnMainSync( + () -> { + player.set(new SimpleExoPlayer.Builder(getInstrumentation().getContext()).build()); + player.get().addTextOutput(textOutput); + player + .get() + .addListener( + new Player.EventListener() { + @Override + public void onPlaybackStateChanged(@Player.State int state) { + if (state == Player.STATE_ENDED) { + playbackEnded.open(); + } + } + }); + player.get().setMediaItem(mediaItem); + player.get().prepare(); + player.get().play(); + }); + + playbackEnded.block(); + + getInstrumentation().runOnMainSync(() -> player.get().release()); + assertThat(Iterables.getOnlyElement(Iterables.concat(textOutput.cues)).text.toString()) + .isEqualTo("This is the first subtitle."); + } + + @Test + public void subtitlesRespectClipping_multiplePeriods() throws Exception { + ImmutableList mediaItems = + ImmutableList.of( + new MediaItem.Builder() + .setUri("asset:///media/mp4/sample.mp4") + .setSubtitles( + ImmutableList.of( + new MediaItem.Subtitle( + Uri.parse("asset:///media/webvtt/typical"), + MimeTypes.TEXT_VTT, + "en", + C.SELECTION_FLAG_DEFAULT))) + // Expect the clipping to affect both subtitles and video. + .setClipEndPositionMs(1000) + .build(), + new MediaItem.Builder() + .setUri("asset:///media/mp4/sample.mp4") + // Not needed for correctness, just makes test run faster. Must be longer than the + // subtitle content (3.5s). + .setClipEndPositionMs(4_000) + .build()); + AtomicReference player = new AtomicReference<>(); + CapturingTextOutput textOutput = new CapturingTextOutput(); + ConditionVariable playbackEnded = new ConditionVariable(); + getInstrumentation() + .runOnMainSync( + () -> { + player.set(new SimpleExoPlayer.Builder(getInstrumentation().getContext()).build()); + player.get().addTextOutput(textOutput); + player + .get() + .addListener( + new Player.EventListener() { + @Override + public void onPlaybackStateChanged(@Player.State int state) { + if (state == Player.STATE_ENDED) { + playbackEnded.open(); + } + } + }); + player.get().setMediaItems(mediaItems); + player.get().prepare(); + player.get().play(); + }); + + playbackEnded.block(); + + getInstrumentation().runOnMainSync(() -> player.get().release()); + assertThat(Iterables.getOnlyElement(Iterables.concat(textOutput.cues)).text.toString()) + .isEqualTo("This is the first subtitle."); + } + + private static class CapturingTextOutput implements TextOutput { + + private final List> cues; + + private CapturingTextOutput() { + cues = new ArrayList<>(); + } + + @Override + public void onCues(List cues) { + this.cues.add(cues); + } + } +} diff --git a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java index f6da219717..e6c287ba58 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java @@ -40,6 +40,7 @@ import com.google.android.exoplayer2.source.MediaSource.MediaPeriodId; import com.google.android.exoplayer2.source.SampleStream; import com.google.android.exoplayer2.source.ShuffleOrder; import com.google.android.exoplayer2.source.TrackGroupArray; +import com.google.android.exoplayer2.text.TextRenderer; import com.google.android.exoplayer2.trackselection.ExoTrackSelection; import com.google.android.exoplayer2.trackselection.TrackSelector; import com.google.android.exoplayer2.trackselection.TrackSelectorResult; @@ -1932,7 +1933,12 @@ import java.util.concurrent.atomic.AtomicBoolean; if (sampleStream != null && renderer.getStream() == sampleStream && renderer.hasReadStreamToEnd()) { - renderer.setCurrentStreamFinal(); + long streamEndPositionUs = + readingPeriodHolder.info.durationUs != C.TIME_UNSET + && readingPeriodHolder.info.durationUs != C.TIME_END_OF_SOURCE + ? readingPeriodHolder.getRendererOffset() + readingPeriodHolder.info.durationUs + : C.TIME_UNSET; + setCurrentStreamFinal(renderer, streamEndPositionUs); } } } @@ -1957,7 +1963,8 @@ import java.util.concurrent.atomic.AtomicBoolean; && readingPeriodHolder.mediaPeriod.readDiscontinuity() != C.TIME_UNSET) { // The new period starts with a discontinuity, so the renderers will play out all data, then // be disabled and re-enabled when they start playing the next period. - setAllRendererStreamsFinal(); + setAllRendererStreamsFinal( + /* streamEndPositionUs= */ readingPeriodHolder.getStartPositionRendererTime()); return; } for (int i = 0; i < renderers.length; i++) { @@ -1973,7 +1980,9 @@ import java.util.concurrent.atomic.AtomicBoolean; // it's a no-sample renderer for which rendererOffsetUs should be updated only when // starting to play the next period. Mark the SampleStream as final to play out any // remaining data. - renderers[i].setCurrentStreamFinal(); + setCurrentStreamFinal( + renderers[i], + /* streamEndPositionUs= */ readingPeriodHolder.getStartPositionRendererTime()); } } } @@ -2098,14 +2107,21 @@ import java.util.concurrent.atomic.AtomicBoolean; return true; } - private void setAllRendererStreamsFinal() { + private void setAllRendererStreamsFinal(long streamEndPositionUs) { for (Renderer renderer : renderers) { if (renderer.getStream() != null) { - renderer.setCurrentStreamFinal(); + setCurrentStreamFinal(renderer, streamEndPositionUs); } } } + private void setCurrentStreamFinal(Renderer renderer, long streamEndPositionUs) { + renderer.setCurrentStreamFinal(); + if (renderer instanceof TextRenderer) { + ((TextRenderer) renderer).setFinalStreamEndPositionUs(streamEndPositionUs); + } + } + private void handlePeriodPrepared(MediaPeriod mediaPeriod) throws ExoPlaybackException { if (!queue.isLoading(mediaPeriod)) { // Stale event. diff --git a/library/core/src/main/java/com/google/android/exoplayer2/text/TextRenderer.java b/library/core/src/main/java/com/google/android/exoplayer2/text/TextRenderer.java index e1c9e7cebe..cbfd43775f 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/text/TextRenderer.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/text/TextRenderer.java @@ -16,6 +16,7 @@ package com.google.android.exoplayer2.text; import static com.google.android.exoplayer2.util.Assertions.checkNotNull; +import static com.google.android.exoplayer2.util.Assertions.checkState; import android.os.Handler; import android.os.Handler.Callback; @@ -91,6 +92,7 @@ public final class TextRenderer extends BaseRenderer implements Callback { @Nullable private SubtitleOutputBuffer subtitle; @Nullable private SubtitleOutputBuffer nextSubtitle; private int nextSubtitleEventIndex; + private long finalStreamEndPositionUs; /** * @param output The output. @@ -121,6 +123,7 @@ public final class TextRenderer extends BaseRenderer implements Callback { outputLooper == null ? null : Util.createHandler(outputLooper, /* callback= */ this); this.decoderFactory = decoderFactory; formatHolder = new FormatHolder(); + finalStreamEndPositionUs = C.TIME_UNSET; } @Override @@ -141,6 +144,21 @@ public final class TextRenderer extends BaseRenderer implements Callback { } } + /** + * Sets the position at which to stop rendering the current stream. + * + *

Must be called after {@link #setCurrentStreamFinal()}. + * + * @param streamEndPositionUs The position to stop rendering at or {@link C#LENGTH_UNSET} to + * render until the end of the current stream. + */ + // TODO(internal b/181312195): Remove this when it's no longer needed once subtitles are decoded + // on the loading side of SampleQueue. + public void setFinalStreamEndPositionUs(long streamEndPositionUs) { + checkState(isCurrentStreamFinal()); + this.finalStreamEndPositionUs = streamEndPositionUs; + } + @Override protected void onStreamChanged(Format[] formats, long startPositionUs, long offsetUs) { streamFormat = formats[0]; @@ -156,6 +174,7 @@ public final class TextRenderer extends BaseRenderer implements Callback { clearOutput(); inputStreamEnded = false; outputStreamEnded = false; + finalStreamEndPositionUs = C.TIME_UNSET; if (decoderReplacementState != REPLACEMENT_STATE_NONE) { replaceDecoder(); } else { @@ -166,6 +185,13 @@ public final class TextRenderer extends BaseRenderer implements Callback { @Override public void render(long positionUs, long elapsedRealtimeUs) { + if (isCurrentStreamFinal() + && finalStreamEndPositionUs != C.TIME_UNSET + && positionUs >= finalStreamEndPositionUs) { + releaseBuffers(); + outputStreamEnded = true; + } + if (outputStreamEnded) { return; } @@ -278,6 +304,7 @@ public final class TextRenderer extends BaseRenderer implements Callback { @Override protected void onDisabled() { streamFormat = null; + finalStreamEndPositionUs = C.TIME_UNSET; clearOutput(); releaseDecoder(); }