Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Aug 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/native-component-list/package.json
Expand Up @@ -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",
Expand Down
8 changes: 7 additions & 1 deletion 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';
Expand Down Expand Up @@ -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);
Expand Down
35 changes: 34 additions & 1 deletion apps/test-suite/tests/Video.js
Expand Up @@ -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 },
Expand Down Expand Up @@ -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', () => {
Expand Down
2 changes: 2 additions & 0 deletions packages/expo-av/CHANGELOG.md
Expand Up @@ -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
Expand Down
Expand Up @@ -347,6 +347,7 @@ public void onCompletion(final MediaPlayer mp) {

if (!mp.isLooping()) {
mAVModule.abandonAudioFocusIfUnused();
stopUpdatingProgressIfNecessary();
}
}

Expand Down
Expand Up @@ -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();
}
}

Expand All @@ -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;
Expand Down
Expand Up @@ -149,8 +149,6 @@ void playPlayerWithRateAndMuteIfNecessary() throws AudioFocusNotAcquiredExceptio
mSimpleExoPlayer.setPlaybackParameters(new PlaybackParameters(mRate, mShouldCorrectPitch ? 1.0f : mRate));

mSimpleExoPlayer.setPlayWhenReady(mShouldPlay);

beginUpdatingProgressIfNecessary();
}

@Override
Expand Down Expand Up @@ -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;
}
Expand Down
Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
Expand Up @@ -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"
Expand Down