[video_player] Adds audio track metadata fetching and audio track selection feature #9925
Conversation
nateshmbhat
requested review from
LongCatIsLooong,
ash2moon,
ditman,
hellohuanlin and
tarrinneal
as code owners
![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 a valuable feature for fetching and selecting audio tracks in the video_player package. The changes are comprehensive, touching the platform interface, Android (ExoPlayer) and iOS (AVFoundation) implementations, and adding a new demo screen.
My review has identified several critical and high-severity issues that should be addressed:
- The new native unit tests for both Android and iOS appear to be broken due to type mismatches and incorrect method calls, and will not compile in their current state.
- The Dart implementation code for both Android and iOS contains unsafe null assertions on data coming from the native side, which could lead to runtime crashes.
- The Android implementation has been updated to use a milestone version of Gradle, which is not recommended for production packages.
- The new audio track selection feature has not been implemented for the web platform, which will lead to
UnimplementedErroron web. - There is a minor issue in the new example app where
contextis used after anawaitwithout checking if the widget is still mounted.
Addressing these points will significantly improve the robustness and completeness of this new feature.
...deo_player_android/android/src/test/java/io/flutter/plugins/videoplayer/AudioTracksTest.java
Outdated
Show resolved
Hide resolved
packages/video_player/video_player_avfoundation/darwin/RunnerTests/AudioTracksTests.m
Outdated
Show resolved
Hide resolved
packages/video_player/video_player_android/android/gradle/wrapper/gradle-wrapper.properties
Outdated
Show resolved
Hide resolved
packages/video_player/video_player_android/lib/src/android_video_player.dart
Outdated
Show resolved
Hide resolved
packages/video_player/video_player_avfoundation/lib/src/avfoundation_video_player.dart
Outdated
Show resolved
Hide resolved
…oPlayer async updates
… fix format builders
|
I updated native andorid tests and ran the tests and verified that all native android tests are passing. |
fix(ios): fixed tests
|
@stuartmorgan-g @ash2moon can you please comment on this PR and lemme know if there are any concerns ? |
stuartmorgan-g
added
triage-ios
|
Any update on this PR needed from my side? |
|
@nateshmbhat You can split out a new PR with the platform implementations (and with the overrides removed in favor of requiring the newly published interface package version), and then the rest of the platform implementation reviews can happen there. |
|
|
||
| // Sample video URLs with multiple audio tracks | ||
| final List<String> _sampleVideos = <String>[ | ||
| 'https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/BigBuckBunny.mp4', |
There was a problem hiding this comment.
Example:
|
|
||
| // Sample video URLs with multiple audio tracks | ||
| final List<String> _sampleVideos = <String>[ | ||
| 'https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/BigBuckBunny.mp4', |
There was a problem hiding this comment.
Or you can upload one with a proper license to https://github.com/flutter/assets-for-api-docs/tree/main/assets/videos
| await controller.initialize(); | ||
|
|
||
| // Add listener for video player state changes | ||
| _controller!.addListener(_onVideoPlayerValueChanged); |
There was a problem hiding this comment.
nit: here and below, use the local controller to avoid !.
| } | ||
|
|
||
| void _onVideoPlayerValueChanged() { | ||
| if (!mounted || _controller == null) { |
There was a problem hiding this comment.
uber nit: the mounted check is not needed if this method is unsubscribed at unmount.
| return const Center(child: CircularProgressIndicator()); | ||
| } | ||
|
|
||
| if (_error != null) { |
There was a problem hiding this comment.
| if (_error != null) { | |
| if (_error case final String error?) { |
so you don't need to use !.
| if (!kIsWeb && Platform.isAndroid) { | ||
| // Add a small delay to allow ExoPlayer to process the track selection change | ||
| // This is needed because ExoPlayer's track selection update is asynchronous | ||
| await Future<void>.delayed(const Duration(milliseconds: 100)); |
There was a problem hiding this comment.
This is still scary even with thorough testing. A future version of exoPlayer can break this hardcoded delay timing (in case the selection takes longer to complete in that version).
I'm not familiar with the API of exoPlayer, but not having an API to tell whether the audio track selection is completed just feels a bit strange.
| NSString *displayName = option.displayName; | ||
|
|
||
| NSString *languageCode = nil; | ||
| if (option.locale) { |
There was a problem hiding this comment.
nit: the if doesn't seem to be necessary?
...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m
Show resolved
Hide resolved
...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m
Show resolved
Hide resolved
...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m
Outdated
Show resolved
Hide resolved
@stuartmorgan-g Raised #10313 and #10312 |
- Added new data classes for audio track management (PlaybackState, AudioTrackMessage, ExoPlayerAudioTrackData, NativeAudioTrackData) - Implemented getAudioTracks() and selectAudioTrack() methods in VideoPlayerInstanceApi - Fixed Map comparison in deepEquals to use containsKey instead of contains - Added message codec handling for new audio track data classes - Updated Android video player implementation to support audio track selection
- Updated video_player_platform_interface dependency from ^6.3.0 to ^6.6.0 across all video_player packages - Removed temporary dependency_overrides for video_player_platform_interface that were used for testing - Reformatted dependency override paths for better readability in remaining overrides
- Reduced sample video URLs from 3 to 2 by removing redundant examples - Changed primary video source to flutter.github.io butterfly video for better reliability - Simplified audio track data construction in Android native code by using direct constructor instead of builder pattern - Fixed code formatting and line wrapping in audio tracks demo UI - Removed unnecessary builder pattern usage in NativeAudioTrackData creation
- Removed fully qualified class names (Messages.ExoPlayerAudioTrackData) in favor of direct class names for better readability - Updated getter method name from getIsSelected() to isSelected() to follow Java boolean getter conventions - Maintained same test coverage and assertions while making code more concise - No functional changes to the test behavior or validation logic
- Upgraded video_player_platform_interface dependency from ^6.4.0 to ^6.6.0 in video_player_web package - Maintains compatibility with web platform support while incorporating latest interface updates
…he code formatting changes: style(video_player): improve code formatting and line wrapping - Fixed line wrapping and indentation in audio_tracks_demo.dart for better readability - Simplified import statements and removed fully qualified class names in AudioTracksTest.java - Improved code formatting and line breaks in avfoundation_video_player files - Adjusted indentation and line breaks in example files to follow consistent style
feat(video): improve audio track selection handling - Removed manual delay for Android track selection and implemented event-based completion tracking - Added AudioTrackChangedEvent to notify when selected audio track changes - Enhanced ExoPlayerEventListener to detect and report audio track changes - Added timeout handling for audio track selection to prevent indefinite waits - Updated message codecs and interfaces to support new audio track change
- Added caching for audio selection options to avoid repeated media group queries - Simplified current audio track selection by using direct item method - Optimized selectAudioTrack to use cached options instead of re-fetching media group - Added cleanup of cached options in dispose method to prevent memory leaks
- Simplified mock format descriptions in VideoPlayerTests to prevent Core Media crashes - Removed unnecessary mock format description setup that could cause test instability - Updated format description validation to only accept genuine Core Media objects, removing NSObject fallback - Fixed potential crash when accessing format descriptions in test environment
|
@LongCatIsLooong have updated the waiting for exoplayer to select the track approach (as per this comment #9925 (comment) ) please check |
…S compatibility - Added macOS 10.13 availability check alongside iOS 11.0 - Updated to use currentMediaSelection property instead of deprecated selectedMediaOptionInMediaSelectionGroup method
…o separate indices - Replaced concatenated string trackId with separate groupIndex and trackIndex parameters in Android implementation - Updated iOS/macOS to use trackType and numeric trackId instead of prefixed string format - Modified Pigeon message definitions and generated code to support new parameter structure
|
Have addressed all the major concerns in this PR , can we please have the platform PRs merged ? If any furthur changes are needed, pls let me know at the earliest so that i can quickly push the changes and we can take this forward sooner. |
I'm not sure why you are pinging people here. This PR can't land until the sub-PRs have landed, and the sub PRs are still under review. Those PRs will be landed when they have been reviewed and approved. |
7 participants
Description
This PR adds comprehensive audio track retrieval and selection support to the video_player package, enabling developers to access detailed information about available audio tracks and switch between them during playback.
Breakout PRs:
Changes Made
Core Features
VideoAudioTrackmodel with comprehensive metadata fields:id,label,language,isSelected,bitrate,sampleRate,channelCount,codecVideoPlayerControllerto expose audio track functionalityPlatform Implementations
getCurrentTracks()APITrackSelectionOverridewith proper error handlingAVAssetTrackfor regular videosAVMediaSelectionGroupfor adaptive streamsTechnical Infrastructure
AudioTrackMessage,ExoPlayerAudioTrackData,AssetAudioTrackData,MediaSelectionAudioTrackData,NativeAudioTrackDataDemo Features
Related Issues
#59437 - Add audio track metadata support to video_player
Testing
Breaking Changes
None - all changes are additive and backward compatible.
Pre-Review Checklist
[video_player]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].///).