-
Notifications
You must be signed in to change notification settings - Fork 3.6k
feat(android): add audio track selection for ExoPlayer #10312
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
feat(android): add audio track selection for ExoPlayer #10312
Conversation
There was a problem hiding this 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.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
...r/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java
Outdated
Show resolved
Hide resolved
- 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
77b0fce to
b544b45
Compare
|
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 |
|
@camsim99 can you please review this PR ? |
camsim99
left a comment
There was a problem hiding this 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.
...r/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java
Show resolved
Hide resolved
...r/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java
Outdated
Show resolved
Hide resolved
...r/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java
Outdated
Show resolved
Hide resolved
| try { | ||
| await _api.selectAudioTrack(groupIndex, trackIndex); | ||
|
|
||
| // Wait for the onTracksChanged event from ExoPlayer with a timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue we don't need the Completer or any of this logic below. But, I think it's ok to just make the call to _api.selectAudioTrack and use the callback on the native side to just report if there were any issues selecting the track. Thoughts?
I acknowledge I may be missing some context here since I'm reviewing this PR before the main PR--let me know if so!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have tried earlier without waiting , for exoplayer there's a slight delay where in if i fetch the tracks right after selecting different track, it would still give me older values. so using this completer we're waiting to ensure that the track switch is finished before resolving the future. so that the developer can reliably get updated information when he calls getAudioTracks after he selects any track.
we had many discussions on this with different approaches which all had some caveats. this approach seemed to be the least risky and more reliable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay gotcha! Can you do me a favor and point me towards the thread about this just so I can read the discussion too?
packages/video_player/video_player_android/lib/src/android_video_player.dart
Show resolved
Hide resolved
...deo_player_android/android/src/test/java/io/flutter/plugins/videoplayer/AudioTracksTest.java
Outdated
Show resolved
Hide resolved
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 |
…va/io/flutter/plugins/videoplayer/VideoPlayer.java Co-authored-by: Camille Simon <43054281+camsim99@users.noreply.github.com>
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
…ter_packages into 28-oct-platform-android
… 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
|
@camsim99 just checking in 😄 |
camsim99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress here, thanks! Still think we need Dart tests and suggested some changes on the Java side.
...r/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java
Show resolved
Hide resolved
packages/video_player/video_player_android/test/android_video_player_test.dart
Outdated
Show resolved
Hide resolved
| try { | ||
| await _api.selectAudioTrack(groupIndex, trackIndex); | ||
|
|
||
| // Wait for the onTracksChanged event from ExoPlayer with a timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay gotcha! Can you do me a favor and point me towards the thread about this just so I can read the discussion too?
...r/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java
Outdated
Show resolved
Hide resolved
camsim99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM!
However, there are some publishability issues -- can you run the formatter and bump the version of the plugin in the pubspec.yaml? Also looks like the linter is failing on generated code, so you may need to update the baseline by running gradlew updateLintBaseline
|
Done! I've:
Regarding the lint baseline - the updateLintBaseline task ran successfully but reported "No baseline file is specified". The current build.gradle uses the deprecated lintOptions block without a baseline file configured. have added a baseline = file("lint-baseline.xml") to the lint configuration first. @camsim99 can you please check ? |
Cherry-picked from upstream PR flutter#10312: - Implement getAudioTracks() to retrieve available audio tracks - Implement selectAudioTrack() to switch between tracks - Implement isAudioTrackSupportAvailable() for runtime capability detection - Add comprehensive tests for audio track functionality - Update Pigeon definitions for audio track interface - Bump version to 2.9.0 Source: flutter#10312
camsim99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fixes!
I'm not sure about the "No baseline file is specified" error but it looks like lints are passing. We should, however, fix the lints we can fix before landing this though.
packages/video_player/video_player_android/android/lint-baseline.xml
Outdated
Show resolved
Hide resolved
…ation methods and remove related lint baseline entries.
…ter_packages into 28-oct-platform-android
camsim99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM thanks for addressing all the comments! Looks like you need to run the formatter again though.
…ter_packages into 28-oct-platform-android
|
@camsim99 done ✅ All Checks are Passing now ✅ 🏁 ✅ |
| testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner" | ||
| } | ||
| lintOptions { | ||
| lint { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reviewers.
lintOptions was Added in 4.2.0 Deprecated in 7.0.0, still present in 9.0.
Our minimum AGP version is 8.1.1. https://github.com/flutter/flutter/blob/a4959f7544f1e8395838f2cdf4e9f4af50f5e788/packages/flutter_tools/gradle/src/main/kotlin/DependencyVersionChecker.kt#L102C37-L102C51
This change is safe to make in all plugins.
| TrackSelectionOverride override = new TrackSelectionOverride(trackGroup, (int) trackIndex); | ||
|
|
||
| // Apply the track selection override | ||
| trackSelector.setParameters( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am looking at the usages of track selector in this class and it looks like the instance variable trackSelector can stay a TrackSelector until this call.
Maybe you are relying on the value being null someplace.
That said I think we should avoid casting until we have to. What do you think?
| <issues format="6" by="lint 8.11.0" type="baseline" client="gradle" dependencies="false" name="AGP (8.11.0)" variant="all" version="8.11.0"> | ||
|
|
||
| <issue | ||
| id="MemberExtensionConflict" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tarrinneal FYI code generator is creating code with lint warnings.
flutter/packages@2cd921c...57725eb 2025-12-16 nateshmbhat1@gmail.com feat(android): add audio track selection for ExoPlayer (flutter/packages#10312) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
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].///).