This CL restructures Flatland vsync loop to fire for each vsync instead
of each OnNextFrameBegin. As shown in the traces attached to the bug,
the current implementation of firing callbacks on each OnNextFrameBegin
causes skips when Flutter has longer draw calls. By scheduling frames in
between, we are increasing the chance of sending one before the latch
point. OnNextFrameBegin is now used to keep track of present credits and
future presentation times as well as when to start frame, replacing the
need for max_frames_in_flight and vsync_offset fields.
Bug: b/296272449
These directly copy the iOS APIs, to minimize the branching needed in plugins with shared implementation code, and to facilitate the long-term goal of merging the iOS and macOS plugin headers. This does mean replicating the unfortunately non-idiomatic behavior of having `valuePublishedByPlugin:` sometimes return `nil` and sometimes return `NSNull`, instead of distinguishing between `nil` cases (if that's actually even necessary here) via a more specific API. In isolation I would definitely not design the API with this behavior, but consistency with iOS is the more important factor.
(Eventually I think we'll need a sort of "v2" of iOS plugin APIs since there are a number of strange behaviors that we're currently stuck with, but migrating iOS and macOS together to a new set of APIs won't be any harder than doing just iOS, and in the short to medium term consistency will help the ecosystem more that trying to pre-create better APIs as macOS-only.)
Also fixes `FlutterEngineRegistrar` to have a weak pointer to the engine. This should really already have been the case since plugins can retain the registrar, creating a likely cycle; it's now a guaranteed cycle (and failed unit tests designed to find cycles) without that since the engine itself is now keeping references to them.
Fixes https://github.com/flutter/flutter/issues/124721
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
This PR enforces the rules as documented in `FlutterView.Render`, where
calls in illegal situations should be ignored - but have never been
enforced.
```
/// This function must be called within the scope of the
/// [PlatformDispatcher.onBeginFrame] or [PlatformDispatcher.onDrawFrame]
/// callbacks being invoked.
///
/// If this function is called a second time during a single
/// [PlatformDispatcher.onBeginFrame]/[PlatformDispatcher.onDrawFrame]
/// callback sequence or called outside the scope of those callbacks, the call
/// will be ignored.
```
This rule is very important to implementing multi-view without having to
introduce new APIs. However, currently these illegal calls are not
ignored, and historically many tests (especially integration tests) were
unknowingly running based on this fact. @goderbauer did great work by
eliminating these cases in g3, and it's time for us to make sure these
calls are ignored.
Most effort of this PR goes to unit testing the changes. Some part of
`Shell::Create` is extracted into a static function to avoid duplicate
code.
## 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].
<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
Conversion of SkTextBlobs to impeller::TextFrame objects is one of the most expensive operations in display list dispatching. While the rest of the engine and framework makes a reasonable attempt to cache the SkTextBlobs generated during paragraph construction, the design of the dl dispatcher means that these the Impeller backend will always reconstruct all text frames on each frame - even if the display list/picture that contained those text frames was unchanged.
Removing this overhead is one of the goals of https://github.com/flutter/engine/pull/45386 , however this patch is also fairly risky and will be difficult to land. As a more incremental solution, we can instead construct the impeller::TextFrame objects when performing paragraph painting and record them in the display list. This both moves the text frame construction to the UI thread and allows the framework/engine to cache unchanged text frames.
This also does not conflict with the dl_aiks_canvas patch directly, and is fine to land before or after it does. (though I'd argue we should land this first).
To compare the current performance levels, I ran the complex_layout_scroll perf test, since this is fairly text filled. On a Pixel 6 pro. Across several runs this is a fairly consistent ~1ms raster time improvement.
### Skia
```
"average_frame_build_time_millis": 1.497333333333333,
"90th_percentile_frame_build_time_millis": 2.038,
"99th_percentile_frame_build_time_millis": 17.686,
"worst_frame_build_time_millis": 23.095,
"missed_frame_build_budget_count": 3,
"average_frame_rasterizer_time_millis": 5.5078589743589745,
"stddev_frame_rasterizer_time_millis": 2.226343414420338,
"90th_percentile_frame_rasterizer_time_millis": 7.481,
"99th_percentile_frame_rasterizer_time_millis": 19.11,
"worst_frame_rasterizer_time_millis": 79.799,
"missed_frame_rasterizer_budget_count": 7,
"frame_count": 234,
"frame_rasterizer_count": 234,
"new_gen_gc_count": 10,
"old_gen_gc_count": 2,
```
### Impeller (ToT)
```
"average_frame_build_time_millis": 1.431575000000001,
"90th_percentile_frame_build_time_millis": 2.196,
"99th_percentile_frame_build_time_millis": 14.486,
"worst_frame_build_time_millis": 23.728,
"missed_frame_build_budget_count": 2,
"average_frame_rasterizer_time_millis": 6.536087499999999,
"stddev_frame_rasterizer_time_millis": 1.9902712500000004,
"90th_percentile_frame_rasterizer_time_millis": 9.705,
"99th_percentile_frame_rasterizer_time_millis": 14.727,
"worst_frame_rasterizer_time_millis": 17.838,
"missed_frame_rasterizer_budget_count": 1,
"frame_count": 240,
"frame_rasterizer_count": 240,
"new_gen_gc_count": 10,
"old_gen_gc_count": 2,
```
### Impeller (Patched)
```
"average_frame_build_time_millis": 1.4500167364016743,
"90th_percentile_frame_build_time_millis": 2.478,
"99th_percentile_frame_build_time_millis": 14.883,
"worst_frame_build_time_millis": 18.782,
"missed_frame_build_budget_count": 1,
"average_frame_rasterizer_time_millis": 5.023033333333336,
"stddev_frame_rasterizer_time_millis": 1.6445388888888894,
"90th_percentile_frame_rasterizer_time_millis": 7.814,
"99th_percentile_frame_rasterizer_time_millis": 13.497,
"worst_frame_rasterizer_time_millis": 15.008,
"missed_frame_rasterizer_budget_count": 0,
"frame_count": 239,
"frame_rasterizer_count": 240,
"new_gen_gc_count": 8,
"old_gen_gc_count": 0,
```
## Background
If the Windows system compositor is enabled, the Windows embedder disables the swap interval so that presenting to a surface does not block until the v-blank. If the Windows system compositor is disabled (which is possible on Windows 7), the Windows embedder enables swap interval to prevent screen tearing.
Updating the swap interval requires making the GL surface current, which we currently do on the platform thread. However, the raster thread also needs the GL surface for rendering. Our current version of ANGLE allows making a GL surface current on multiple threads. However, the latest version of ANGLE errors if a GL context is made current on multiple threads. This is causing the ANGLE roll to fail ([example](https://ci.chromium.org/ui/p/flutter/builders/try/Windows%20Engine%20Drone/203788/overview)).
## Solution
There's two fixes:
1. The GL context is released once the swap interval is updated. This allows the platform thread to set the initial swap interval at start up, before the raster thread is started. This fixes the ANGLE roll.
3. When the system compositor changes, the Windows embedder now switches to the raster thread before updating the swap interval. This ensures that the GL surface is only made current on the raster thread once the raster thread has started. I updated unit tests to cover this scenario and tested this manually on a Windows 7 machine.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Update: Blocked on https://github.com/flutter/engine/pull/44912 landing,
and merging into google3.
---
Partial work towards https://github.com/flutter/flutter/issues/112498.
_**tl;dr**: In Impeller's backend we intend to _always_ dither
gradients, and never allow any changes long-term (i.e., write your own
shader if you want different behavior) with the assumption that dithered
gradients look better most of the time, and don't typically hurt
elsewhere._
Note that, at the time of this writing, I couldn't find a single case of
this being set explicitly to `true` inside Google, and there are between
[100 and 200 publicly accessible on
GitHub](https://github.com/search?q=%22Paint.enableDithering%22+language%3ADart&type=code&l=Dart),
of which virtually all are setting it explicitly to `true`.
There are some (valid) concerns this would cause a lot of golden-file
image diffs, so I'm going to seek explicit approval from the Google
testing team as well as someone from the framework team before landing
this commit.
## Description
This PR fixes a mistake I made on https://github.com/flutter/engine/pull/44636 where the error handling code wrongly relied on `fl_method_channel_invoke_method_finish` instead of `fl_binary_messenger_send_on_channel_finish`.
The error handling code was not called when running the tests added in https://github.com/flutter/engine/pull/44636 so this mistake did not pop up.
@robert-ancell I added a test that simulates an error response and I had to rely on `g_idle_add` to make it works. Is this approach ok?
## Related Issue
Linux implementation for https://github.com/flutter/flutter/issues/132386
## Tests
Adds one test.
Links in an implementation of _availability_version_check on macOS.
This is required due to an upstream compiler builtin (runtime) change
that marked this function as weakly-linked via `__attribute__((weak import))`
As such, no linking failure occurs when the function is unavailable at
link time.
By providing an implementation, the linker picks up our implementation,
which looks up symbol in question at runtime (via dlsym) on the first
invocation, caches it for later invocations, then invokes it. This is,
in fact, precisely what the original clang builtin implementation did.
Upstream clang change: https://reviews.llvm.org/D150397
Issue: https://github.com/flutter/flutter/issues/133777
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
In https://skia-review.googlesource.com/c/skia/+/742797 Skia refactored
GrBackend* to not require #ifdefs. This changes
callsites in Flutter to use static functions instead of methods that
were conditionally compiled on those classes.
There should be no functional change.
Fixes https://github.com/flutter/flutter/issues/129073
## Changes to Embedder API
Introduced `FlutterRegion` to represent arbitrary region:
```cpp
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`:
```cpp
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`:
```cpp
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
https://github.com/flutter/flutter/assets/96958/09a75eee-316b-4d53-a8b4-d6bb4e1e52f7
## Pre-launch Checklist
- [X] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [X] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [X] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [X] I listed at least one issue that this PR fixes in the description
above.
- [X] 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.
- [X] I updated/added relevant documentation (doc comments with `///`).
- [X] I signed the [CLA].
- [X] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel
on [Discord].
<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
Relands https://github.com/flutter/engine/pull/45131
Fixes https://github.com/flutter/flutter/issues/132416
Differences from last time:
- Some minor merge conflict fixes
- Use the RTree to get the bounds instead of recalculating the bounds
- Make the iOS platform view controller implementation use the impeller-aware slices instead of the display list ones. This has been fixed for Android and the desktop embedding, but I missed iOS. The unit tests weren't actually running before I branched for my PR, @zanderso fixed them up separately and this resulted in catching the failures on post submit last time.
Sad Trombone. Because Flutter runs from a background thread, if we touch a CATransaction while there is a main thread transaction occuring then we'll trigger a framework error.
Fixes https://github.com/flutter/flutter/issues/133147
Reverts flutter/engine#45131
This is failing the Impeller variants of the unobstructed platform views tests:
https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20Production%20Engine%20Drone/132249/overview
```
Failing tests:
-[UnobstructedPlatformViewTests testPlatformViewsMaxOverlays]
-[UnobstructedPlatformViewTests testOneOverlay]
-[UnobstructedPlatformViewTests testOneOverlayPartialIntersection]
-[UnobstructedPlatformViewTests testTwoIntersectingOverlays]
-[UnobstructedPlatformViewTests testOneOverlayAndTwoIntersectingOverlays]
** TEST FAILED **
```
We've retried it a few times so I suspect this isn't a flake.
This is a reland of https://github.com/flutter/engine/pull/44248
Fixes https://github.com/flutter/flutter/issues/132416
Changes from last time:
- The `drawPoints` benchmark was failing to render anything meaningful because of an error in `AiksLayer` that resulted in an infinitely sized bounding rectangle poisoning the bounds with `NaN` (this happens, for example, with a `drawPaint` call, which that benchmark happens to use). Added a test covering this and filed https://github.com/flutter/flutter/issues/132770 to explore ways to avoid this in the future.
- There was a bug in `DlAiksCanvas::SaveLayer` where a `nullptr` `paint` but non-`nullptr` `backdrop` was failing to actually save the layer. This resulted in incorrect rendering.
- There was a bug in `impeller::Canvas::DrawPicture` that resulted in incorrect stencil depth counting. That was fixed separately by @bdero, but was the cause of incorrect rendering in some Wonderous screens.
- I've added a simple implementation for `AiksLayer::IsReplacing`. It does not currently compare as deeply as the `DisplayListLayer` version potentially does, but it is good enough to avoid the regression noted in https://github.com/flutter/flutter/issues/132071. That regression was on a benchmark that greatly benefits from partial repaint. With the new implementation, it still gains partial repaint where it previously did not. There is more work that can be done here, filed https://github.com/flutter/flutter/issues/133361 to track that work.
I merged but did not fully integrate the `DisplayListBuilder`/`CanvasToReceiver` work that @flar has done. I have a local experiment with that, but would prefer to see this land and run through the device lab so we get some better comparison numbers for which one performs better.
In the change here b653a2823f, an intentional ABI breaking change was introduced to the clang runtime library for macOS and iOS. That change caused a symbol requiring dynamic linkage to be exposed that triggers iOS App Store checks for usage of private API.
This PR resolves that issue by rolling clang forward and introducing a definition of `_availability_version_check`. The declaration with weak linkage in the clang runtime library [here](b653a2823f/compiler-rt/lib/builtins/os_version_check.c (L89)) will then be resolved against the definition introduced in this PR. Since the declaration in the clang runtime library will now be resolved by static linking, the Flutter dylib will no longer require it to be dynamically linked, and will therefore not trigger the App Store check for using private API.
The definition of `_availability_version_check` is implemented using the `dlsym` strategy used by the old version of clang [here](f9ac557567/compiler-rt/lib/builtins/os_version_check.c (L97)).
Fixes https://github.com/flutter/flutter/issues/132130
