An internal customer is seeing flickering on some high FPS phones.
While we try and reproduce / gain more knowledge of this problem this PR adds a AndroidManifest.xml flag to disable their usage.
The boolean flag is:
```
"io.flutter.embedding.android.DisableImageReaderPlatformViews"
```
Adding the following to your manifest will disable their usage:
```
<meta-data
android:name="io.flutter.embedding.android.DisableImageReaderPlatformViews"
android:value="true" />
```
This PR refactors `Rasterizer` so that it's more suitable for multi-view.
Design doc: [flutter.dev/go/multi-view-pipeline-and-rasterizer](http://flutter.dev/go/multi-view-pipeline-and-rasterizer)
With this change, `Rasterizer::DrawToSurfaces` has a structure that can handle drawing to multiple views, although the lack of some functionality still blocks it, mostly related to `ExternalViewEmbedder` and `RasterCache`.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Due to https://github.com/flutter/flutter/issues/127500 , we can get in a state where enable-impeller is true but we're using Skia. We need to either fall back completely to Skia, make this configuration fatal, or remote the check
----------------
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.
Fixes https://github.com/flutter/flutter/issues/133204
Reverts flutter/engine#45418
Some google3 tests are hitting the CHECK I added in the DlSkCanvasDispatcher::drawTextFrame, which indicates that the SkParagraph code likely thinks impeller is enabled, whereas other code might be running with Skia.
Perhaps this could happen if its software rendering? It should be a fatal error on startup so we can track this down.
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,
```
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.
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.
Resolves https://github.com/flutter/flutter/issues/130947.
- Allow for setting the text renderer (`TextRenderContext`) when the
`AiksContext` is built. Previously, the text renderer was selected at
build time through linking a `TextRenderContext::Create` symbol.
- Support using Impeller without a text backend. Throw a clear error
when trying to render text if no text backend is present.
- Don't track the Impeller context in `TextRenderContext`. Just let the
renderer supply its context when generating atlases.
- Allow for overriding the `TextRenderContext` for individual Aiks
playground tests/goldens.
DisplayListBuilder grew over time from a class that implemented a developer-unfriendly stateful API into one that implemented both the developer-friendly DlCanvas API as well as its original stateful API. Over time, the stateful API was buried under a "testing only" facade.
In the meantime, the optimization features that it applies to the DlCanvas calls before it records the operations in a DlOp store are useful without the recording process and so I've been wanting to break those into 2 parts for a while with the goal of removing all stateful APIs from DisplayListBuilder (see https://github.com/flutter/flutter/issues/108303).
This PR takes a major step along that direction by splitting DisplayListBuilder into essentially a convenience class that marries 2 new classes together to achieve its old functionality:
- `DlCanvasToReceiver` - a class that implements DlCanvas, optimizes common situations, and then sends commands to any object that implements `DlOpReceiver`
- `DlOpRecorder` - an implementation of DlOpReceiver that records the operations in a buffer from which to create a `DisplayList` object
- `DisplayListBuilder` now inherits from DlCanvasToReceiver to get the optimizations and provides it with an instance of `DlOpRecorder` as the receiver that it will send its results to
- Similarly, a `DlCanvasToReceiver` instance could be directed to an `impeller:DlDispatcher` to achieve a more straight-through path from the DlCanvas interface to impeller Pictures.
Fixes https://github.com/flutter/flutter/issues/130141
The primary goal of this patch is to move dispatching of `dart:ui` `Canvas` commands to the UI thread.
Before this patch, the architecture is something like:
## UI Thread
- `dart:ui` talks to `DisplayListBuilder`, a `DlCanvas` implementation.
- `DisplayListBuilder` does some clip/bounds tracking and creates a `DisplayList` object that is held by `dart:ui`'s `Picture` objects.
- `DisplayList`s are added to `DisplayListLayer`s in `flow`.
## Raster Thread
- `flow` flattens the various operations into a single `DisplayList` via another `DisplayListBuilder`.
- A `DlOpReceiver`implementation converts that `DisplayList` into an `Aiks` `Canvas`/`Picture`.
After this patch, the architecture instead looks like:
## UI Thread
- No change for Skia.
- If Impeller, use a new `DlCanvasImplementation` that talks to `Aiks`'s `Canvas`.
- If Impeller, `dart:ui` Picture's now hold an `Aiks` `Picture`, which get shared into `AiksLayer`s in `flow`.
## Raster thread
- No change for Skia, but some light refactoring for places that assumed a `DisplayListBuilder` where they really just needed a `DlCanvas`.
- The `Aiks` `Picture`s are combined using new API on `DlCanvas` and still backed by `Aiks`.
These changes show significant improvement on raster times on Android and only very small regressions on UI times in local testing, see https://gist.github.com/dnfield/26528090194c9f5abdbac13cdcbf4f79 for old gallery transition perf numbers.
Many of the other changes in this patch are related to the following:
- Making `DlRTree` usable for Impeller.
- It would be nice to have a version of DlRTree that speaks `impeller::Rect`.
- Creating the requisite classes to support `EmbeddedViews` so that Desktop works.
This patch does not remove the `impeller::DlDispatcher`, which now would only be used in tests.
We have tests today that are trying to testwith impeller but using software rendering, so they're not actually testing with impeller.
In an unrelated patch, I'm causing the request to enable impeller to have additional checks that Impeller related objects/configurations are set when using Impeller, which isn't true if software rendering is enabled, and is causing the tests getting disabled here to fail.
The tests are not actually testing Impeller and thus are getting added to the skip list (they now crash with this change). https://github.com/flutter/flutter/issues/131888 tracks re-enabling them. They will almost certainly need different golden files than the Skia ones.
Fixes https://github.com/flutter/flutter/issues/131887
Fixes the following errors when building with a C++20 toolchain.
I don't really know C++ that well so any pointers would be appreciated.
For the change to the lambda, we can't follow the recommendation to capture `*this` explicitly because that wouldn't compile under C++17. This PR changes the semantics slightly to capture `surface_` by reference, which might be still okay?
For b/289776142
```
error: implicit capture of 'this' with a capture default of '=' is deprecated [-Werror,-Wdeprecated-this-capture]
93 | if (surface_) {
| ^
note: add an explicit capture of 'this' to capture '*this' by reference
91 | raster_thread_merger_->SetMergeUnmergeCallback([=]() {
| ^
| , this
```
```
error: bitwise operation between different enumeration types ('CGImageAlphaInfo' and '(unnamed enum at third_party/apple_sdks/xcode_14_2/iphonesimulator/System/Library/Frameworks/CoreGraphics.framework/Headers/CGImage.h:53:9)') is deprecated [-Werror,-Wdeprecated-anon-enum-enum-conversion]
159 | static_cast<CGBitmapInfo>(kCGImageAlphaPremultipliedLast |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
160 | kCGBitmapByteOrder32Big), // CGBitmapInfo bitmapInfo
| ~~~~~~~~~~~~~~~~~~~~~~~
```
Fixes https://github.com/flutter/flutter/issues/129862
This reverts commit cd52c0ce82b37bc9d16bea26fbd1753174c49e42.
These changes were exposing an underlying bug in the DisplayListMatrixClipTracker that has now been fixed independently (https://github.com/flutter/engine/pull/43664). There are no changes to the commit being relanded here, it has been tested on the Wondrous app which demonstrated the regression before the NOP culling feature was reverted and it now works fine due to the fix of the underlying cause.
The change in [0] has propagated now everywhere, so we can use
`utf8.encode()` instead of the longer `const Utf8Encoder.convert()`.
Also it cleans up code like
```
Uint8List bytes;
bytes.buffer.asByteData();
```
as that is not guaranteed to be correct, the correct version would be
```
Uint8List bytes;
bytes.buffer.asByteData(bytes.offsetInBytes, bytes.length);
```
a shorter hand for that is:
```
Uint8List bytes;
ByteData.sublistView(bytes);
```
[0] https://github.com/dart-lang/sdk/issues/52801
Actually works now, though there is some issue with the default fbo stencil so I've filled https://github.com/flutter/flutter/issues/130048
Other issues:
* ~~Rendering looks wrong~~
* ~~Resizing window hangs~~
* ~~Reactor isn't set up correctly and all blit passes are currently failing.~~
* ~~Needs to handle falling back to sample count of 1 like we do on Android~~.
