Commit f1508565 by olly Committed by Oliver Woodman

Fix masking step 1

1. Move Timeline/Manifest into PlaybackInfo
2. Don't update externally visible Timeline/Manifest during preparation
3. Ignore MSG_POSITION_DISCONTINUITY during preparation
4. Correctly set masking variables at start of preparation, and use them

Once this change goes in, PlaybackInfo will contain timeline, manifest
and position, which should always be self-consistent with one another.
The next step would then be to move a bunch of logic in ExoPlayerImpl
that derives state from timeline and position into PlaybackInfo, and
split that into its own top level class that can be easily tested to make
sure it never IndexOutOfBounds.

I think we could also replace the masking variables and instead just assign
a new PlaybackInfo to the playbackInfo variable whenever we're doing
something that requires masking. This should be possible because we no
longer update playbackInfo whenever we have pending acks. It would
require allowing PlaybackInfo to mask the window position internally when
the timeline is empty, but I think this is ok, and again is something we
could test pretty easily.

Issue: #3362

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=173909791
parent 9b9a294f
...@@ -56,7 +56,7 @@ public final class ExoPlayerTest extends TestCase { ...@@ -56,7 +56,7 @@ public final class ExoPlayerTest extends TestCase {
.setTimeline(timeline).setRenderers(renderer) .setTimeline(timeline).setRenderers(renderer)
.build().start().blockUntilEnded(TIMEOUT_MS); .build().start().blockUntilEnded(TIMEOUT_MS);
testRunner.assertPositionDiscontinuityCount(0); testRunner.assertPositionDiscontinuityCount(0);
testRunner.assertTimelinesEqual(timeline); testRunner.assertTimelinesEqual();
assertEquals(0, renderer.formatReadCount); assertEquals(0, renderer.formatReadCount);
assertEquals(0, renderer.bufferReadCount); assertEquals(0, renderer.bufferReadCount);
assertFalse(renderer.isEnded); assertFalse(renderer.isEnded);
......
...@@ -22,7 +22,6 @@ import android.os.Message; ...@@ -22,7 +22,6 @@ import android.os.Message;
import android.support.annotation.Nullable; import android.support.annotation.Nullable;
import android.util.Log; import android.util.Log;
import com.google.android.exoplayer2.ExoPlayerImplInternal.PlaybackInfo; import com.google.android.exoplayer2.ExoPlayerImplInternal.PlaybackInfo;
import com.google.android.exoplayer2.ExoPlayerImplInternal.SourceInfo;
import com.google.android.exoplayer2.source.MediaSource; import com.google.android.exoplayer2.source.MediaSource;
import com.google.android.exoplayer2.source.MediaSource.MediaPeriodId; import com.google.android.exoplayer2.source.MediaSource.MediaPeriodId;
import com.google.android.exoplayer2.source.TrackGroupArray; import com.google.android.exoplayer2.source.TrackGroupArray;
...@@ -105,9 +104,9 @@ import java.util.concurrent.CopyOnWriteArraySet; ...@@ -105,9 +104,9 @@ import java.util.concurrent.CopyOnWriteArraySet;
ExoPlayerImpl.this.handleEvent(msg); ExoPlayerImpl.this.handleEvent(msg);
} }
}; };
playbackInfo = new ExoPlayerImplInternal.PlaybackInfo(0, 0); playbackInfo = new ExoPlayerImplInternal.PlaybackInfo(timeline, manifest, 0, 0);
internalPlayer = new ExoPlayerImplInternal(renderers, trackSelector, loadControl, playWhenReady, internalPlayer = new ExoPlayerImplInternal(renderers, trackSelector, loadControl, playWhenReady,
repeatMode, shuffleModeEnabled, eventHandler, playbackInfo, this); repeatMode, shuffleModeEnabled, eventHandler, this);
} }
@Override @Override
...@@ -137,6 +136,15 @@ import java.util.concurrent.CopyOnWriteArraySet; ...@@ -137,6 +136,15 @@ import java.util.concurrent.CopyOnWriteArraySet;
@Override @Override
public void prepare(MediaSource mediaSource, boolean resetPosition, boolean resetState) { public void prepare(MediaSource mediaSource, boolean resetPosition, boolean resetState) {
if (!resetPosition) {
maskingWindowIndex = getCurrentWindowIndex();
maskingPeriodIndex = getCurrentPeriodIndex();
maskingWindowPositionMs = getCurrentPosition();
} else {
maskingWindowIndex = 0;
maskingPeriodIndex = 0;
maskingWindowPositionMs = 0;
}
if (resetState) { if (resetState) {
if (!timeline.isEmpty() || manifest != null) { if (!timeline.isEmpty() || manifest != null) {
timeline = Timeline.EMPTY; timeline = Timeline.EMPTY;
...@@ -263,6 +271,7 @@ import java.util.concurrent.CopyOnWriteArraySet; ...@@ -263,6 +271,7 @@ import java.util.concurrent.CopyOnWriteArraySet;
maskingPeriodIndex = periodIndex; maskingPeriodIndex = periodIndex;
} }
if (positionMs == C.TIME_UNSET) { if (positionMs == C.TIME_UNSET) {
// TODO: Work out when to call onPositionDiscontinuity on listeners for this case.
maskingWindowPositionMs = 0; maskingWindowPositionMs = 0;
internalPlayer.seekTo(timeline, windowIndex, C.TIME_UNSET); internalPlayer.seekTo(timeline, windowIndex, C.TIME_UNSET);
} else { } else {
...@@ -313,7 +322,7 @@ import java.util.concurrent.CopyOnWriteArraySet; ...@@ -313,7 +322,7 @@ import java.util.concurrent.CopyOnWriteArraySet;
@Override @Override
public int getCurrentPeriodIndex() { public int getCurrentPeriodIndex() {
if (timeline.isEmpty() || pendingSeekAcks > 0) { if (shouldMaskPosition()) {
return maskingPeriodIndex; return maskingPeriodIndex;
} else { } else {
return playbackInfo.periodId.periodIndex; return playbackInfo.periodId.periodIndex;
...@@ -322,7 +331,7 @@ import java.util.concurrent.CopyOnWriteArraySet; ...@@ -322,7 +331,7 @@ import java.util.concurrent.CopyOnWriteArraySet;
@Override @Override
public int getCurrentWindowIndex() { public int getCurrentWindowIndex() {
if (timeline.isEmpty() || pendingSeekAcks > 0) { if (shouldMaskPosition()) {
return maskingWindowIndex; return maskingWindowIndex;
} else { } else {
return timeline.getPeriod(playbackInfo.periodId.periodIndex, period).windowIndex; return timeline.getPeriod(playbackInfo.periodId.periodIndex, period).windowIndex;
...@@ -358,7 +367,7 @@ import java.util.concurrent.CopyOnWriteArraySet; ...@@ -358,7 +367,7 @@ import java.util.concurrent.CopyOnWriteArraySet;
@Override @Override
public long getCurrentPosition() { public long getCurrentPosition() {
if (timeline.isEmpty() || pendingSeekAcks > 0) { if (shouldMaskPosition()) {
return maskingWindowPositionMs; return maskingWindowPositionMs;
} else { } else {
return playbackInfoPositionUsToWindowPositionMs(playbackInfo.positionUs); return playbackInfoPositionUsToWindowPositionMs(playbackInfo.positionUs);
...@@ -368,7 +377,7 @@ import java.util.concurrent.CopyOnWriteArraySet; ...@@ -368,7 +377,7 @@ import java.util.concurrent.CopyOnWriteArraySet;
@Override @Override
public long getBufferedPosition() { public long getBufferedPosition() {
// TODO - Implement this properly. // TODO - Implement this properly.
if (timeline.isEmpty() || pendingSeekAcks > 0) { if (shouldMaskPosition()) {
return maskingWindowPositionMs; return maskingWindowPositionMs;
} else { } else {
return playbackInfoPositionUsToWindowPositionMs(playbackInfo.bufferedPositionUs); return playbackInfoPositionUsToWindowPositionMs(playbackInfo.bufferedPositionUs);
...@@ -398,7 +407,7 @@ import java.util.concurrent.CopyOnWriteArraySet; ...@@ -398,7 +407,7 @@ import java.util.concurrent.CopyOnWriteArraySet;
@Override @Override
public boolean isPlayingAd() { public boolean isPlayingAd() {
return !timeline.isEmpty() && pendingSeekAcks == 0 && playbackInfo.periodId.isAd(); return !shouldMaskPosition() && playbackInfo.periodId.isAd();
} }
@Override @Override
...@@ -469,28 +478,9 @@ import java.util.concurrent.CopyOnWriteArraySet; ...@@ -469,28 +478,9 @@ import java.util.concurrent.CopyOnWriteArraySet;
break; break;
} }
case ExoPlayerImplInternal.MSG_SOURCE_INFO_REFRESHED: { case ExoPlayerImplInternal.MSG_SOURCE_INFO_REFRESHED: {
SourceInfo sourceInfo = (SourceInfo) msg.obj; int prepareAcks = msg.arg1;
pendingPrepareAcks -= sourceInfo.prepareAcks; int seekAcks = msg.arg2;
pendingSeekAcks -= sourceInfo.seekAcks; handlePlaybackInfo((PlaybackInfo) msg.obj, prepareAcks, seekAcks, false);
if (pendingPrepareAcks == 0) {
timeline = sourceInfo.timeline;
manifest = sourceInfo.manifest;
playbackInfo = sourceInfo.playbackInfo;
if (pendingSeekAcks == 0 && timeline.isEmpty()) {
// Update the masking variables, which are used when the timeline is empty.
maskingPeriodIndex = 0;
maskingWindowIndex = 0;
maskingWindowPositionMs = 0;
}
for (Player.EventListener listener : listeners) {
listener.onTimelineChanged(timeline, manifest);
}
}
if (pendingSeekAcks == 0 && sourceInfo.seekAcks > 0) {
for (Player.EventListener listener : listeners) {
listener.onSeekProcessed();
}
}
break; break;
} }
case ExoPlayerImplInternal.MSG_TRACKS_CHANGED: { case ExoPlayerImplInternal.MSG_TRACKS_CHANGED: {
...@@ -507,30 +497,12 @@ import java.util.concurrent.CopyOnWriteArraySet; ...@@ -507,30 +497,12 @@ import java.util.concurrent.CopyOnWriteArraySet;
break; break;
} }
case ExoPlayerImplInternal.MSG_SEEK_ACK: { case ExoPlayerImplInternal.MSG_SEEK_ACK: {
if (--pendingSeekAcks == 0) { boolean seekPositionAdjusted = msg.arg1 != 0;
playbackInfo = (ExoPlayerImplInternal.PlaybackInfo) msg.obj; handlePlaybackInfo((PlaybackInfo) msg.obj, 0, 1, seekPositionAdjusted);
if (timeline.isEmpty()) {
// Update the masking variables, which are used when the timeline is empty.
maskingPeriodIndex = 0;
maskingWindowIndex = 0;
maskingWindowPositionMs = 0;
}
for (Player.EventListener listener : listeners) {
if (msg.arg1 != 0) {
listener.onPositionDiscontinuity(DISCONTINUITY_REASON_INTERNAL);
}
listener.onSeekProcessed();
}
}
break; break;
} }
case ExoPlayerImplInternal.MSG_POSITION_DISCONTINUITY: { case ExoPlayerImplInternal.MSG_POSITION_DISCONTINUITY: {
if (pendingSeekAcks == 0) { handlePlaybackInfo((PlaybackInfo) msg.obj, 0, 0, true);
playbackInfo = (ExoPlayerImplInternal.PlaybackInfo) msg.obj;
for (Player.EventListener listener : listeners) {
listener.onPositionDiscontinuity(DISCONTINUITY_REASON_PERIOD_TRANSITION);
}
}
break; break;
} }
case ExoPlayerImplInternal.MSG_PLAYBACK_PARAMETERS_CHANGED: { case ExoPlayerImplInternal.MSG_PLAYBACK_PARAMETERS_CHANGED: {
...@@ -555,6 +527,43 @@ import java.util.concurrent.CopyOnWriteArraySet; ...@@ -555,6 +527,43 @@ import java.util.concurrent.CopyOnWriteArraySet;
} }
} }
private void handlePlaybackInfo(PlaybackInfo playbackInfo, int prepareAcks, int seekAcks,
boolean positionDiscontinuity) {
Assertions.checkNotNull(playbackInfo.timeline);
pendingPrepareAcks -= prepareAcks;
pendingSeekAcks -= seekAcks;
if (pendingPrepareAcks == 0 && pendingSeekAcks == 0) {
this.playbackInfo = playbackInfo;
boolean timelineOrManifestChanged = timeline != playbackInfo.timeline
|| manifest != playbackInfo.manifest;
timeline = playbackInfo.timeline;
manifest = playbackInfo.manifest;
if (timeline.isEmpty()) {
// Update the masking variables, which are used when the timeline is empty.
maskingPeriodIndex = 0;
maskingWindowIndex = 0;
maskingWindowPositionMs = 0;
}
if (timelineOrManifestChanged) {
for (Player.EventListener listener : listeners) {
listener.onTimelineChanged(timeline, manifest);
}
}
if (positionDiscontinuity) {
for (Player.EventListener listener : listeners) {
listener.onPositionDiscontinuity(
seekAcks > 0 ? DISCONTINUITY_REASON_INTERNAL : DISCONTINUITY_REASON_PERIOD_TRANSITION
);
}
}
}
if (pendingSeekAcks == 0 && seekAcks > 0) {
for (Player.EventListener listener : listeners) {
listener.onSeekProcessed();
}
}
}
private long playbackInfoPositionUsToWindowPositionMs(long positionUs) { private long playbackInfoPositionUsToWindowPositionMs(long positionUs) {
long positionMs = C.usToMs(positionUs); long positionMs = C.usToMs(positionUs);
if (!playbackInfo.periodId.isAd()) { if (!playbackInfo.periodId.isAd()) {
...@@ -564,4 +573,8 @@ import java.util.concurrent.CopyOnWriteArraySet; ...@@ -564,4 +573,8 @@ import java.util.concurrent.CopyOnWriteArraySet;
return positionMs; return positionMs;
} }
private boolean shouldMaskPosition() {
return timeline.isEmpty() || pendingSeekAcks > 0 || pendingPrepareAcks > 0;
}
} }
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment