Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,30 @@

package io.flutter.plugins.videoplayer;

import android.os.Handler;
import android.os.Looper;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.media3.common.C;
import androidx.media3.common.PlaybackException;
import androidx.media3.common.Player;
import androidx.media3.common.Timeline;
import androidx.media3.common.Tracks;
import androidx.media3.exoplayer.ExoPlayer;

public abstract class ExoPlayerEventListener implements Player.Listener {
private static final long DURATION_UNSET_INITIALIZATION_TIMEOUT_MS = 500;
private boolean isInitialized = false;
private boolean isWaitingForValidDuration = false;
private final Handler mainHandler = new Handler(Looper.getMainLooper());
private final Runnable initializationFallback =
() -> {
if (!isInitialized && isWaitingForValidDuration) {
isWaitingForValidDuration = false;
isInitialized = true;
sendInitialized();
}
};
protected final ExoPlayer exoPlayer;
protected final VideoPlayerCallbacks events;

Expand Down Expand Up @@ -51,6 +65,39 @@ public ExoPlayerEventListener(

protected abstract void sendInitialized();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The use of Handler.postDelayed introduces a risk of crashing if the player is disposed before the timeout expires. If initializationFallback runs after exoPlayer.release(), calling sendInitialized() (which typically accesses the player) will likely result in an IllegalStateException.

Please add a cleanup method to cancel the pending callback, and ensure the VideoPlayer implementation stores the listener and calls this method during disposal.

  protected abstract void sendInitialized();

  /**
   * Cancels any pending initialization callbacks.
   *
   * <p>This should be called when the listener is no longer needed to prevent memory leaks or
   * crashes from delayed execution.
   */
  public void release() {
    mainHandler.removeCallbacks(initializationFallback);
  }


/** Cancels pending initialization callbacks when the player is disposed. */
public void dispose() {
isWaitingForValidDuration = false;
mainHandler.removeCallbacks(initializationFallback);
}

private boolean hasValidDuration() {
return exoPlayer.getDuration() != C.TIME_UNSET;
}

private boolean shouldWaitForValidDuration() {
return !exoPlayer.isCurrentMediaItemLive() && !exoPlayer.isCurrentMediaItemDynamic();
}

private void maybeSendInitialized() {
if (isInitialized) {
return;
}

if (!hasValidDuration() && shouldWaitForValidDuration()) {
if (!isWaitingForValidDuration) {
isWaitingForValidDuration = true;
mainHandler.postDelayed(initializationFallback, DURATION_UNSET_INITIALIZATION_TIMEOUT_MS);
}
return;
}
Comment on lines +87 to +93
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation resets the initialization fallback timer on every call to maybeSendInitialized if the duration is still unset. If there are frequent timeline updates or state changes, this could delay the fallback initialization significantly beyond the intended 500ms.

Consider only posting the delayed runnable if isWaitingForValidDuration is not already true to ensure the fallback fires within the expected timeframe.

Suggested change
if (!hasValidDuration() && shouldWaitForValidDuration()) {
isWaitingForValidDuration = true;
mainHandler.removeCallbacks(initializationFallback);
mainHandler.postDelayed(initializationFallback, DURATION_UNSET_INITIALIZATION_TIMEOUT_MS);
return;
}
if (!hasValidDuration() && shouldWaitForValidDuration()) {
if (!isWaitingForValidDuration) {
isWaitingForValidDuration = true;
mainHandler.postDelayed(initializationFallback, DURATION_UNSET_INITIALIZATION_TIMEOUT_MS);
}
return;
}


isWaitingForValidDuration = false;
isInitialized = true;
mainHandler.removeCallbacks(initializationFallback);
sendInitialized();
}

@Override
public void onPlaybackStateChanged(final int playbackState) {
PlatformPlaybackState platformState = PlatformPlaybackState.UNKNOWN;
Expand All @@ -60,10 +107,7 @@ public void onPlaybackStateChanged(final int playbackState) {
break;
case Player.STATE_READY:
platformState = PlatformPlaybackState.READY;
if (!isInitialized) {
isInitialized = true;
sendInitialized();
}
maybeSendInitialized();
break;
case Player.STATE_ENDED:
platformState = PlatformPlaybackState.ENDED;
Expand All @@ -75,6 +119,13 @@ public void onPlaybackStateChanged(final int playbackState) {
events.onPlaybackStateChanged(platformState);
}

@Override
public void onTimelineChanged(@NonNull Timeline timeline, int reason) {
if (isWaitingForValidDuration && exoPlayer.getPlaybackState() == Player.STATE_READY) {
maybeSendInitialized();
}
}
Comment on lines +123 to +127
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The PR description mentions that new tests were added, but no changes to ExoPlayerEventListenerTest.java are included in this pull request. The new logic for waiting for a valid duration and the fallback mechanism should be thoroughly tested. The existing tests pass by default because the mock ExoPlayer returns a valid duration (0) instead of C.TIME_UNSET.

Please add unit tests that verify:

  • Initialization is delayed when duration is C.TIME_UNSET for non-live media.
  • Initialization occurs immediately when a valid duration is provided.
  • The fallback mechanism triggers after the timeout if the duration remains unset.
  • onTimelineChanged correctly triggers initialization when the duration becomes available.
References
  1. Code should be tested. Changes to plugin packages should have appropriate tests. (link)


@Override
public void onPlayerError(@NonNull final PlaybackException error) {
if (error.errorCode == PlaybackException.ERROR_CODE_BEHIND_LIVE_WINDOW) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public abstract class VideoPlayer implements VideoPlayerInstanceApi {
@NonNull protected final VideoPlayerCallbacks videoPlayerEvents;
@Nullable protected final SurfaceProducer surfaceProducer;
@Nullable private DisposeHandler disposeHandler;
@Nullable private ExoPlayerEventListener exoPlayerEventListener;
@NonNull protected ExoPlayer exoPlayer;
// TODO: Migrate to stable API, see https://github.com/flutter/flutter/issues/147039.
@UnstableApi @Nullable protected DefaultTrackSelector trackSelector;
Expand Down Expand Up @@ -75,7 +76,8 @@ public VideoPlayer(

exoPlayer.setMediaItem(mediaItem);
exoPlayer.prepare();
exoPlayer.addListener(createExoPlayerEventListener(exoPlayer, surfaceProducer));
exoPlayerEventListener = createExoPlayerEventListener(exoPlayer, surfaceProducer);
exoPlayer.addListener(exoPlayerEventListener);
setAudioAttributes(exoPlayer, options.mixWithOthers);
}

Expand Down Expand Up @@ -237,6 +239,10 @@ public void dispose() {
if (disposeHandler != null) {
disposeHandler.onDispose();
}
if (exoPlayerEventListener != null) {
exoPlayerEventListener.dispose();
exoPlayerEventListener = null;
}
exoPlayer.release();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,21 @@

package io.flutter.plugins.videoplayer;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

import android.os.Looper;
import androidx.media3.common.C;
import androidx.media3.common.PlaybackException;
import androidx.media3.common.Player;
import androidx.media3.common.Timeline;
import androidx.media3.exoplayer.ExoPlayer;
import java.time.Duration;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
Expand All @@ -20,6 +27,7 @@
import org.mockito.junit.MockitoJUnit;
import org.mockito.junit.MockitoRule;
import org.robolectric.RobolectricTestRunner;
import org.robolectric.Shadows;

/**
* Unit tests for {@link ExoPlayerEventListener}.
Expand Down Expand Up @@ -87,13 +95,69 @@ public void onPlaybackStateChangedIdleSendsIdle() {

@Test
public void onPlaybackStateChangedReadySendsInitializedAndReady() {
when(mockExoPlayer.getDuration()).thenReturn(10L);

eventListener.onPlaybackStateChanged(Player.STATE_READY);

verify(mockCallbacks).onPlaybackStateChanged(PlatformPlaybackState.READY);
verifyNoMoreInteractions(mockCallbacks);
assertTrue(eventListener.calledSendInitialized());
}

@Test
public void onPlaybackStateChangedReadyDelaysInitializationWhenDurationUnsetForNonLiveMedia() {
when(mockExoPlayer.getDuration()).thenReturn(C.TIME_UNSET);
when(mockExoPlayer.isCurrentMediaItemLive()).thenReturn(false);
when(mockExoPlayer.isCurrentMediaItemDynamic()).thenReturn(false);

eventListener.onPlaybackStateChanged(Player.STATE_READY);

verify(mockCallbacks).onPlaybackStateChanged(PlatformPlaybackState.READY);
verifyNoMoreInteractions(mockCallbacks);
assertFalse(eventListener.calledSendInitialized());
}

@Test
public void onPlaybackStateChangedReadySendsInitializedImmediatelyWhenDurationAvailable() {
when(mockExoPlayer.getDuration()).thenReturn(10L);

eventListener.onPlaybackStateChanged(Player.STATE_READY);

assertTrue(eventListener.calledSendInitialized());
}

@Test
public void onPlaybackStateChangedReadyFallsBackWhenDurationRemainsUnset() {
when(mockExoPlayer.getDuration()).thenReturn(C.TIME_UNSET);
when(mockExoPlayer.isCurrentMediaItemLive()).thenReturn(false);
when(mockExoPlayer.isCurrentMediaItemDynamic()).thenReturn(false);

eventListener.onPlaybackStateChanged(Player.STATE_READY);
assertFalse(eventListener.calledSendInitialized());

Shadows.shadowOf(Looper.getMainLooper()).idleFor(Duration.ofMillis(500));

assertTrue(eventListener.calledSendInitialized());
}

@Test
public void onTimelineChangedSendsInitializedWhenDurationBecomesAvailable() {
when(mockExoPlayer.getDuration()).thenReturn(C.TIME_UNSET);
when(mockExoPlayer.isCurrentMediaItemLive()).thenReturn(false);
when(mockExoPlayer.isCurrentMediaItemDynamic()).thenReturn(false);

eventListener.onPlaybackStateChanged(Player.STATE_READY);
assertFalse(eventListener.calledSendInitialized());

when(mockExoPlayer.getPlaybackState()).thenReturn(Player.STATE_READY);
when(mockExoPlayer.getDuration()).thenReturn(10L);

eventListener.onTimelineChanged(
mock(Timeline.class), Player.TIMELINE_CHANGE_REASON_SOURCE_UPDATE);

assertTrue(eventListener.calledSendInitialized());
}

@Test
public void onErrorBehindLiveWindowSeekToDefaultAndPrepare() {
eventListener.onPlayerError(
Expand Down