Skip to content

Conversation

@nateshmbhat
Copy link
Contributor

@nateshmbhat nateshmbhat commented Oct 28, 2025

  • Implemented getAudioTracks() and selectAudioTrack() methods for Android video player

Android PR for : #9925

Pre-Review Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran [the auto-formatter].
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I [linked to at least one issue that this PR fixes] in the description above.
  • I updated pubspec.yaml with 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].
  • I updated CHANGELOG.md to 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].
  • I updated/added any relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or I have commented below to indicate which [test exemption] this PR falls under[^1].
  • All existing and new tests are passing.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines 50 to 59
private void setBuffering(boolean buffering) {
if (isBuffering == buffering) {
return;
}
isBuffering = buffering;
if (buffering) {
events.onBufferingStart();
} else {
events.onBufferingEnd();
}

Choose a reason for hiding this comment

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

medium

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.

Comment on lines 123 to 127
public @NonNull Messages.PlaybackState getPlaybackState() {
return new Messages.PlaybackState.Builder()
.setPlayPosition(exoPlayer.getCurrentPosition())
.setBufferPosition(exoPlayer.getBufferedPosition())
.build();

Choose a reason for hiding this comment

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

medium

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.

Comment on lines 183 to 187
public void selectAudioTrack(@NonNull String trackId) {
if (trackSelector == null) {
Log.w("VideoPlayer", "Cannot select audio track: track selector is null");
return;
}

Choose a reason for hiding this comment

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

medium

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.

Comment on lines 247 to 234
trackSelector.setParameters(
trackSelector.buildUponParameters().setOverrideForType(override).build());

Choose a reason for hiding this comment

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

medium

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.

Comment on lines 46 to 54
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);

Choose a reason for hiding this comment

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

medium

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.

- Added getAudioTracks() method to retrieve available audio tracks with metadata (bitrate, sample rate, channel count, codec)
- Added selectAudioTrack() method to switch between audio tracks using ExoPlayer's track selector
- Implemented onTracksChanged listener to notify when audio track selection changes
@nateshmbhat nateshmbhat force-pushed the 28-oct-platform-android branch from 77b0fce to b544b45 Compare November 7, 2025 16:27
@nateshmbhat
Copy link
Contributor Author

as we don't wanna do breaking changes to the platform interface @ash2moon have moved the trackindex/groupindex handling logic parsing/encoding logic to the dart side. Please check pr

@reidbaker reidbaker requested review from a team and camsim99 and removed request for ash2moon November 11, 2025 18:09
@nateshmbhat
Copy link
Contributor Author

@camsim99 can you please review this PR ?

@nateshmbhat nateshmbhat requested a review from ash2moon November 14, 2025 15:02
Copy link
Contributor

@camsim99 camsim99 left a comment

Choose a reason for hiding this comment

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

Overall this is looking good! Left some comments, sorry for the delay.

@camsim99
Copy link
Contributor

as we don't wanna do breaking changes to the platform interface @ash2moon have moved the trackindex/groupindex handling logic parsing/encoding logic to the dart side. Please check pr

Also to follow up on this: Is there a reason we don't just send the track index and group index over separately versus as a group_track concatenated String format as ash2moon mentioned?

…va/io/flutter/plugins/videoplayer/VideoPlayer.java

Co-authored-by: Camille Simon <43054281+camsim99@users.noreply.github.com>
@nateshmbhat
Copy link
Contributor Author

as we don't wanna do breaking changes to the platform interface @ash2moon have moved the trackindex/groupindex handling logic parsing/encoding logic to the dart side. Please check pr

Also to follow up on this: Is there a reason we don't just send the track index and group index over separately versus as a group_track concatenated String format as ash2moon mentioned?

initially the idea was to have a single track id which makes it more easy to manage for the developer using the library. also on ios there's no group index. so it was done in that way.

…ion and consolidate tests

- Replaced try-catch with explicit bounds checking for groupIndex and trackIndex
- Added validation for negative indices
- Removed redundant error handling logic
- Consolidated audio track tests from separate AudioTracksTest.java into VideoPlayerTest.java
- Improved code readability by removing nested conditional logic
… and fix formatting

- Added test to verify isAudioTrackSupportAvailable returns true
- Provided dummy values for NativeAudioTrackData and Future types in test setup
- Fixed import ordering to follow convention (package imports before java imports)
- Removed trailing whitespace in VideoPlayerTest.java
- Added comment clarifying that comprehensive audio track tests are in Java layer
@nateshmbhat nateshmbhat requested a review from camsim99 November 19, 2025 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p: video_player platform-android triage-android Should be looked at in Android triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants