Skip to content
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

[macOS] Implement unobstructed platform views #42960

Merged
merged 5 commits into from
Aug 31, 2023

Conversation

knopp
Copy link
Member

@knopp knopp commented Jun 17, 2023

Fixes flutter/flutter#129073

Changes to Embedder API

Introduced FlutterRegion to represent arbitrary region:

typedef struct {
  /// The size of this struct. Must be sizeof(FlutterRegion).
  size_t struct_size;
  /// Number of rectangles in the region.
  size_t rects_count;
  /// The rectangles that make up the region.
  FlutterRect* rects;
} FlutterRegion;

Note that this is identical to struct FlutterDamage with more generic naming. Maybe down the line we could deprecate FlutterDamage and use FlutterRegion instead.

Introduced FlutterBackingStorePresentInfo:

typedef struct {
  size_t struct_size;

  /// The area of the backing store that contains Flutter contents. Pixels
  /// outside of this area are transparent and the embedder may choose not
  /// to render them. Coordinates are in physical pixels.
  FlutterRegion* paint_region;
} FlutterBackingStorePresentInfo;

In future this struct may also contain more precise hit test region (when framework supports it) and/or information relevant to partial repaint (buffer damage, frame damage).

Added a backing_store_present_info field to FlutterLayer:

typedef struct {
  ...
  /// Extra information for the backing store that the embedder may
  /// use during presentation.
  FlutterBackingStorePresentInfo* backing_store_present_info;
} FlutterLayer;

Changes to the macOS embedder

This PR adds support for FLTEnableSurfaceDebugInfo flag in main bundle Info.plist that enables visual indicators of overlay layers.

Example of unobstructed platform views

surface-debug.mov

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@knopp knopp force-pushed the macos_unobstructed_platform_views branch from 5c12795 to b98df6f Compare June 18, 2023 08:00
@knopp knopp changed the title WIP: [macOS] Implement unobstructed platform views [macOS] Implement unobstructed platform views Jun 18, 2023
@knopp knopp force-pushed the macos_unobstructed_platform_views branch from 734e869 to 05a74f4 Compare June 18, 2023 11:46
@chinmaygarde
Copy link
Member

chinmaygarde commented Jun 21, 2023 via email

/// The size of this struct. Must be sizeof(FlutterRegion).
size_t struct_size;
/// Number of rectangles in the region.
size_t num_rects;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been inconsistent, but I believe the preferred naming pattern is rects_count

@loic-sharma
Copy link
Member

loic-sharma commented Jun 21, 2023

@chinmaygarde I deleted my comment as I figured it'd better to keep FlutterDamage and FlutterRegion separate structs in case we ever need to add new members to FlutterDamage that don't make sense in FlutterRegion. Thanks for the feedback though, I agree that'd be better 👍

@knopp
Copy link
Member Author

knopp commented Jun 21, 2023

The ABI remains stable but the API breaks. I'd rather not break. You could use typedefs.

FlutterDamage has field called damage while FlutterRegion has field called rects. Not sure how the typedef would help unless we union the fields, at which point maybe it's better to just keep the structs separate.

@knopp knopp force-pushed the macos_unobstructed_platform_views branch 5 times, most recently from 01d28c5 to 49665b8 Compare June 26, 2023 10:18
@knopp knopp force-pushed the macos_unobstructed_platform_views branch 2 times, most recently from 1776db3 to 24491a1 Compare June 28, 2023 13:29
@loic-sharma
Copy link
Member

@knopp FYI, Chris Bracken is out on vacation until July 10th and will likely not be able to review until then. Apologies for the delay!

@knopp
Copy link
Member Author

knopp commented Jun 29, 2023

@knopp FYI, Chris Bracken is out on vacation until July 10th and will likely not be able to review until then. Apologies for the delay!

Thanks for the heads-up!

@cbracken
Copy link
Member

I'm back and taking a look at this and the others! Thanks so much for sending them!

@chinmaygarde chinmaygarde removed their request for review July 13, 2023 20:04
@knopp knopp force-pushed the macos_unobstructed_platform_views branch from 24491a1 to cf5f6d5 Compare July 26, 2023 11:16
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great - I'm still looking at the embedder API side changes. Sent the rest of the comments.

@knopp knopp force-pushed the macos_unobstructed_platform_views branch from cf5f6d5 to 59a46a8 Compare July 31, 2023 12:08
@knopp knopp force-pushed the macos_unobstructed_platform_views branch from 62a2b91 to 4f68220 Compare August 22, 2023 20:32
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick note on the embedder.h changes that I'll send out now while I continue to review.

shell/platform/embedder/embedder_backing_store.cc Outdated Show resolved Hide resolved
shell/platform/embedder/embedder_backing_store.h Outdated Show resolved Hide resolved
shell/platform/embedder/embedder.h Outdated Show resolved Hide resolved
@knopp
Copy link
Member Author

knopp commented Aug 23, 2023

I have removed the commit [Move FlutterBackingStorePresentInfo into FlutterBackingStore]. This moves backing_store_present_info back to FlutterLayer. Question is, whether this should be another union - maybe down the line we'll have additional present information for other layer types as well?

@knopp knopp force-pushed the macos_unobstructed_platform_views branch from 4f68220 to e22bfe6 Compare August 23, 2023 18:15
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - thanks for your patience on this one, in particular on the embedder API changes.

@knopp knopp force-pushed the macos_unobstructed_platform_views branch from ae5a33e to d0c76bc Compare August 30, 2023 20:44
@knopp knopp merged commit 30f69f3 into flutter:main Aug 31, 2023
@knopp knopp deleted the macos_unobstructed_platform_views branch August 31, 2023 06:02
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 31, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 31, 2023
…133739)

flutter/engine@1f1071d...1917fa9

2023-08-31 [email protected] Roll Skia from adaad6716b2c to 676a16152834 (1 revision) (flutter/engine#45314)
2023-08-31 [email protected] [macOS] Implement unobstructed platform views (flutter/engine#42960)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[macOS] Implement unobstructed platform views
4 participants