[video_player_avfoundation] Platform view support #8237
Conversation
… and platform view for each video
|
I don't own these packages so removing myself, but this seems like a good entrypoint for an incoming web PR adding a "texture"-like rendering for the video player there. PR (it got closed though, so I'm not sure if it'll ever get updated to leverage this). |
...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m
Show resolved
Hide resolved
...win/video_player_avfoundation/Sources/video_player_avfoundation/FVPTextureBasedVideoPlayer.m
Outdated
Show resolved
Hide resolved
...win/video_player_avfoundation/Sources/video_player_avfoundation/FVPTextureBasedVideoPlayer.m
Show resolved
Hide resolved
...win/video_player_avfoundation/Sources/video_player_avfoundation/FVPTextureBasedVideoPlayer.m
Show resolved
Hide resolved
...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m
Outdated
Show resolved
Hide resolved
...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m
Outdated
Show resolved
Hide resolved
.../video_player_avfoundation/Sources/video_player_avfoundation_ios/FVPNativeVideoViewFactory.m
Outdated
Show resolved
Hide resolved
.../video_player_avfoundation/Sources/video_player_avfoundation_ios/FVPNativeVideoViewFactory.m
Outdated
Show resolved
Hide resolved
| Future<int?> create(DataSource dataSource) { | ||
| throw UnimplementedError('create() has not been implemented.'); | ||
| } | ||
|
|
||
| /// Creates an instance of a video player based on creation options | ||
| /// and returns its playerId. If view type is [VideoViewType.textureView], | ||
| /// the playerId is also the texture id. |
There was a problem hiding this comment.
Looking at just the platform interface part again, I don't think we should encode this into the interface, as it will make it harder for the higher-level code to untangle the now-ideally-abstract player ID from the texture ID, which has a very specific meaning.
There's also a design issue here where both createWithOptions and buildViewWithOptions are being passed a view type, but once createWithOptions has been called, there is only one valid type that can be passed to buildViewWithOptions for that player ID, so the calling code is being forced to maintain view type state.
Couldn't VideoViewOptions only contain the player ID for now, and the state of how to map player ID to implementation details like the texture ID (if any) be maintained in the platform implementation, which is already stateful?
There was a problem hiding this comment.
I did as suggested. Now the responsibility of maintaining view type state for a given player relies on the platform implementation.
The player ID as viewed by the app facing package should now more abstract, but currently we ended up in a very ugly state where most of the methods still use textureId for parameter names (which now should be renamed to playerId). I suppose that we could fix it all up when creating platform interface PR?
In case of texture view, the player ID will still be the same as texture ID, we would just not explicitly reveal that. If we wanted to change this and make the player ID fully decoupled from the texture ID, that would probably require some significant changes - either moving the player ID generation process to the Dart side or changing the pigeon interface so that it returns player ID as well as texture ID (which can be null).
There was a problem hiding this comment.
In case of texture view, the player ID will still be the same as texture ID, we would just not explicitly reveal that.
That's fine for now as an implementation detail, I just don't want that to become something we are stuck with indefinitely because it's part of the API contract of the new methods.
| test('createWithOptions with incorrect asset throws exception', () async { | ||
| try { | ||
| await player.createWithOptions( | ||
| VideoCreationOptions( | ||
| dataSource: DataSource( | ||
| sourceType: DataSourceType.asset, | ||
| asset: '/path/to/incorrect_asset', | ||
| ), | ||
| viewType: VideoViewType.textureView, | ||
| ), | ||
| ); | ||
| fail('should throw PlatformException'); | ||
| } catch (e) { | ||
| expect(e, isException); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I copied this test from the one above (for create method) which was added in this PR, but I don't think it checks anything.
I assume the call to createWithOptions (or create in the original test) should fail, then we should catch it, and verify the exception. But looks like what actually happens is that the fail call throws, we just catch it and verify that it is exception (which it is because it's a TestFailure).
Is this test needed? What value does it bring?
There was a problem hiding this comment.
But looks like what actually happens is that the
failcall throws, we just catch it and verify that it is exception (which it is because it's aTestFailure).
Sounds like the test is broken; the preferred way to write it would be with a throwsA matcher on the call, instead of a manual try/catch.
There was a problem hiding this comment.
Does this test make sense here in the first place? The exception it is supposed to check comes from the native side.
There was a problem hiding this comment.
Ugh, I see; sorry, I hadn't looked at the initial PR. You are correct, this test makes no sense here. It should have been either a native unit test, or a Dart integration test, since a Dart unit test can't possibly exercise that code. Unfortunately that was missed in review.
There was a problem hiding this comment.
I didn't fully re-review, but I looked at the platform interface part and skimmed how that affected the implementation. It looks like this approach does what we need, and will be clearly applicable to other platforms (and if ever necessary, added view types) so I think we're good to split that piece out to its own PR now, so that it can be landed and unblock Android work, while the iOS review proceeds.
| /// A map that associates player ID with a view type. | ||
| /// This is used to determine which view type to use when building a view. | ||
| @visibleForTesting | ||
| final Map<int, VideoViewType> playerViewTypes = <int, VideoViewType>{}; |
There was a problem hiding this comment.
In the interest of making this less confusing to disentangle later, let's instead make the value here a sealed base type called something like _VideoPlayerState, with subtypes for the two video types. The texture view subtype can contain a textureId field, and that can be used on line 186 so that we aren't subtly relying on player ID and texture ID being the same, such that if we change that later in one place, we have to know to change it in a totally different method elsewhere in the file.
(In the short term it'll be redundant in practice because the player ID and the texture ID will be the same, but this way we're tracking the different uses explicitly from the start.)
This PR adds support for platform views on iOS as a way of displaying a video. When creating a video, it's now possible to choose between texture view approach (rendered using
Texturewidget on the Flutter side) and platform view approach (rendered on the native side, usingAVPlayerLayer).FVPVideoPlayerclass now has nothing to do with texture. The texture-related code was moved from it toFVPVideoPlayerTextureApproach- a subclass ofFVPVideoPlayerthat adds texture functionality. In the plugin class (createWithOptionsmethod) we create either the basic version (for platform view) or the texture subclass (in case of texture approach) based on the parameter passed in from Flutter side.Platform view is only supported on iOS, no MacOS implementation is added in this PR. The functionality is not yet exposed in the app-facing package (only in example app) - it will be done later, once we add the Android implementation. The PR does not introduce breaking changes, I followed the rule "non-breaking changes, even at the expense of a less-clean API" (
buildViewWithOptionsandcreateWithOptionsmethods).Up to this point, the variable naming relied heavily on the texture (we had a lot of
textureIdvariables and properties). Since now you can use a platform view instead of a texture view, these variables and parameters will be renamed to justplayerId. This will be done in a separate PR to keep git diff for this one clean.I left some comments in the PR to clarify/discuss some choices.
Resolves #86613 (not fully though, as it is not yet available in the app-facing package).
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.