[macOS] Refactor rendering infrastructure by knopp · Pull Request #37789 · flutter/engine

knopp · 2022-11-20T18:40:51Z

Notable changes:

Removed FlutterIOSurfaceHolder, FlutterResizableBackingStore
Replaced FlutterResizeSynchronizer with FlutterThreadSynchronizer
Changed the role of FlutterSurfaceManager
Unified most of the rendering codepath with and without compositor

High level overview

FlutterView owns FlutterSurfaceManager
FlutterCompositor and FlutterRenderer interact with FlutterSurfaceManager in order to
- Obtain render surfaces, which are then represented as metal textures and provided to engine.
- Present render surfaces, which FlutterSurfaceManager sets as contents of CALayers that it manages.
FlutterSurfaceManager caches surfaces so that during normal (no resizing) repaint IOSurfaces do not need to be recreated.
FlutterThreadSynchronizer is in charge of synchronising raster and platform thread:
- During resizing blocks platform thread until content of requested size is available.
- When rasterizing is done blocks raster thread while layers contents are updated and platform views are mutated (on platform thread).

Fixes

Simplifies rendering flow.
Fixes threading issues with platform views (FlutterEngine.Compositor test now works)
Fixes issue with IOSurfaces constantly being recreated during regular (non resizing) repaints when using platform views.

List which issues are fixed by this PR. You must list at least one issue.

Fixes flutter/flutter#96668
Fixes flutter/flutter#113785

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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.

shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm

knopp · 2022-11-21T22:13:51Z

I updated @iskakaushik's platform view example if anyone wants to test platform views:

https://github.com/knopp/flutter_macos_platform_view_example

platform.view.example.mov

dkwingsmt

The change looks OK to me, but I don't have enough knowledge to make the final approval.

shell/platform/darwin/macos/framework/Source/FlutterView.h

shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm

shell/platform/darwin/macos/framework/Source/FlutterCompositor.h

cbracken

Overall, this is a really nice cleanup. Almost all of my comments are nitpicks.

Sending first round of comments. Still looking at FlutterSurfaceManager and FlutterThreadManager.

shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm

shell/platform/darwin/macos/framework/Source/FlutterSurface.mm

shell/platform/darwin/macos/framework/Source/FlutterRenderer.mm

skia-gold · 2022-12-01T12:15:29Z

Gold has detected about 2 new digest(s) on patchset 8.
View them at https://flutter-engine-gold.skia.org/cl/github/37789

Co-authored-by: Chris Bracken <chris@bracken.jp>

…agerTest.mm Co-authored-by: Chris Bracken <chris@bracken.jp>

…ager.h Co-authored-by: Chris Bracken <chris@bracken.jp>

…agerTest.mm Co-authored-by: Chris Bracken <chris@bracken.jp>

…ager.h Co-authored-by: Chris Bracken <chris@bracken.jp>

cbracken

🎉🎉🎉

