feat(ios): add audio track selection support for AVFoundation #10313
Conversation
nateshmbhat
requested review from
LongCatIsLooong and
hellohuanlin
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 audio track selection support for AVFoundation, enabling users to retrieve and select audio tracks from both HLS streams and regular video files. The changes include modifications to the CHANGELOG, platform-specific code (ObjC, Swift), example app, and test files. The minimum supported SDK versions have been updated, and new methods (getAudioTracks(), selectAudioTrack()) have been implemented. The code adheres to the repository's style guide, focusing on objective summaries and using code as the source of truth.
packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation.podspec
Outdated
Show resolved
Hide resolved
packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation/Package.swift
Outdated
Show resolved
Hide resolved
| // Skip nil options | ||
| if (!option || [option isKindOfClass:[NSNull class]]) { | ||
| continue; |
There was a problem hiding this comment.
Consider adding a comment explaining why nil options are skipped. This will help future developers understand the logic behind this check.
There was a problem hiding this comment.
+1, why are we checking against NSNull or checking if the option is nil?
| } | ||
| // For asset tracks, we don't have a direct way to select them in AVFoundation | ||
| // This would require more complex track selection logic that's not commonly used |
There was a problem hiding this comment.
Add a comment explaining why direct selection isn't possible and what the limitations are. This will help future developers understand the current implementation and potential areas for improvement.
packages/video_player/video_player_avfoundation/example/ios/Flutter/AppFrameworkInfo.plist
Outdated
Show resolved
Hide resolved
packages/video_player/video_player_avfoundation/example/ios/Runner.xcodeproj/project.pbxproj
Outdated
Show resolved
Hide resolved
packages/video_player/video_player_avfoundation/example/macos/Podfile
Outdated
Show resolved
Hide resolved
packages/video_player/video_player_avfoundation/example/macos/Runner.xcodeproj/project.pbxproj
Outdated
Show resolved
Hide resolved
| assets: | ||
| - assets/flutter-mark-square-64.png | ||
| - assets/Butterfly-209.mp4 | ||
| # FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE. |
There was a problem hiding this comment.
Per the repository style guide, the review agent should not comment about dependency_overrides sections in pubspec.yaml files that have a comment starting with: "FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE." 1
Style Guide References
Footnotes
- Added getAudioTracks() method supporting both HLS media selection and regular asset tracks - Added selectAudioTrack() method with support for both track selection mechanisms - Included comprehensive test coverage for audio track functionality with edge cases
nateshmbhat
force-pushed
the
28-oct-platform-avfoundation
branch
from
4784bd8 to
459e7f8
Compare
nateshmbhat
changed the title
- Update iOS minimum deployment target from 12.0 to 13.0 - Update macOS minimum deployment target from 10.14 to 10.15 - Add CocoaPods framework embedding build phases to Xcode projects
|
@LongCatIsLooong @tarrinneal could you please review this PR |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, I've been out as my wife is recovering from surgery. I'll try to get to this in the next couple days. |
|
(just started reviewing this, I'll finish doing the review tomorrow, sorry for the delay I was working on a bugfix). |
There was a problem hiding this comment.
So far I've only looked at the objective-c implementation.
My biggest question: why is it required to drop down to lower-level AVAssetTrack stuff? I went to read a little about media selection group and my understanding is that it should be sufficient for most user-facing use cases. What are the use cases for the tracks information?
Additionally, track loading is lazy and asynchronous according to the documentation. I don't see logic that handles the asynchronism.
...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
| // Skip nil options | ||
| if (!option || [option isKindOfClass:[NSNull class]]) { | ||
| continue; |
There was a problem hiding this comment.
+1, why are we checking against NSNull or checking if the option is nil?
...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
...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
…layer_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m Co-authored-by: LongCatIsLooong <[email protected]>
- Remove _cachedAudioSelectionOptions instance variable - Fetch options from media selection group each time in selectAudioTrack - Address PR review feedback from LongCatIsLooong
- Remove unnecessary nil checks for AVMediaSelectionOption - Remove redundant equality check in isSelected comparison - Simplify commonMetadataTitle extraction - Remove try-catch block and type validation for format description parsing - Remove defensive checks for Core Media format descriptions - Streamline audio format description extraction logic
…ayer - Return FlutterError instead of empty data when video not loaded in getAudioTracks - Return FlutterError instead of silently failing in selectAudioTrack when no video loaded - Remove test for nil media selection option handling (no longer needed) - Add getAudioTracks and selectAudioTrack helper methods to _PlayerInstance - Apply dart format to fix code style inconsistencies
…/flutter_packages into 28-oct-platform-avfoundation
@nateshmbhat I have asked you several times on these PRs to stop pinging people for updates/reviews. We have a regular process for following up on reviews, and will handle reminders within the team as necessary.
Delays in review happen because everyone involved in the reviews has a lot of work they are balancing. The problem is not that the Flutter team does not understand what their jobs entail, and implying otherwise is disrespectful to the team. |
tarrinneal
left a comment
tarrinneal
left a comment
There was a problem hiding this comment.
Seems like some of the repo lints have changed since you uploaded this. I noted a couple, but you'll need to update the formatting also.
My approval is for the dart code only. All darwin code will need approval from a member of the ios team before this can land.
packages/video_player/video_player_avfoundation/lib/src/avfoundation_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
…/flutter_packages into 28-oct-platform-avfoundation
LongCatIsLooong
left a comment
LongCatIsLooong
left a comment
There was a problem hiding this comment.
Sorry for the delay. I would think the media selection group abstraction should be sufficient for selecting alternative tracks (although the documentation does not explicitly say that, it would be weird if a basic function like selecting audio needs to be handled in a format specific fashion). I think it would be best if this can be verified in a test.
packages/video_player/video_player_avfoundation/darwin/RunnerTests/VideoPlayerTests.m
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // If no media selection group or empty, try to get tracks from AVAsset (for regular video files) | ||
| NSArray<AVAssetTrack *> *assetAudioTracks = [asset tracksWithMediaType:AVMediaTypeAudio]; |
There was a problem hiding this comment.
For local MP4/MOV files with multiple embedded audio tracks, will mediaSelectionGroupForMediaCharacteristic:AVMediaCharacteristicAudible return those tracks?
The documentation does not say but I assume it should, the mediaSelectionGroup abstraction shouldn't be format dependent. In fact you can verify that in tests if you write one with existing MP4 assets (e.g., "bees.mp4").
…/flutter_packages into 28-oct-platform-avfoundation
|
Hi team 👋 I wanted to share some thoughts on the review experience for this PR series. Timeline so far: Parent PR #9925 opened: Aug 27, 2025 (4+ months ago) Rather, I wanted to flag a pattern I've observed: most feedback rounds have been for relatively minor items (formatting, style, test approach changes), but each round takes 1-3 weeks. By the time the next review happens, context is often lost, and the reviewer needs to re-familiarize themselves with the changes. I'm wondering if there's a way to: Batch minor feedback into fewer rounds, or |
It was reviewed both November 25th and December 1st. A month is certainly longer than our goal for initial reviews, but misrepresenting the facts is not productive. I understand your frustration here, but:
I don't think you do fully understand the extent to which this is true. It sounds from your comment that you think that the core problem is some combination of:
None of those things are the case. The core problem is what I said before:
Fundamentally, having faster turn-around on reviews would require giving those reviews higher priority, which would require deprioritizing other work. There is no way around that reality.
I would urge you consider the effect of consuming team time with meta-discussions about review latency, given that the problem is limited team time. Community members who want to directly help improve review latency are certainly welcome to pick areas of code to learn about and start reviewing PRs for. Having more reviewers would address the core limitation of time. |
|
I understand the situation now. |
LongCatIsLooong
left a comment
LongCatIsLooong
left a comment
There was a problem hiding this comment.
Hmm I still don't understand why AVAssetTrack data needs to be extracted if no media group data is available. From the implementation it looks like those tracks won't be selectable and attempting to select them will fail silently.
If you really want to expose AVAssetTrack I think the API user would need a way to distinguish selectable audio tracks and unselectable ones, and selectAudioTrack should reject unselectable tracks (ideally by proper typing but it looks like the interface PR's already merged), so I'm leaning towards just getting rid of the AVAssetTrack related code and only extract selectable tracks.
| } | ||
|
|
||
| // Always return media selection tracks when there's a media selection group | ||
| // even if all options were nil/invalid (empty array) |
There was a problem hiding this comment.
The if clause checks for empty array right? So it couldn't be empty.
| [currentItem selectMediaOption:option inMediaSelectionGroup:audioGroup]; | ||
| } | ||
| } | ||
| // For asset tracks, we don't have a direct way to select them in AVFoundation |
There was a problem hiding this comment.
If the API user calls this method on an AVAssetTrack, this would fail silently it seems?
| @override | ||
| Future<void> selectAudioTrack(int playerId, String trackId) { | ||
| // Parse the trackId to determine type and extract the integer ID | ||
| String trackType; |
There was a problem hiding this comment.
nit: both local variables should be final
| int numericTrackId; | ||
|
|
||
| if (trackId.startsWith('media_selection_')) { | ||
| trackType = 'mediaSelection'; |
There was a problem hiding this comment.
Injecting prefix to the id for distinguishing the types doesn't seem safe? What if my AVAssetTrack is titled media_selection_a?
| trackType = 'mediaSelection'; | ||
| numericTrackId = int.parse(trackId.substring('media_selection_'.length)); | ||
| } else { | ||
| // Asset track - the trackId is just the integer as a string |
There was a problem hiding this comment.
This would be a no-op right? As the objective-c code for selecting an AVAssetTrack doesn't do anything?
…/flutter_packages into 28-oct-platform-avfoundation
|
Thanks for the review! I've removed all the AVAssetTrack related code based on your comments. This also simplifies the API to only expose tracks that are actually selectable (HLS MediaSelection tracks) |
LongCatIsLooong
left a comment
LongCatIsLooong
left a comment
There was a problem hiding this comment.
The implementation LGTM, but it seems the tests are not compiling.
| final tracks = <VideoAudioTrack>[]; | ||
|
|
||
| for (final track in nativeData) { | ||
| final String? label = track.commonMetadataTitle ?? track.displayName; |
There was a problem hiding this comment.
Shouldn't displayName be preferred over metadata title, because the display name is potentially localized?
There was a problem hiding this comment.
Also should the fallback happen in the objective c part of this implementation so that we don't have to encode / decode both common metadata title and display name in case both are present?
There was a problem hiding this comment.
Our general direction in the repo is to have logic in Dart when there's not a strong reason to have it on the native side; encoding a couple of short short strings isn't something we would expect to have non-trivial perf impact.
fixed the tests @LongCatIsLooong |
|
Can you resolve the conflicts and merge master? |
5 participants
AVFoundation Platform 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].///).