[video_player_android] Add video track selection support#11475
[video_player_android] Add video track selection support#11475nateshmbhat wants to merge 2 commits intoflutter:mainfrom
Conversation
Implements getVideoTracks() and selectVideoTrack() methods for video track (quality) selection using ExoPlayer. Android breakout PR for flutter#10688.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces video track selection capabilities to the Android video player plugin. Key changes include adding getVideoTracks(), selectVideoTrack(), and enableAutoVideoQuality() methods to the VideoPlayerInstanceApi and their implementations in VideoPlayer.java and android_video_player.dart. A workaround is implemented in selectVideoTrack to handle video dimension changes by temporarily disabling and re-enabling the video track type to force a renderer reset. New Pigeon messages and data structures are introduced to support video track information exchange, and ExoPlayerEventListener is updated to notify about video track changes. Comprehensive unit tests for the new video track functionalities have also been added. The review comment points out a potential crash risk in the postDelayed workaround within VideoPlayer.java if the player is disposed during the 150ms delay, as the current trackSelector == null guard is ineffective. It recommends using a cancellable callback mechanism or an isDisposed flag for robust handling.
| new android.os.Handler(android.os.Looper.getMainLooper()) | ||
| .postDelayed( | ||
| () -> { | ||
| // Guard against player disposal during the delay | ||
| if (trackSelector == null) { | ||
| return; | ||
| } | ||
|
|
||
| trackSelector.setParameters( | ||
| trackSelector | ||
| .buildUponParameters() | ||
| .setTrackTypeDisabled(C.TRACK_TYPE_VIDEO, false) | ||
| .setOverrideForType(override) | ||
| .build()); | ||
|
|
||
| // Restore playback state | ||
| exoPlayer.seekTo(currentPosition); | ||
| if (wasPlaying) { | ||
| exoPlayer.play(); | ||
| } | ||
| }, | ||
| 150); |
There was a problem hiding this comment.
The postDelayed workaround for dimension changes introduces a potential crash risk. If the VideoPlayer is disposed (and the ExoPlayer released) during the 150ms delay, the calls to exoPlayer.seekTo() and exoPlayer.play() inside the delayed callback will throw an IllegalStateException because the player has been released.
The current guard if (trackSelector == null) (line 414) is likely ineffective because trackSelector is a final field in this class and is not nullified during the dispose() call in the current implementation of this plugin.
Recommendation:
Use a mechanism to cancel the pending callback or a robust way to check if the player has been disposed. For example, you could use a member Handler and call handler.removeCallbacksAndMessages(null) in dispose(), or maintain an isDisposed boolean flag that is set to true in dispose() and checked at the beginning of the delayed callback.
|
This PR requires #11474 to land and be published. |
nateshmbhat
force-pushed
the
breakout/video-track-android
branch
2 times, most recently
from
5ebe09b to
12597e9
Compare
|
@stuartmorgan-g PR unblocked. good to merge ? |
There was a problem hiding this comment.
Code Review
This pull request implements video track selection for the Android video player using ExoPlayer, including methods to retrieve available tracks, select a specific track, and enable auto-quality selection. The implementation includes a workaround for renderer resets when video dimensions change and updates the Pigeon-generated messaging layer. Feedback is provided regarding potential race conditions in the Dart selectVideoTrack implementation where concurrent calls could overwrite shared state, and concerns are raised about state inconsistency and potential crashes during the 150ms delay used for dimension-change renderer resets.
| Future<void> selectVideoTrack(VideoTrack? track) async { | ||
| // Create a completer to wait for the track selection to complete | ||
| _videoTrackSelectionCompleter = Completer<void>(); | ||
|
|
||
| if (track == null) { | ||
| // Auto quality - use dedicated method | ||
| _expectedVideoTrackId = null; | ||
| try { | ||
| await _api.enableAutoVideoQuality(); | ||
|
|
||
| // Wait for the onTracksChanged event from ExoPlayer with a timeout | ||
| await _videoTrackSelectionCompleter!.future.timeout( | ||
| const Duration(seconds: 5), | ||
| onTimeout: () { | ||
| // If we timeout, just continue - the track may still have been selected | ||
| }, | ||
| ); | ||
| } finally { | ||
| _videoTrackSelectionCompleter = null; | ||
| _expectedVideoTrackId = null; | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Extract groupIndex and trackIndex from the track id | ||
| final List<String> parts = track.id.split('_'); | ||
| if (parts.length != 2) { | ||
| throw ArgumentError( | ||
| 'Invalid track id format: "${track.id}". Expected format: "groupIndex_trackIndex"', | ||
| ); | ||
| } | ||
|
|
||
| final int groupIndex = int.parse(parts[0]); | ||
| final int trackIndex = int.parse(parts[1]); | ||
|
|
||
| _expectedVideoTrackId = track.id; | ||
|
|
||
| try { | ||
| await _api.selectVideoTrack(groupIndex, trackIndex); | ||
|
|
||
| // Wait for the onTracksChanged event from ExoPlayer with a timeout | ||
| await _videoTrackSelectionCompleter!.future.timeout( | ||
| const Duration(seconds: 5), | ||
| onTimeout: () { | ||
| // If we timeout, just continue - the track may still have been selected | ||
| }, | ||
| ); | ||
| } finally { | ||
| _videoTrackSelectionCompleter = null; | ||
| _expectedVideoTrackId = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
The selectVideoTrack implementation is vulnerable to race conditions when multiple calls occur concurrently. Since _videoTrackSelectionCompleter and _expectedVideoTrackId are shared fields, a second call will overwrite the state of an ongoing first call. Additionally, the finally block of the first call to complete will clear these fields, which can cause subsequent calls to timeout or resolve prematurely with incorrect state. Using local variables to guard the cleanup ensures that each call only clears the fields if it is still the 'current' active request.
Future<void> selectVideoTrack(VideoTrack? track) async {
final Completer<void> completer = Completer<void>();
_videoTrackSelectionCompleter = completer;
final String? expectedId = track?.id;
_expectedVideoTrackId = expectedId;
try {
if (track == null) {
// Auto quality - use dedicated method
await _api.enableAutoVideoQuality();
} else {
// Extract groupIndex and trackIndex from the track id
final List<String> parts = track.id.split('_');
if (parts.length != 2) {
throw ArgumentError(
'Invalid track id format: "${track.id}". Expected format: "groupIndex_trackIndex"',
);
}
final int groupIndex = int.parse(parts[0]);
final int trackIndex = int.parse(parts[1]);
await _api.selectVideoTrack(groupIndex, trackIndex);
}
// Wait for the onTracksChanged event from ExoPlayer with a timeout
await completer.future.timeout(
const Duration(seconds: 5),
onTimeout: () {
// If we timeout, just continue - the track may still have been selected
},
);
} finally {
if (_videoTrackSelectionCompleter == completer) {
_videoTrackSelectionCompleter = null;
}
if (_expectedVideoTrackId == expectedId) {
_expectedVideoTrackId = null;
}
}
}| if (dimensionsChanged) { | ||
| final boolean wasPlaying = exoPlayer.isPlaying(); | ||
| final long currentPosition = exoPlayer.getCurrentPosition(); | ||
|
|
||
| // Disable video track type to force renderer release | ||
| trackSelector.setParameters( | ||
| trackSelector | ||
| .buildUponParameters() | ||
| .setTrackTypeDisabled(C.TRACK_TYPE_VIDEO, true) | ||
| .build()); | ||
|
|
||
| // Re-enable video with the new track selection after allowing renderer to release. | ||
| // | ||
| // Why 150ms delay? | ||
| // This delay is necessary to allow the MediaCodec decoder and video renderer to fully | ||
| // release their resources before we attempt to create new ones. Without this delay, | ||
| // the new decoder may be initialized before the old one is completely released, leading | ||
| // to resource conflicts and rendering artifacts. The 150ms value was determined through | ||
| // empirical testing across various Android devices and provides a reliable balance | ||
| // between responsiveness and ensuring complete resource cleanup. Shorter delays (e.g., | ||
| // 50-100ms) were found to still cause glitches on some devices, while longer delays | ||
| // would unnecessarily impact user experience. | ||
| new android.os.Handler(android.os.Looper.getMainLooper()) | ||
| .postDelayed( | ||
| () -> { | ||
| // Guard against player disposal during the delay | ||
| if (trackSelector == null) { | ||
| return; | ||
| } | ||
|
|
||
| trackSelector.setParameters( | ||
| trackSelector | ||
| .buildUponParameters() | ||
| .setTrackTypeDisabled(C.TRACK_TYPE_VIDEO, false) | ||
| .setOverrideForType(override) | ||
| .build()); | ||
|
|
||
| // Restore playback state | ||
| exoPlayer.seekTo(currentPosition); | ||
| if (wasPlaying) { | ||
| exoPlayer.play(); | ||
| } | ||
| }, | ||
| 150); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The 150ms delay workaround for dimension changes introduces a risk of state inconsistency. By capturing wasPlaying and currentPosition at the start of the delay and restoring them at the end, any user interactions (like seeking or pausing) that occur during those 150ms will be lost. Furthermore, if the player is disposed during this interval, calling seekTo or play on the released exoPlayer instance might lead to unexpected behavior or crashes, despite the check for trackSelector == null.
No PRs can me merged without review. This needs final re-review by the folks who looked at the original combination PR. |
stuartmorgan-g
added
triage-android
2 participants
Summary
Android breakout PR for #10688.
getVideoTracks()andselectVideoTrack()methods using ExoPlayer'sTrackSelectionOverrideonVideoTrackChangedevent callback for track change notificationsDependency Chain
This PR is second in a series of breakout PRs:
video_player_platform_interface([video_player_platform_interface] Add video track selection support #11474) - pendingvideo_player_android(this PR)video_player_avfoundation(pending)video_player+video_player_web(pending - original PR [video_player] : Add video track selection support for Android and iOS #10688 updated)Note: This PR depends on
video_player_platform_interface6.7.0 being published first.Test Plan
getVideoTracks()andselectVideoTrack()inVideoPlayerTest.javaonVideoTrackChangedcallback inVideoPlayerEventCallbacksTest.javaAndroidVideoPlayeroverrides