Fix threading issues between ExoPlayerImpl/ExoPlayerImplInternal

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=138758739

Fix threading issues between ExoPlayerImpl/ExoPlayerImplInternal
------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=138758739
8cc7dfda · olly · Oliver Woodman · 7ac1cab2 · 8cc7dfda · 8cc7dfda
Commit 8cc7dfda authored Nov 10, 2016 by olly Committed by Oliver Woodman Nov 10, 2016
Showing with 102 additions and 57 deletions
library/src/main/java/com/google/android/exoplayer2/ExoPlayer.java
library/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java
library/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java
--- a/library/src/main/java/com/google/android/exoplayer2/ExoPlayer.java
+++ b/library/src/main/java/com/google/android/exoplayer2/ExoPlayer.java
@@ -113,6 +113,19 @@ public interface ExoPlayer {
  interface EventListener {
    /**
+     * Called when the timeline and/or manifest has been refreshed.
+     * <p>
+     * Note that if the timeline has changed then a position discontinuity may also have occurred.
+     * For example the current period index may have changed as a result of periods being added or
+     * removed from the timeline. The will <em>not</em> be reported via a separate call to
+     * {@link #onPositionDiscontinuity()}.
+     *
+     * @param timeline The latest timeline, or null if the timeline is being cleared.
+     * @param manifest The latest manifest, or null if the manifest is being cleared.
+     */
+    void onTimelineChanged(Timeline timeline, Object manifest);
+    /**
     * Called when the available or selected tracks change.
     *
     * @param trackGroups The available tracks. Never null, but may be of length zero.
@@ -139,14 +152,6 @@ public interface ExoPlayer {
    void onPlayerStateChanged(boolean playWhenReady, int playbackState);
    /**
-     * Called when timeline and/or manifest has been refreshed.
-     *
-     * @param timeline The latest timeline, or null if the timeline is being cleared.
-     * @param manifest The latest manifest, or null if the manifest is being cleared.
-     */
-    void onTimelineChanged(Timeline timeline, Object manifest);
-    /**
     * Called when an error occurs. The playback state will transition to {@link #STATE_IDLE}
     * immediately after this method is called. The player instance can still be used, and
     * {@link #release()} must still be called on the player should it no longer be required.
@@ -156,9 +161,14 @@ public interface ExoPlayer {
    void onPlayerError(ExoPlaybackException error);
    /**
-     * Called when a position discontinuity occurs. Position discontinuities occur when seeks are
+     * Called when a position discontinuity occurs without a change to the timeline. A position
-     * performed, when playbacks transition from one period in the timeline to the next, and when
+     * discontinuity occurs when the current window or period index changes (as a result of playback
-     * the player introduces discontinuities internally.
+     * transitioning from one period in the timeline to the next), or when the playback position
+     * jumps within the period currently being played (as a result of a seek being performed, or
+     * when the source introduces a discontinuity internally).
+     * <p>
+     * When a position discontinuity occurs as a result of a change to the timeline this method is
+     * <em>not</em> called. {@link #onTimelineChanged(Timeline, Object)} is called in this case.
     */
    void onPositionDiscontinuity();
@@ -403,7 +413,7 @@ public interface ExoPlayer {
  Timeline getCurrentTimeline();
  /**
-   * Returns the index of the period currently being played, or {@link C#INDEX_UNSET} if unknown.
+   * Returns the index of the period currently being played.
   */
  int getCurrentPeriodIndex();

--- a/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java
+++ b/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java
@@ -20,8 +20,8 @@ import android.os.Handler;
 import android.os.Looper;
 import android.os.Message;
 import android.util.Log;
-import android.util.Pair;
 import com.google.android.exoplayer2.ExoPlayerImplInternal.PlaybackInfo;
+import com.google.android.exoplayer2.ExoPlayerImplInternal.SourceInfo;
 import com.google.android.exoplayer2.ExoPlayerImplInternal.TrackInfo;
 import com.google.android.exoplayer2.source.MediaSource;
 import com.google.android.exoplayer2.source.TrackGroupArray;
@@ -329,8 +329,7 @@ import java.util.concurrent.CopyOnWriteArraySet;
        break;
      }
      case ExoPlayerImplInternal.MSG_SEEK_ACK: {
-        pendingSeekAcks -= msg.arg1;
+        if (--pendingSeekAcks == 0) {
-        if (pendingSeekAcks == 0) {
          playbackInfo = (ExoPlayerImplInternal.PlaybackInfo) msg.obj;
          for (EventListener listener : listeners) {
            listener.onPositionDiscontinuity();
@@ -348,10 +347,11 @@ import java.util.concurrent.CopyOnWriteArraySet;
        break;
      }
      case ExoPlayerImplInternal.MSG_SOURCE_INFO_REFRESHED: {
-        @SuppressWarnings("unchecked")
+        SourceInfo sourceInfo = (SourceInfo) msg.obj;
-        Pair<Timeline, Object> timelineAndManifest = (Pair<Timeline, Object>) msg.obj;
+        timeline = sourceInfo.timeline;
-        timeline = timelineAndManifest.first;
+        manifest = sourceInfo.manifest;
-        manifest = timelineAndManifest.second;
+        playbackInfo = sourceInfo.playbackInfo;
+        pendingSeekAcks -= sourceInfo.seekAcks;
        for (EventListener listener : listeners) {
          listener.onTimelineChanged(timeline, manifest);
        }

--- a/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java
+++ b/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java
@@ -79,6 +79,22 @@ import java.io.IOException;
  }
+  public static final class SourceInfo {
+    public final Timeline timeline;
+    public final Object manifest;
+    public final PlaybackInfo playbackInfo;
+    public final int seekAcks;
+    public SourceInfo(Timeline timeline, Object manifest, PlaybackInfo playbackInfo, int seekAcks) {
+      this.timeline = timeline;
+      this.manifest = manifest;
+      this.playbackInfo = playbackInfo;
+      this.seekAcks = seekAcks;
+    }
+  }
  private static final String TAG = "ExoPlayerImplInternal";
  // External messages
@@ -533,15 +549,14 @@ import java.io.IOException;
    try {
      if (periodIndex == playbackInfo.periodIndex
-          && ((periodPositionUs == C.TIME_UNSET && playbackInfo.positionUs == C.TIME_UNSET)
+          && ((periodPositionUs / 1000) == (playbackInfo.positionUs / 1000))) {
-          || ((periodPositionUs / 1000) == (playbackInfo.positionUs / 1000)))) {
        // Seek position equals the current position. Do nothing.
        return;
      }
      periodPositionUs = seekToPeriodPosition(periodIndex, periodPositionUs);
    } finally {
      playbackInfo = new PlaybackInfo(periodIndex, periodPositionUs);
-      eventHandler.obtainMessage(MSG_SEEK_ACK, 1, 0, playbackInfo).sendToTarget();
+      eventHandler.obtainMessage(MSG_SEEK_ACK, playbackInfo).sendToTarget();
    }
  }
@@ -551,11 +566,10 @@ import java.io.IOException;
    rebuffering = false;
    setState(ExoPlayer.STATE_BUFFERING);
-    if (periodPositionUs == C.TIME_UNSET || (readingPeriodHolder != playingPeriodHolder
+    if (readingPeriodHolder != playingPeriodHolder && (periodIndex == playingPeriodHolder.index
-        && (periodIndex == playingPeriodHolder.index
+        || periodIndex == readingPeriodHolder.index)) {
-        || periodIndex == readingPeriodHolder.index))) {
+      // Clear the timeline because a renderer is reading ahead to the next period and the seek is
-      // Clear the timeline because either the seek position is not known, or a renderer is reading
+      // to either the playing or reading period.
-      // ahead to the next period and the seek is to either the playing or reading period.
      periodIndex = C.INDEX_UNSET;
    }
@@ -605,10 +619,8 @@ import java.io.IOException;
      playingPeriodHolder = null;
      readingPeriodHolder = null;
      loadingPeriodHolder = null;
-      if (periodPositionUs != C.TIME_UNSET) {
      resetRendererPosition(periodPositionUs);
    }
-    }
    updatePlaybackPositions();
    handler.sendEmptyMessage(MSG_DO_SOME_WORK);
    return periodPositionUs;
@@ -821,30 +833,37 @@ import java.io.IOException;
  private void handleSourceInfoRefreshed(Pair<Timeline, Object> timelineAndManifest)
      throws ExoPlaybackException, IOException {
-    eventHandler.obtainMessage(MSG_SOURCE_INFO_REFRESHED, timelineAndManifest).sendToTarget();
+    Timeline oldTimeline = timeline;
-    Timeline oldTimeline = this.timeline;
+    timeline = timelineAndManifest.first;
-    this.timeline = timelineAndManifest.first;
+    Object manifest = timelineAndManifest.second;
+    if (oldTimeline == null) {
      if (pendingInitialSeekCount > 0) {
        Pair<Integer, Long> periodPosition = resolveSeekPosition(pendingSeekPosition);
        if (periodPosition == null) {
          // TODO: We should probably propagate an error here.
          // We failed to resolve the seek position. Stop the player.
+          finishSourceInfoRefresh(manifest, false);
          stopInternal();
          return;
        }
        playbackInfo = new PlaybackInfo(periodPosition.first, periodPosition.second);
-      eventHandler.obtainMessage(MSG_SEEK_ACK, pendingInitialSeekCount, 0, playbackInfo)
+      } else if (playbackInfo.startPositionUs == C.TIME_UNSET) {
-          .sendToTarget();
+        Pair<Integer, Long> defaultPosition = getPeriodPosition(0, C.TIME_UNSET);
-      pendingInitialSeekCount = 0;
+        playbackInfo = new PlaybackInfo(defaultPosition.first, defaultPosition.second);
-      pendingSeekPosition = null;
+      }
    }
    // Update the loaded periods to take into account the new timeline.
    if (playingPeriodHolder != null) {
      int index = timeline.getIndexOfPeriod(playingPeriodHolder.uid);
      if (index == C.INDEX_UNSET) {
-        attemptRestart(playingPeriodHolder.index, oldTimeline, timeline);
+        boolean restarted = attemptRestart(playingPeriodHolder.index, oldTimeline, timeline);
+        finishSourceInfoRefresh(manifest, true);
+        if (!restarted) {
+          // TODO: We should probably propagate an error here.
+          stopInternal();
+        }
        return;
      }
@@ -873,8 +892,8 @@ import java.io.IOException;
            long newPositionUs = seekToPeriodPosition(index, playbackInfo.positionUs);
            if (newPositionUs != playbackInfo.positionUs) {
              playbackInfo = new PlaybackInfo(index, newPositionUs);
-              eventHandler.obtainMessage(MSG_POSITION_DISCONTINUITY, playbackInfo).sendToTarget();
            }
+            finishSourceInfoRefresh(manifest, true);
            return;
          }
@@ -899,7 +918,12 @@ import java.io.IOException;
      Object uid = loadingPeriodHolder.uid;
      int index = timeline.getIndexOfPeriod(uid);
      if (index == C.INDEX_UNSET) {
-        attemptRestart(loadingPeriodHolder.index, oldTimeline, timeline);
+        boolean restarted = attemptRestart(playingPeriodHolder.index, oldTimeline, timeline);
+        finishSourceInfoRefresh(manifest, true);
+        if (!restarted) {
+          // TODO: We should probably propagate an error here.
+          stopInternal();
+        }
        return;
      } else {
        int windowIndex = timeline.getPeriod(index, this.period).windowIndex;
@@ -908,7 +932,6 @@ import java.io.IOException;
      }
    }
-    // TODO[playlists]: Signal the identifier discontinuity, even if the index hasn't changed.
    if (oldTimeline != null) {
      int newPlayingIndex = playingPeriodHolder != null ? playingPeriodHolder.index
          : loadingPeriodHolder != null ? loadingPeriodHolder.index : C.INDEX_UNSET;
@@ -916,18 +939,16 @@ import java.io.IOException;
          && newPlayingIndex != playbackInfo.periodIndex) {
        playbackInfo = new PlaybackInfo(newPlayingIndex, playbackInfo.positionUs);
        updatePlaybackPositions();
-        eventHandler.obtainMessage(MSG_POSITION_DISCONTINUITY, playbackInfo).sendToTarget();
      }
    }
+    finishSourceInfoRefresh(manifest, true);
  }
-  private void attemptRestart(int oldPeriodIndex, Timeline oldTimeline, Timeline newTimeline) {
+  private boolean attemptRestart(int oldPeriodIndex, Timeline oldTimeline, Timeline newTimeline) {
    int newPeriodIndex = resolveSubsequentPeriod(oldPeriodIndex, oldTimeline, newTimeline);
    if (newPeriodIndex == C.INDEX_UNSET) {
-      // TODO: We should probably propagate an error here.
      // We failed to find a replacement period. Stop the player.
-      stopInternal();
+      return false;
-      return;
    }
    // Release all loaded periods.
@@ -943,9 +964,18 @@ import java.io.IOException;
        timeline.getPeriod(newPeriodIndex, period).windowIndex, C.TIME_UNSET);
    newPeriodIndex = defaultPosition.first;
    long newPlayingPositionUs = defaultPosition.second;
    playbackInfo = new PlaybackInfo(newPeriodIndex, newPlayingPositionUs);
-    eventHandler.obtainMessage(MSG_POSITION_DISCONTINUITY, playbackInfo).sendToTarget();
+    return true;
+  }
+  private void finishSourceInfoRefresh(Object manifest, boolean processedInitialSeeks) {
+    SourceInfo sourceInfo = new SourceInfo(timeline, manifest, playbackInfo,
+        processedInitialSeeks ? pendingInitialSeekCount : 0);
+    eventHandler.obtainMessage(MSG_SOURCE_INFO_REFRESHED, sourceInfo).sendToTarget();
+    if (processedInitialSeeks) {
+      pendingInitialSeekCount = 0;
+      pendingSeekPosition = null;
+    }
  }
  /**
@@ -1076,17 +1106,22 @@ import java.io.IOException;
        int windowIndex = timeline.getPeriod(newLoadingPeriodIndex, period).windowIndex;
        boolean isFirstPeriodInWindow = newLoadingPeriodIndex
            == timeline.getWindow(windowIndex, window).firstPeriodIndex;
-        long periodStartPositionUs = loadingPeriodHolder == null ? playbackInfo.positionUs
+        long periodStartPositionUs;
-            : (isFirstPeriodInWindow ? C.TIME_UNSET : 0);
+        if (loadingPeriodHolder == null) {
-        if (periodStartPositionUs == C.TIME_UNSET) {
+          periodStartPositionUs = playbackInfo.startPositionUs;
-          // This is the first period of a new window or we don't have a start position, so seek to
+        } else if (!isFirstPeriodInWindow) {
-          // the default position for the window. If we're buffering ahead we also project the
+          // We're starting to buffer a new period in the current window. Always start from the
-          // default position so that it's correct for starting playing the buffered duration of
+          // beginning of the period.
-          // time in the future.
+          periodStartPositionUs = 0;
-          long defaultPositionProjectionUs = loadingPeriodHolder == null ? 0
+        } else {
-              : (loadingPeriodHolder.rendererPositionOffsetUs
+          // We're starting to buffer a new window. When playback transitions to this window we'll
+          // want it to be from its default start position. The expected delay until playback
+          // transitions is equal the duration of media that's currently buffered (assuming no
+          // interruptions). Hence we project the default start position forward by the duration of
+          // the buffer, and start buffering from this point.
+          long defaultPositionProjectionUs = loadingPeriodHolder.rendererPositionOffsetUs
              + timeline.getPeriod(loadingPeriodHolder.index, period).getDurationUs()
-                  - loadingPeriodHolder.startPositionUs - rendererPositionUs);
+              - loadingPeriodHolder.startPositionUs - rendererPositionUs;
          Pair<Integer, Long> defaultPosition = getPeriodPosition(timeline, windowIndex,
              C.TIME_UNSET, Math.max(0, defaultPositionProjectionUs));
          if (defaultPosition == null) {