-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[video_player] Adds audio track metadata fetching and audio track selection feature #9925
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
base: main
Are you sure you want to change the base?
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
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 ? |
|
native PRs are merged. I have removed the dependency overrides with the new version of the packages. anything else needed from my side ? |
- Implemented getAudioTracks() and selectAudioTrack() methods for Android video player Android PR for : flutter#9925 ## Pre-Review Checklist
|
Is there any plans to support also subtitle (text) selection? And maybe styles of them? |
| ) { | ||
| return VideoAudioTrack( | ||
| id: platformTrack.id, | ||
| label: platformTrack.label ?? 'Unknown', |
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 comment has not been addressed. I don't think this is just a nit; the fact that this is making up placeholder values rather than returning null when nothing is available would be a breaking change to fix if shipped this way.
Co-authored-by: LongCatIsLooong <31859944+LongCatIsLooong@users.noreply.github.com>
- Make VideoAudioTrack label and language nullable to match platform API
- Remove normalization of null values ('Unknown', 'und') in favor of nullable fields
- Update audio tracks demo to handle nullable fields properly
- Replace Exception with StateError for disposed/uninitialized controller states
- Improve listener cleanup and null safety in demo
- Add comment clarifying menu dismissal behavior
stuartmorgan-g
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.
Just minor comments left.
@tarrinneal please re-review the final state, since there have been changes.
| import 'package:flutter/material.dart'; | ||
| import 'package:flutter/services.dart'; | ||
| import 'package:video_player_platform_interface/video_player_platform_interface.dart'; | ||
| import 'package:video_player_platform_interface/video_player_platform_interface.dart' |
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.
There should not be two imports of the same file with different prefixes. If we need prefixing, the other import needs to be removed.
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 think option here is to either replace every usage of all the classes that are used from platform interface with platform_interface. prefix or to rename the VideoAudioTrack to something else in video_player.dart to avoid the naming conflict. what would you prefer ? here's a summary of options we have.
Options for handling duplicate imports
Option 1: Keep both imports (current approach)
import 'package:video_player_platform_interface/video_player_platform_interface.dart';
import 'package:video_player_platform_interface/video_player_platform_interface.dart'
as platform_interface;Pros: Minimal code changes - use unprefixed types everywhere except where disambiguation is needed (platform_interface.VideoAudioTrack)
Cons: Two imports of the same file looks redundant
Option 2: Single prefixed import with show clause
import 'package:video_player_platform_interface/video_player_platform_interface.dart'
as platform_interface show VideoAudioTrack, DataSource, ...;Option 3: Rename the local VideoAudioTrack class
Rename to something like VideoAudioTrackInfo or AudioTrack to avoid the naming collision.
Pros: No duplicate imports; cleaner code
Cons: API change
Option 4: Use hide + prefixed show
dart
import 'package:video_player_platform_interface/video_player_platform_interface.dart'
hide VideoAudioTrack;
import 'package:video_player_platform_interface/video_player_platform_interface.dart'
as platform_interface show VideoAudioTrack;
…rminology - Add blank lines after doc comment opening lines for better readability - Change "exception" to "error" in documentation to match StateError usage - Improve consistency in documentation style across VideoAudioTrack properties
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
Fixes flutter/flutter#59437
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].///).