[expo-av] Fix progress events when no playback is active (#9545)

* [ncl] Add video playback status logging * [expo-av] Fix status events emitted when not playing on Android * [test] Add test for Video progress status update events * [expo-av] Update changelog
expo · Aug 5, 2020 · 9ed42d1 · 9ed42d1
1 parent 480c13d
commit 9ed42d1
Show file tree

Hide file tree

Showing 9 changed files with 77 additions and 16 deletions.
diff --git a/apps/native-component-list/package.json b/apps/native-component-list/package.json
@@ -20,6 +20,7 @@
     "@react-navigation/stack": "~5.6.1",
     "@use-expo/permissions": "^2.0.0",
     "date-format": "^2.0.0",
+    "deep-object-diff": "^1.1.0",
     "expo": "~38.0.0",
     "expo-ads-admob": "~8.2.1",
     "expo-ads-facebook": "~8.3.0",

diff --git a/apps/native-component-list/src/screens/AV/VideoPlayer.tsx b/apps/native-component-list/src/screens/AV/VideoPlayer.tsx
@@ -1,3 +1,4 @@
+import { diff } from 'deep-object-diff';
 import { Asset } from 'expo-asset';
 import { Video, AVPlaybackStatus, VideoFullscreenUpdateEvent } from 'expo-av';
 import React from 'react';
@@ -41,12 +42,17 @@ export default class VideoPlayer extends React.Component<
   };
 
   _video?: Video;
+  private prevStatus?: AVPlaybackStatus;
 
   _handleError = (errorMessage: string) => this.setState({ errorMessage });
 
   _handleVideoMount = (ref: Video) => (this._video = ref);
 
-  _handlePlaybackStatusUpdate = (status: AVPlaybackStatus) => this.setState({ status });
+  _handlePlaybackStatusUpdate = (status: AVPlaybackStatus) => {
+    console.log('onPlaybackStatusUpdate: ', diff(this.prevStatus || {}, status));
+    this.prevStatus = status;
+    this.setState({ status });
+  };
 
   _handleFullScreenUpdate = (event: VideoFullscreenUpdateEvent) =>
     console.log('onFullscreenUpdate', event);

diff --git a/apps/test-suite/tests/Video.js b/apps/test-suite/tests/Video.js
@@ -341,7 +341,7 @@ export function test(t, { setPortalChild, cleanupPortal }) {
         t.expect(status.status.isLoaded).toBe(true);
       });
 
-      t.it("gets called for HLS streams", async () => {
+      t.it('gets called for HLS streams', async () => {
         const props = {
           style,
           source: { uri: hlsStreamUri },
@@ -593,6 +593,39 @@ export function test(t, { setPortalChild, cleanupPortal }) {
           }, 1000);
         });
       });