* 1a1b1fe Roll Skia from 537e1e8c1ca6 to 729ccbfb87bc (7 revisions) (flutter/engine#38277) * 3b18821 Roll Fuchsia Linux SDK from A0jnUUORf2LQu1z2V... to e2lfUFBW5ddtTZBbw... (flutter/engine#38280) * beb94ea Roll Skia from 729ccbfb87bc to 3171deabd88a (4 revisions) (flutter/engine#38279) * 8f9071a Roll Fuchsia Mac SDK from FQQdl8AGAsALFniHl... to u-tC0QEGUT4xQ4KOo... (flutter/engine#38282) * aa78cd8 add link to website (flutter/engine#38273) * 955447b pylint all Python scripts under testing/ (flutter/engine#38268) * 3f22e19 [web] correct float count in runtime effect (flutter/engine#38288) * 479bb73 Fix issues related to keyboard inset (flutter/engine#37719) * 6cd8561 [macOS] Refactor rendering infrastructure (flutter/engine#37789) * d1533d1 [web] Make Canvaskit's malloc more useful (flutter/engine#38130) * db5605e Fix new `unnecessary_parenthesis` diagnostics. (flutter/engine#38291)

This reverts commit 6cd8561.

* [macos] Refactor rendering process * Put includes and imports together * Update shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm Co-authored-by: Chris Bracken <chris@bracken.jp> * Update shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm Co-authored-by: Chris Bracken <chris@bracken.jp> * Update shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm Co-authored-by: Chris Bracken <chris@bracken.jp> * Update shell/platform/darwin/macos/framework/Source/FlutterPlatformViewController.mm Co-authored-by: Chris Bracken <chris@bracken.jp> * Update shell/platform/darwin/macos/framework/Source/FlutterRenderer.mm Co-authored-by: Chris Bracken <chris@bracken.jp> * Remove cast * Do not manually add platform view layer to FlutterView * Remove unnecesary _Internal header files * Make surfaceManager a readonly property * Add comment * Style nit: Rewrite as a noun phrase. * Naming nit for consistency with removeSurfaceForSize: * Write as ternary conditional. * Fix plural in comment. * Offset and index are already set to 0. * Remove FlutterSurfaceManager lookupSurface And the associate bookkeeping of borrowed (lent) surfaces. * Update shell/platform/darwin/macos/framework/Source/FlutterThreadSynchronizer.mm Co-authored-by: Chris Bracken <chris@bracken.jp> * Update shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm Co-authored-by: Chris Bracken <chris@bracken.jp> * Update shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm Co-authored-by: Chris Bracken <chris@bracken.jp> * Update shell/platform/darwin/macos/framework/Source/FlutterRenderer.mm Co-authored-by: Chris Bracken <chris@bracken.jp> * Update shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm Co-authored-by: Chris Bracken <chris@bracken.jp> * Update shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.h Co-authored-by: Chris Bracken <chris@bracken.jp> * Update shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm Co-authored-by: Chris Bracken <chris@bracken.jp> * Update shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.h Co-authored-by: Chris Bracken <chris@bracken.jp> * Update shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.h Co-authored-by: Chris Bracken <chris@bracken.jp> * Update shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.h Co-authored-by: Chris Bracken <chris@bracken.jp> * Fix build error * Address nits * Replace EVent with fml::AutoResetWaitableEvent * Remove unecessary CATransaction * Add GetRequiredFrameSize Co-authored-by: Chris Bracken <chris@bracken.jp>

…17109) * 1a1b1fe Roll Skia from 537e1e8c1ca6 to 729ccbfb87bc (7 revisions) (flutter/engine#38277) * 3b18821 Roll Fuchsia Linux SDK from A0jnUUORf2LQu1z2V... to e2lfUFBW5ddtTZBbw... (flutter/engine#38280) * beb94ea Roll Skia from 729ccbfb87bc to 3171deabd88a (4 revisions) (flutter/engine#38279) * 8f9071a Roll Fuchsia Mac SDK from FQQdl8AGAsALFniHl... to u-tC0QEGUT4xQ4KOo... (flutter/engine#38282) * aa78cd8 add link to website (flutter/engine#38273) * 955447b pylint all Python scripts under testing/ (flutter/engine#38268) * 3f22e19 [web] correct float count in runtime effect (flutter/engine#38288) * 479bb73 Fix issues related to keyboard inset (flutter/engine#37719) * 6cd8561 [macOS] Refactor rendering infrastructure (flutter/engine#37789) * d1533d1 [web] Make Canvaskit's malloc more useful (flutter/engine#38130) * db5605e Fix new `unnecessary_parenthesis` diagnostics. (flutter/engine#38291)

flutter-dashboard bot added the platform-macos label Nov 20, 2022

knopp force-pushed the macos_rendering_refactor branch 2 times, most recently from 296fc00 to 3bc00c7 Compare November 20, 2022 20:27

knopp mentioned this pull request Nov 20, 2022

WIP: macOS: Fix platform view threading issues #35894

Closed

8 tasks

knopp force-pushed the macos_rendering_refactor branch 4 times, most recently from d368c3d to d949242 Compare November 21, 2022 12:50

knopp changed the title ~~WIP: [macos] Refactor rendering process~~ [macos] Refactor rendering process Nov 21, 2022

knopp force-pushed the macos_rendering_refactor branch from e4afa28 to 1881f57 Compare November 21, 2022 16:43

This comment was marked as outdated.

Sign in to view

flutter-dashboard bot added the will affect goldens label Nov 21, 2022

knopp requested review from cbracken, dkwingsmt and iskakaushik November 21, 2022 17:34

knopp force-pushed the macos_rendering_refactor branch from c7a3ad9 to 1c7d550 Compare November 21, 2022 17:40

knopp changed the title ~~[macos] Refactor rendering process~~ [macOS] Refactor rendering process Nov 21, 2022

knopp changed the title ~~[macOS] Refactor rendering process~~ [macOS] Refactor rendering infrastructure Nov 21, 2022

dkwingsmt reviewed Nov 21, 2022

View reviewed changes

shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm Outdated Show resolved Hide resolved

dkwingsmt reviewed Nov 21, 2022

View reviewed changes

shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm Outdated Show resolved Hide resolved

knopp added affects: desktop and removed will affect goldens labels Nov 21, 2022

dkwingsmt reviewed Nov 21, 2022

View reviewed changes

shell/platform/darwin/macos/framework/Source/FlutterView.h Outdated Show resolved Hide resolved

shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm Show resolved Hide resolved

knopp force-pushed the macos_rendering_refactor branch 2 times, most recently from 6accd38 to 50a9413 Compare November 25, 2022 16:45

cbracken reviewed Nov 30, 2022

View reviewed changes

shell/platform/darwin/macos/framework/Source/FlutterCompositor.h Show resolved Hide resolved

knopp force-pushed the macos_rendering_refactor branch from cb2b42f to 3d68042 Compare November 30, 2022 12:00

cbracken reviewed Dec 1, 2022

View reviewed changes

knopp and others added 11 commits December 13, 2022 20:12

Update shell/platform/darwin/macos/framework/Source/FlutterRenderer.mm

e077a6c

Co-authored-by: Chris Bracken <chris@bracken.jp>

Update shell/platform/darwin/macos/framework/Source/FlutterSurfaceMan…

cb7c4b7

…agerTest.mm Co-authored-by: Chris Bracken <chris@bracken.jp>

Update shell/platform/darwin/macos/framework/Source/FlutterSurfaceMan…

a13d260

…ager.h Co-authored-by: Chris Bracken <chris@bracken.jp>

Update shell/platform/darwin/macos/framework/Source/FlutterSurfaceMan…

e25e502

…agerTest.mm Co-authored-by: Chris Bracken <chris@bracken.jp>

Update shell/platform/darwin/macos/framework/Source/FlutterSurfaceMan…

117b15c

…ager.h Co-authored-by: Chris Bracken <chris@bracken.jp>

Update shell/platform/darwin/macos/framework/Source/FlutterSurfaceMan…

6bab84a

…ager.h Co-authored-by: Chris Bracken <chris@bracken.jp>

Update shell/platform/darwin/macos/framework/Source/FlutterSurfaceMan…

10275f7

…ager.h Co-authored-by: Chris Bracken <chris@bracken.jp>

Fix build error

849a273

Address nits

71649f3

Replace EVent with fml::AutoResetWaitableEvent

77b5209

Remove unecessary CATransaction

768aca8

knopp force-pushed the macos_rendering_refactor branch from beb61ac to 768aca8 Compare December 13, 2022 19:12

Add GetRequiredFrameSize

f981e60

cbracken approved these changes Dec 13, 2022

View reviewed changes

knopp merged commit 6cd8561 into flutter:main Dec 14, 2022

knopp deleted the macos_rendering_refactor branch December 14, 2022 19:41

engine-flutter-autoroll mentioned this pull request Dec 14, 2022

Roll Flutter Engine from 0a6a4a58f4f7 to db5605ea7115 (11 revisions) flutter/flutter#117109

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2022

6cd8561 [macOS] Refactor rendering infrastructure (flutter/engine#37789)

b32ff89

CaseyHillers added a commit that referenced this pull request Dec 15, 2022

Revert "[macOS] Refactor rendering infrastructure (#37789)"

1b318f0

This reverts commit 6cd8561.

CaseyHillers mentioned this pull request Dec 15, 2022

Revert "[macOS] Refactor rendering infrastructure" #38332

Closed

tgucio mentioned this pull request Dec 19, 2022

Moving a Flutter desktop window between screens can fail flutter/flutter#117247

Closed

knopp mentioned this pull request Dec 19, 2022

[macOS] Update FlutterView layer scale when backing properties change #38402

Merged

8 tasks

knopp mentioned this pull request Jan 11, 2023

macOS desktop app freezes on close flutter/flutter#118315

Closed

imWildCat mentioned this pull request Apr 10, 2023

[macOS][platform_view] Click event was not dispatched to PlatformView flutter/flutter#124492

Closed

Conversation

knopp commented Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High level overview

Fixes

Pre-launch Checklist

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

knopp commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skia-gold commented Dec 1, 2022

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

knopp commented Nov 20, 2022 •

edited

Loading

knopp commented Nov 21, 2022 •

edited

Loading