From bf7c97a66288908c3d0dccb1f402364857da0468 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 022c2864895..9c01fead162 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -48,13 +48,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 00000000000..b7f05b08695 --- /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 4814686c51d..3da8409de1e 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.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; @@ -1940,7 +1941,12 @@ private void maybeUpdateReadingPeriod() { 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); } } } @@ -1965,7 +1971,8 @@ private void maybeUpdateReadingPeriod() { && 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++) { @@ -1981,7 +1988,9 @@ private void maybeUpdateReadingPeriod() { // 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()); } } } @@ -2106,14 +2115,21 @@ private boolean hasReadingPeriodFinishedReading() { 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 e1c9e7cebe4..cbfd43775f7 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 TextRenderer( outputLooper == null ? null : Util.createHandler(outputLooper, /* callback= */ this); this.decoderFactory = decoderFactory; formatHolder = new FormatHolder(); + finalStreamEndPositionUs = C.TIME_UNSET; } @Override @@ -141,6 +144,21 @@ public int supportsFormat(Format format) { } } + /** + * 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 @@ protected void onPositionReset(long positionUs, boolean joining) { clearOutput(); inputStreamEnded = false; outputStreamEnded = false; + finalStreamEndPositionUs = C.TIME_UNSET; if (decoderReplacementState != REPLACEMENT_STATE_NONE) { replaceDecoder(); } else { @@ -166,6 +185,13 @@ protected void onPositionReset(long positionUs, boolean joining) { @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 void render(long positionUs, long elapsedRealtimeUs) { @Override protected void onDisabled() { streamFormat = null; + finalStreamEndPositionUs = C.TIME_UNSET; clearOutput(); releaseDecoder(); }