+
+      t.it('gets called periodically when playing', async () => {
+        const onPlaybackStatusUpdate = t.jasmine.createSpy('onPlaybackStatusUpdate');
+        const props = {
+          onPlaybackStatusUpdate,
+          source,
+          style,
+          ref: refSetter,
+          progressUpdateIntervalMillis: 10,
+        };
+        await mountAndWaitFor(<Video {...props} />);
+        await new Promise(resolve => setTimeout(resolve, 100));
+        await retryForStatus(instance, { isBuffering: false, isLoaded: true });
+        // Verify that status-update doesn't get called periodically when not started
+        const beforeCount = onPlaybackStatusUpdate.calls.count();
+        t.expect(beforeCount).toBeLessThan(6);
+
+        const status = await instance.getStatusAsync();
+        await instance.setStatusAsync({
+          shouldPlay: true,
+          positionMillis: status.durationMillis - 500,
+        });
+        await retryForStatus(instance, { isPlaying: true });
+        await new Promise(resolve => setTimeout(resolve, 500));
+        await retryForStatus(instance, { isPlaying: false });
+        const duringCount = onPlaybackStatusUpdate.calls.count() - beforeCount;
+        t.expect(duringCount).toBeGreaterThan(50);
+
+        // Wait a bit longer and verify it doesn't get called anymore
+        await new Promise(resolve => setTimeout(resolve, 100));
+        const afterCount = onPlaybackStatusUpdate.calls.count() - beforeCount - duringCount;
+        t.expect(afterCount).toBeLessThan(3);
+      });
     });
 
     /*t.describe('Video.setProgressUpdateIntervalAsync', () => {

diff --git a/packages/expo-av/CHANGELOG.md b/packages/expo-av/CHANGELOG.md
@@ -8,6 +8,8 @@
 
 ### 🐛 Bug fixes
 
+- Fix progress events when no playback is active on Android. ([#9545](https://github.com/expo/expo/pull/9545) by [@IjzerenHein](https://github.com/IjzerenHein))
+
 ## 8.4.1 — 2020-07-29
 
 ### 🐛 Bug fixes

diff --git a/packages/expo-av/android/src/main/java/expo/modules/av/player/MediaPlayerData.java b/packages/expo-av/android/src/main/java/expo/modules/av/player/MediaPlayerData.java
@@ -347,6 +347,7 @@ public void onCompletion(final MediaPlayer mp) {
 
     if (!mp.isLooping()) {
       mAVModule.abandonAudioFocusIfUnused();
+      stopUpdatingProgressIfNecessary();
     }
   }
 

diff --git a/packages/expo-av/android/src/main/java/expo/modules/av/player/PlayerData.java b/packages/expo-av/android/src/main/java/expo/modules/av/player/PlayerData.java
@@ -148,28 +148,27 @@ final void stopUpdatingProgressIfNecessary() {
     mProgressUpdater.stopLooping();
   }
 
-  private void progressUpdateLoop() {
-    if (!shouldContinueUpdatingProgress()) {
-      stopUpdatingProgressIfNecessary();
-    } else {
+  final void beginUpdatingProgressIfNecessary() {
+    if (shouldContinueUpdatingProgress() && !mProgressUpdater.isLooping() && (mStatusUpdateListener != null)) {
       mProgressUpdater.loop(mProgressUpdateIntervalMillis, () -> {
         this.callStatusUpdateListener();
         return null;
       });
     }
   }
 
-  final void beginUpdatingProgressIfNecessary() {
-    mProgressUpdater.loop(mProgressUpdateIntervalMillis, () -> {
-      this.callStatusUpdateListener();
-      return null;
-    });
-  }
-
   public final void setStatusUpdateListener(final StatusUpdateListener listener) {
+    final StatusUpdateListener oldListener = mStatusUpdateListener;
     mStatusUpdateListener = listener;
     if (mStatusUpdateListener != null) {
       beginUpdatingProgressIfNecessary();
+
+      // Notify app about the current status upon setting the status listener
+      if (oldListener == null) {
+        callStatusUpdateListener();
+      }
+    } else {
+      stopUpdatingProgressIfNecessary();
     }
   }
 
@@ -192,7 +191,15 @@ abstract void applyNewStatus(final Integer newPositionMillis, final Boolean newI
 
   final void setStatusWithListener(final Bundle status, final SetStatusCompletionListener setStatusCompletionListener) {
     if (status.containsKey(STATUS_PROGRESS_UPDATE_INTERVAL_MILLIS_KEY_PATH)) {
-      mProgressUpdateIntervalMillis = (int) status.getDouble(STATUS_PROGRESS_UPDATE_INTERVAL_MILLIS_KEY_PATH);
+      if (mProgressUpdateIntervalMillis != (int) status.getDouble(STATUS_PROGRESS_UPDATE_INTERVAL_MILLIS_KEY_PATH)) {
+        mProgressUpdateIntervalMillis = (int) status.getDouble(STATUS_PROGRESS_UPDATE_INTERVAL_MILLIS_KEY_PATH);
+
+        // Restart looper when update interval is changed
+        if (mProgressUpdater.isLooping()) {
+          stopUpdatingProgressIfNecessary();
+          beginUpdatingProgressIfNecessary();
+        }
+      }
     }
 
     final Integer newPositionMillis;

diff --git a/packages/expo-av/android/src/main/java/expo/modules/av/player/SimpleExoPlayerData.java b/packages/expo-av/android/src/main/java/expo/modules/av/player/SimpleExoPlayerData.java
@@ -149,8 +149,6 @@ void playPlayerWithRateAndMuteIfNecessary() throws AudioFocusNotAcquiredExceptio
     mSimpleExoPlayer.setPlaybackParameters(new PlaybackParameters(mRate, mShouldCorrectPitch ? 1.0f : mRate));
 
     mSimpleExoPlayer.setPlayWhenReady(mShouldPlay);
-
-    beginUpdatingProgressIfNecessary();
   }
 
   @Override
@@ -301,8 +299,12 @@ public void onPlayerStateChanged(final boolean playWhenReady, final int playback
       && playbackState != mLastPlaybackState
       && playbackState == Player.STATE_ENDED) {
       callStatusUpdateListenerWithDidJustFinish();
+      stopUpdatingProgressIfNecessary();
     } else {
       callStatusUpdateListener();
+      if (playWhenReady && (playbackState == Player.STATE_READY)) {
+        beginUpdatingProgressIfNecessary();
+      }
     }
     mLastPlaybackState = playbackState;
   }

diff --git a/packages/expo-av/android/src/main/java/expo/modules/av/progress/ProgressLooper.kt b/packages/expo-av/android/src/main/java/expo/modules/av/progress/ProgressLooper.kt
@@ -42,6 +42,10 @@ class ProgressLooper(private val timeMachine: TimeMachine) {
     this.listener = null
   }
 
+  fun isLooping(): Boolean {
+    return this.interval > 0;
+  }
+
   private fun scheduleNextTick() {
     if (nextExpectedTick == -1L) {
       nextExpectedTick = timeMachine.time

diff --git a/yarn.lock b/yarn.lock
@@ -6441,6 +6441,11 @@ deep-is@^0.1.3, deep-is@~0.1.3:
   resolved "https://registry.yarnpkg.com/deep-is/-/deep-is-0.1.3.tgz#b369d6fb5dbc13eecf524f91b070feedc357cf34"
   integrity sha1-s2nW+128E+7PUk+RsHD+7cNXzzQ=
 
+deep-object-diff@^1.1.0:
+  version "1.1.0"
+  resolved "https://registry.yarnpkg.com/deep-object-diff/-/deep-object-diff-1.1.0.tgz#d6fabf476c2ed1751fc94d5ca693d2ed8c18bc5a"
+  integrity sha512-b+QLs5vHgS+IoSNcUE4n9HP2NwcHj7aqnJWsjPtuG75Rh5TOaGt0OjAYInh77d5T16V5cRDC+Pw/6ZZZiETBGw==
+
 deep-scope-analyser@^1.6.0:
   version "1.7.0"
   resolved "https://registry.yarnpkg.com/deep-scope-analyser/-/deep-scope-analyser-1.7.0.tgz#23015b3a1d23181b1d9cebd25b783a7378ead8da"