-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[video_player] : Add video track selection support for Android and iOS #10688
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?
[video_player] : Add video track selection support for Android and iOS #10688
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 video track (quality) selection support for Android and iOS in the video_player plugin. It adds getVideoTracks, selectVideoTrack, and isVideoTrackSupportAvailable methods to the VideoPlayerController and the platform interface. The Android implementation leverages ExoPlayer's track selection, including a workaround for resolution changes, while the iOS implementation uses AVAssetVariant for iOS 15+ and preferredPeakBitRate. The web platform includes stub implementations since the feature is not supported there. The changes are well-structured, with corresponding updates to the example app and comprehensive tests. I have one suggestion to improve the example app's robustness by avoiding the use of BuildContext across asynchronous gaps.
| Future<void> _selectVideoTrack(VideoTrack? track) async { | ||
| final VideoPlayerController? controller = _controller; | ||
| if (controller == null) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await controller.selectVideoTrack(track); | ||
|
|
||
| setState(() { | ||
| _isAutoQuality = track == null; | ||
| }); | ||
|
|
||
| // Reload tracks to update selection status | ||
| await _loadVideoTracks(); | ||
|
|
||
| if (!mounted) { | ||
| return; | ||
| } | ||
| final String message = track == null | ||
| ? 'Switched to automatic quality' | ||
| : 'Selected video track: ${_getTrackLabel(track)}'; | ||
| ScaffoldMessenger.of(context).showSnackBar(SnackBar(content: Text(message))); | ||
| } catch (e) { | ||
| if (!mounted) { | ||
| return; | ||
| } | ||
| ScaffoldMessenger.of( | ||
| context, | ||
| ).showSnackBar(SnackBar(content: Text('Failed to select video track: $e'))); | ||
| } | ||
| } |
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.
Avoid using BuildContext across asynchronous gaps. The context might be invalid after an await. It's safer to get a reference to ScaffoldMessenger before the async calls.
Future<void> _selectVideoTrack(VideoTrack? track) async {
final VideoPlayerController? controller = _controller;
if (controller == null) {
return;
}
final ScaffoldMessengerState scaffoldMessenger = ScaffoldMessenger.of(context);
try {
await controller.selectVideoTrack(track);
setState(() {
_isAutoQuality = track == null;
});
// Reload tracks to update selection status
await _loadVideoTracks();
if (!mounted) {
return;
}
final String message = track == null
? 'Switched to automatic quality'
: 'Selected video track: ${_getTrackLabel(track)}';
scaffoldMessenger.showSnackBar(SnackBar(content: Text(message)));
} catch (e) {
if (!mounted) {
return;
}
scaffoldMessenger
.showSnackBar(SnackBar(content: Text('Failed to select video track: $e')));
}
}
hellohuanlin
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 contribution. this seems like a useful feature. left some comments on iOS part.
| @@ -1,5 +1,5 @@ | |||
| # Uncomment this line to define a global platform for your project | |||
| # platform :ios, '12.0' | |||
| # platform :ios, '13.0' | |||
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.
is this change expected? cc @vashworth
| @@ -1,4 +1,4 @@ | |||
| platform :osx, '10.14' | |||
| platform :osx, '10.15' | |||
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.
also this change
...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m
Outdated
Show resolved
Hide resolved
...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m
Outdated
Show resolved
Hide resolved
...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m
Outdated
Show resolved
Hide resolved
...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m
Outdated
Show resolved
Hide resolved
...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m
Outdated
Show resolved
Hide resolved
|
@hellohuanlin Thanks for the review. have addressed your comments 🙏🏻 |
| video_player_platform_interface: | ||
| path: ../video_player_platform_interface | ||
| video_player_web: | ||
| path: ../video_player_web |
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 don't think these changes are correct. We can't pin dependencies to a local path in pubspec.yaml - they must be pinned to published versions. Otherwise, pub publish will reject the package.
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.
btw to test stuff locally, you can try using a pubspec_overrides.yaml which is gitignored, but will still let you use your local changes.
| # the parent directory to use the current plugin's version. | ||
| path: ../ | ||
| video_player_platform_interface: ^6.6.0 | ||
| video_player_platform_interface: |
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.
same comment as my above pubspec comment
| flutter: | ||
| sdk: flutter | ||
| video_player_platform_interface: ^6.6.0 | ||
| video_player_platform_interface: |
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.
here too
| # the parent directory to use the current plugin's version. | ||
| path: ../ | ||
| video_player_platform_interface: ^6.3.0 | ||
| video_player_platform_interface: |
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.
here too
| flutter: | ||
| sdk: flutter | ||
| video_player_platform_interface: ^6.3.0 | ||
| video_player_platform_interface: |
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.
here too
| flutter: | ||
| sdk: flutter | ||
| video_player_platform_interface: ^6.3.0 | ||
| video_player_platform_interface: |
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.
here too
| flutter_web_plugins: | ||
| sdk: flutter | ||
| video_player_platform_interface: ^6.4.0 | ||
| video_player_platform_interface: |
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.
here too
This PR adds video track selection (quality switching) support to the video_player plugin for Android and iOS platforms. This allows users to programmatically get available video quality variants and select a specific quality track, similar to the existing audio track selection feature.
Addresses issue : flutter/flutter#58854
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].///).