[video_player] Add audio track metadata support (bitrate, sample rate, channels, codec) #9782
Conversation
…mple rate, and codec info
There was a problem hiding this comment.
Code Review
This pull request adds valuable support for audio track metadata, including bitrate, sample rate, and codec information. The changes are well-structured across the different packages, with good test coverage for the new functionality. The new VideoAudioTrack model and qualityDescription getter in the platform interface are great additions.
I have two main points of feedback. First, the new example page uses a video that doesn't showcase the multi-track feature well; I've suggested a more suitable one. Second, and more importantly, the iOS/macOS implementation for HLS streams appears to fabricate metadata to match Android's behavior. This could be misleading, and I've recommended providing only the actual data available from the platform.
...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m
Outdated
Show resolved
Hide resolved
| const String videoUrl = | ||
| 'https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/BigBuckBunny.mp4'; |
There was a problem hiding this comment.
The example video URL points to BigBuckBunny.mp4, which is unlikely to have multiple audio tracks. This makes the demo less effective at showcasing the new audio track selection feature. Consider using a test stream that is known to have multiple audio tracks, such as an HLS stream, to make the example more helpful for developers.
For example, Apple provides test streams that could be used here:
const String videoUrl =
'https://devstreaming-cdn.apple.com/videos/streaming/examples/bipbop_16x9/bipbop_16x9_variant.m3u8';
|
since in my code i've used the packages of video_player from the same branch's code instead of published ones using "path" option, i think some of these checks are failing. What should i do in this case ? until its published, the code would have to use "path" option right ? |
|
Thanks for the contribution! A few high-level comments before this can be reviewed:
Please see the instructions in our contributor docs.
Please check that you've uploaded all of your changes; the PR as it is here does not have comprehensive test coverage. The only testing here appears to be of the Dart data class, with no testing of any of the new platform logic.
This is out of scope for the plugin, and should be removed. It's up to plugin clients how/if they want to format any data.
This PR does not appear to have any support for these use cases. Is the PR description inaccurate, or is the PR still a work in progress? |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for audio track metadata, including bitrate, sample rate, channel count, and codec information. The changes are well-implemented across the platform interface, Android (ExoPlayer), and iOS (AVFoundation) layers. The addition of the VideoAudioTrack model is clean, and the new getAudioTracks method is a valuable feature. The accompanying tests are thorough, and the example application is a great way to demonstrate the new functionality. I've identified a couple of minor issues in the example app code that could be improved for robustness and correctness, but overall this is an excellent contribution.
| onPressed: () async { | ||
| if (_controller.value.isInitialized) { | ||
| final audioTracks = await _controller.getAudioTracks(); | ||
| setState(() { | ||
| _audioTracks = audioTracks; | ||
| _isLoadingTracks = false; | ||
| }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
The _isLoadingTracks flag is never set to true, which means the CircularProgressIndicator in the button's icon is never shown. To provide visual feedback to the user while tracks are being fetched, you should set _isLoadingTracks to true before the asynchronous call and to false after it completes. Using a try...finally block is recommended to ensure the loading state is correctly reset even if an error occurs.
onPressed: () async {
if (!_controller.value.isInitialized) {
return;
}
setState(() {
_isLoadingTracks = true;
});
try {
final List<VideoAudioTrack> audioTracks =
await _controller.getAudioTracks();
if (mounted) {
setState(() {
_audioTracks = audioTracks;
});
}
} finally {
if (mounted) {
setState(() {
_isLoadingTracks = false;
});
}
}
},
| Future<void> _getAudioTracks() async { | ||
| if (_controller == null) return; | ||
|
|
||
| setState(() { | ||
| _isLoading = true; | ||
| }); | ||
|
|
||
| try { | ||
| final tracks = await _controller!.getAudioTracks(); | ||
| setState(() { | ||
| _audioTracks = tracks; | ||
| _isLoading = false; | ||
| }); | ||
| } catch (e) { | ||
| debugPrint('Error getting audio tracks: $e'); | ||
| setState(() { | ||
| _isLoading = false; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The _getAudioTracks method can be simplified to improve readability and ensure setState is only called once after the asynchronous operation completes. This also makes it easier to handle the widget's lifecycle by checking mounted once before updating the state.
Future<void> _getAudioTracks() async {
if (_controller == null) {
return;
}
setState(() {
_isLoading = true;
});
List<VideoAudioTrack> tracks = <VideoAudioTrack>[];
try {
tracks = await _controller!.getAudioTracks();
} catch (e) {
debugPrint('Error getting audio tracks: $e');
}
if (!mounted) {
return;
}
setState(() {
_audioTracks = tracks;
_isLoading = false;
});
}
|
…federated plugins documentation
|
@stuartmorgan-g have addressed your high level concerns. anything else that i need to do ? |
There are some CI failures that still need to be addressed, but those don't need to block review. I won't be the primary reviewer here, but some things I can flag from a quick skim while this is waiting for a full review:
|
|
|
||
| // Check if this is an HLS stream | ||
| if ([urlString containsString:@".m3u8"] || | ||
| [urlString containsString:@"application/x-mpegURL"]) { |
There was a problem hiding this comment.
In what circumstances would a MIME type appear in a URL?
Description
This PR adds comprehensive audio track metadata support to the video_player package, enabling developers to access detailed information about available audio tracks including bitrate, sample rate, channel count, and codec information.
Changes Made
bitrate,sampleRate,channelCount,codecRelated Issues :
#59437 - Add audio track metadata support to video_player
Testing
Breaking Changes
None - all changes are additive and backward compatible.
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].///).