feat(android): add audio track selection for ExoPlayer #10312
Conversation
- Implemented getAudioTracks() and selectAudioTrack() methods for Android video player using ExoPlayer - Removed Kotlin dependencies and switched back to pure Java implementation - Updated event listener to handle buffering states more accurately with start/end events - Simplified playback state handling by removing unnecessary state enum
![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
The pull request implements audio track selection for ExoPlayer in the Android video player. It adds getAudioTracks() and selectAudioTrack() methods, updates the Pigeon definitions, modifies the ExoPlayerEventListener to handle buffering states, and includes corresponding tests. The changes also involve removing Kotlin-related configurations from build.gradle and updating the CHANGELOG.md file.
| private void setBuffering(boolean buffering) { | ||
| if (isBuffering == buffering) { | ||
| return; | ||
| } | ||
| isBuffering = buffering; | ||
| if (buffering) { | ||
| events.onBufferingStart(); | ||
| } else { | ||
| events.onBufferingEnd(); | ||
| } |
There was a problem hiding this comment.
This method encapsulates the logic for setting the buffering state and invoking the corresponding callbacks. It checks if the buffering state has changed before invoking the callbacks, preventing redundant calls. This is a good practice for optimizing performance and avoiding unnecessary UI updates. However, there is no error handling for the callback invocations. Consider adding try-catch blocks around the callback invocations to handle potential exceptions and prevent crashes.
| public @NonNull Messages.PlaybackState getPlaybackState() { | ||
| return new Messages.PlaybackState.Builder() | ||
| .setPlayPosition(exoPlayer.getCurrentPosition()) | ||
| .setBufferPosition(exoPlayer.getBufferedPosition()) | ||
| .build(); |
There was a problem hiding this comment.
The getPlaybackState method creates and returns a PlaybackState object with the current play and buffer positions. This is a good approach for encapsulating the playback state and providing a consistent way to access it. However, the method does not handle potential exceptions that might occur when accessing the ExoPlayer's state. Consider adding error handling to gracefully handle exceptions and prevent crashes.
| public void selectAudioTrack(@NonNull String trackId) { | ||
| if (trackSelector == null) { | ||
| Log.w("VideoPlayer", "Cannot select audio track: track selector is null"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
It's important to handle the case where trackSelector is null to prevent NullPointerException. Logging a warning is a good approach, but consider throwing an exception or returning an error code to the Flutter side to indicate that audio track selection is not supported in this case. This would allow the Flutter app to handle the situation gracefully and provide feedback to the user.
| trackSelector.setParameters( | ||
| trackSelector.buildUponParameters().setOverrideForType(override).build()); |
There was a problem hiding this comment.
The setParameters method can throw exceptions if the provided parameters are invalid or if there is an issue with the underlying player. Consider adding a try-catch block around this call to handle potential exceptions and prevent crashes. Log the exception for debugging purposes.
| Map<String, Object> event = new HashMap<>(); | ||
| event.put("event", "initialized"); | ||
| event.put("width", width); | ||
| event.put("height", height); | ||
| event.put("duration", durationInMs); | ||
| if (rotationCorrectionInDegrees != 0) { | ||
| event.put("rotationCorrection", rotationCorrectionInDegrees); | ||
| } | ||
| eventSink.success(event); |
There was a problem hiding this comment.
The onInitialized method constructs a Map to represent the initialization event. This is a good approach for sending structured data to the Flutter side. However, the code does not handle potential exceptions that might occur when putting values into the Map. Consider adding error handling to gracefully handle exceptions and prevent crashes.
- Moved video event processing logic to Dart for better buffer range updates during pause - Switched Pigeon-generated code to Kotlin for improved type safety - Added Kotlin plugin and configuration to build.gradle - Implemented type-safe event channels for internal communication - Refactored event handling to use dedicated event classes instead of HashMap - Simplified buffering state management by moving logic to Dart layer
| // Create AudioTrackMessage with metadata | ||
| Messages.ExoPlayerAudioTrackData audioTrack = | ||
| new Messages.ExoPlayerAudioTrackData.Builder() | ||
| .setTrackId(groupIndex + "_" + trackIndex) |
There was a problem hiding this comment.
Instead of this string join on groupIndex and trackIndex and sending that as the track id, can we just send the group and track index separately? That way we don't have to do any decoding logic when retrieving the track ID back from the dart side.
There was a problem hiding this comment.
You mean send it as a model or a tuple? And expect the same while setting?
3 participants
Android PR for : #9925
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [version change exemption] this PR falls under[^1].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [CHANGELOG exemption] this PR falls under[^1].///).