-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[video_player] Adds platform view support on macOS #9576
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
[video_player] Adds platform view support on macOS #9576
Conversation
This takes over #8558, since I wanted to make sure we don't lose that work and have unnecessary platform forking in the plugin. |
|
||
@implementation FVPPlayerView | ||
|
||
+ (Class)layerClass { |
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.
Doesn't this make this NSView
's layer a AVPlayerLayer
? If so why do we still need to initialize another AVPlayerLayer
(in initWithFrame
) and add it to this layer as a child?
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 what's the purpose of this NSView subclass, can the logic be moved to FVPNativeVideoView
instead?
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.
Doesn't this make this
NSView
's layer aAVPlayerLayer
?
It turns out that this does... nothing. layerClass
is an iOS-ism that isn't implemented on macOS. The fun of using a language without override annotations to warn about this kind of mistake. The macOS equivalent is the slightly more cumbersome makeBackingLayer
.
If so why do we still need to initialize another
AVPlayerLayer
(ininitWithFrame
) and add it to this layer as a child?
I suspect that not realizing the above is the answer to this question 🙂
Also what's the purpose of this NSView subclass, can the logic be moved to
FVPNativeVideoView
instead?
Probably translating too closely from the iOS version, and on iOS the platform view APIs are a bit weirder. Although now I'm thinking I should take another look at the iOS version to see if we actually need a second view there; we may be able to simplify that code too. (I refreshed my memory of these APIs, and I'm pretty sure that's what happened. On iOS, FlutterPlatformView
isn't a view at all, it's a protocol that does nothing but return a view. That seemed so strange that I didn't replicate it into the macOS version of the API when I was bringing up the macOS embedding, so on macOS there is no wrapper object, so no need for two objects at all.)
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, I was so focused on updating the PR to current main
that I didn't (dust off my long-unused layer-backed NSView experience and) step back and do a big-picture review of the structure of this part of the code.
The new version is much simpler, and seems to work well in my testing.
|
||
@implementation FVPPlayerView | ||
|
||
+ (Class)layerClass { |
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.
Doesn't this make this
NSView
's layer aAVPlayerLayer
?
It turns out that this does... nothing. layerClass
is an iOS-ism that isn't implemented on macOS. The fun of using a language without override annotations to warn about this kind of mistake. The macOS equivalent is the slightly more cumbersome makeBackingLayer
.
If so why do we still need to initialize another
AVPlayerLayer
(ininitWithFrame
) and add it to this layer as a child?
I suspect that not realizing the above is the answer to this question 🙂
Also what's the purpose of this NSView subclass, can the logic be moved to
FVPNativeVideoView
instead?
Probably translating too closely from the iOS version, and on iOS the platform view APIs are a bit weirder. Although now I'm thinking I should take another look at the iOS version to see if we actually need a second view there; we may be able to simplify that code too. (I refreshed my memory of these APIs, and I'm pretty sure that's what happened. On iOS, FlutterPlatformView
isn't a view at all, it's a protocol that does nothing but return a view. That seemed so strange that I didn't replicate it into the macOS version of the API when I was bringing up the macOS embedding, so on macOS there is no wrapper object, so no need for two objects at all.)
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.
LGTM!
flutter/packages@cba2e90...4a231ae 2025-07-08 [email protected] [video_player] Adds platform view support on macOS (flutter/packages#9576) 2025-07-08 [email protected] [camera] fix `CameraLensType` export (flutter/packages#9536) 2025-07-08 [email protected] [video_player] Move iOS/macOS to per-player-instance Pigeon APIs (flutter/packages#9529) 2025-07-08 [email protected] Roll Flutter from 28a4e85 to adffe24 (17 revisions) (flutter/packages#9580) 2025-07-08 [email protected] [google_sign_in] Don't crash a misconfigured iOS app (flutter/packages#9486) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
#171879)" (#171897) <!-- start_original_pr_link --> Reverts: #171879 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: chinmaygarde <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: #171896 <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: engine-flutter-autoroll <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {fluttergithubbot} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: flutter/packages@cba2e90...4a231ae 2025-07-08 [email protected] [video_player] Adds platform view support on macOS (flutter/packages#9576) 2025-07-08 [email protected] [camera] fix `CameraLensType` export (flutter/packages#9536) 2025-07-08 [email protected] [video_player] Move iOS/macOS to per-player-instance Pigeon APIs (flutter/packages#9529) 2025-07-08 [email protected] Roll Flutter from 28a4e85 to adffe24 (17 revisions) (flutter/packages#9580) 2025-07-08 [email protected] [google_sign_in] Don't crash a misconfigured iOS app (flutter/packages#9486) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <[email protected]>
…revisions) (#171879)" (#171897)" (#171910) <!-- start_original_pr_link --> Reverts: #171897 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: matanlurey <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: Was not the reason for the test failure, it was an OOB tag change. <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: auto-submit[bot] <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {fluttergithubbot} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: <!-- start_original_pr_link --> Reverts: #171879 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: chinmaygarde <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: #171896 <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: engine-flutter-autoroll <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {fluttergithubbot} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: flutter/packages@cba2e90...4a231ae 2025-07-08 [email protected] [video_player] Adds platform view support on macOS (flutter/packages#9576) 2025-07-08 [email protected] [camera] fix `CameraLensType` export (flutter/packages#9536) 2025-07-08 [email protected] [video_player] Move iOS/macOS to per-player-instance Pigeon APIs (flutter/packages#9529) 2025-07-08 [email protected] Roll Flutter from 28a4e85 to adffe24 (17 revisions) (flutter/packages#9580) 2025-07-08 [email protected] [google_sign_in] Don't crash a misconfigured iOS app (flutter/packages#9486) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md <!-- end_revert_body --> <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <[email protected]>
…r#171879) flutter/packages@cba2e90...4a231ae 2025-07-08 [email protected] [video_player] Adds platform view support on macOS (flutter/packages#9576) 2025-07-08 [email protected] [camera] fix `CameraLensType` export (flutter/packages#9536) 2025-07-08 [email protected] [video_player] Move iOS/macOS to per-player-instance Pigeon APIs (flutter/packages#9529) 2025-07-08 [email protected] Roll Flutter from 28a4e85 to adffe24 (17 revisions) (flutter/packages#9580) 2025-07-08 [email protected] [google_sign_in] Don't crash a misconfigured iOS app (flutter/packages#9486) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
flutter#171879)" (flutter#171897) <!-- start_original_pr_link --> Reverts: flutter#171879 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: chinmaygarde <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: flutter#171896 <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: engine-flutter-autoroll <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {fluttergithubbot} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: flutter/packages@cba2e90...4a231ae 2025-07-08 [email protected] [video_player] Adds platform view support on macOS (flutter/packages#9576) 2025-07-08 [email protected] [camera] fix `CameraLensType` export (flutter/packages#9536) 2025-07-08 [email protected] [video_player] Move iOS/macOS to per-player-instance Pigeon APIs (flutter/packages#9529) 2025-07-08 [email protected] Roll Flutter from 28a4e85 to adffe24 (17 revisions) (flutter/packages#9580) 2025-07-08 [email protected] [google_sign_in] Don't crash a misconfigured iOS app (flutter/packages#9486) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <[email protected]>
…revisions) (flutter#171879)" (flutter#171897)" (flutter#171910) <!-- start_original_pr_link --> Reverts: flutter#171897 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: matanlurey <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: Was not the reason for the test failure, it was an OOB tag change. <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: auto-submit[bot] <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {fluttergithubbot} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: <!-- start_original_pr_link --> Reverts: flutter#171879 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: chinmaygarde <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: flutter#171896 <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: engine-flutter-autoroll <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {fluttergithubbot} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: flutter/packages@cba2e90...4a231ae 2025-07-08 [email protected] [video_player] Adds platform view support on macOS (flutter/packages#9576) 2025-07-08 [email protected] [camera] fix `CameraLensType` export (flutter/packages#9536) 2025-07-08 [email protected] [video_player] Move iOS/macOS to per-player-instance Pigeon APIs (flutter/packages#9529) 2025-07-08 [email protected] Roll Flutter from 28a4e85 to adffe24 (17 revisions) (flutter/packages#9580) 2025-07-08 [email protected] [google_sign_in] Don't crash a misconfigured iOS app (flutter/packages#9486) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md <!-- end_revert_body --> <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <[email protected]>
…r#171879) flutter/packages@cba2e90...4a231ae 2025-07-08 [email protected] [video_player] Adds platform view support on macOS (flutter/packages#9576) 2025-07-08 [email protected] [camera] fix `CameraLensType` export (flutter/packages#9536) 2025-07-08 [email protected] [video_player] Move iOS/macOS to per-player-instance Pigeon APIs (flutter/packages#9529) 2025-07-08 [email protected] Roll Flutter from 28a4e85 to adffe24 (17 revisions) (flutter/packages#9580) 2025-07-08 [email protected] [google_sign_in] Don't crash a misconfigured iOS app (flutter/packages#9486) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
flutter#171879)" (flutter#171897) <!-- start_original_pr_link --> Reverts: flutter#171879 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: chinmaygarde <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: flutter#171896 <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: engine-flutter-autoroll <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {fluttergithubbot} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: flutter/packages@cba2e90...4a231ae 2025-07-08 [email protected] [video_player] Adds platform view support on macOS (flutter/packages#9576) 2025-07-08 [email protected] [camera] fix `CameraLensType` export (flutter/packages#9536) 2025-07-08 [email protected] [video_player] Move iOS/macOS to per-player-instance Pigeon APIs (flutter/packages#9529) 2025-07-08 [email protected] Roll Flutter from 28a4e85 to adffe24 (17 revisions) (flutter/packages#9580) 2025-07-08 [email protected] [google_sign_in] Don't crash a misconfigured iOS app (flutter/packages#9486) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <[email protected]>
…revisions) (flutter#171879)" (flutter#171897)" (flutter#171910) <!-- start_original_pr_link --> Reverts: flutter#171897 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: matanlurey <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: Was not the reason for the test failure, it was an OOB tag change. <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: auto-submit[bot] <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {fluttergithubbot} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: <!-- start_original_pr_link --> Reverts: flutter#171879 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: chinmaygarde <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: flutter#171896 <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: engine-flutter-autoroll <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {fluttergithubbot} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: flutter/packages@cba2e90...4a231ae 2025-07-08 [email protected] [video_player] Adds platform view support on macOS (flutter/packages#9576) 2025-07-08 [email protected] [camera] fix `CameraLensType` export (flutter/packages#9536) 2025-07-08 [email protected] [video_player] Move iOS/macOS to per-player-instance Pigeon APIs (flutter/packages#9529) 2025-07-08 [email protected] Roll Flutter from 28a4e85 to adffe24 (17 revisions) (flutter/packages#9580) 2025-07-08 [email protected] [google_sign_in] Don't crash a misconfigured iOS app (flutter/packages#9486) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md <!-- end_revert_body --> <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <[email protected]>
Adds macOS support for the platform view display option, brining macOS in line with the iOS implementation. This: - extracts the macOS additions from flutter#8558 as cherry picks, - updates it for some changes that happened during iOS review, and - makes some minor adjustments (mostly combining the factory classes into one shared file) Part of flutter/flutter#86613 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
Adds macOS support for the platform view display option, brining macOS in line with the iOS implementation.
This:
Part of flutter/flutter#86613
Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///
).Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3