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
Closes https://github.com/flutter/flutter/issues/135057.
This is a fair bit more involved than previous changes, just due to the sheer number of implicit conversions.
Highlights:
- Made `public uint32_t argb` `private uint32_t argb_`, and added `argb()` instead.
- Added `ToSk(DlColor)` instead of using implicit conversions.
There were a bunch of places where I had to make a judgement call (particularly in tests) to keep the code a bit "messy", i.e. `DlColor(SK_RED)`, just to make the diff as small as possible and to prevent silly copy and paste bugs. I'd be open to filing a follow-up issue to reduce unnecessary wrapping.
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.
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.
This allows us to remove libpng from skia entirely, which saves us about 25kb brotli compressed.
Note, this should not change any functionality. The existing functionality is covered by the unit tests here: bfcb9d08e8/lib/web_ui/test/ui/image_golden_test.dart (L197-L197)
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.
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.
Reverts flutter/engine#43118
The incorrect default value (`true` instead of `false`) was used in the PR and that caused internal test failures. I'll add a test before trying to reland.
The goal is to remove the rounding applied in skparagraph and in the framework: https://github.com/flutter/flutter/issues/31707
The plumbing is done via a new static variable `ParagraphBuilder.shouldDisableRoundingHack` that toggles the rounding behavior in skparagraph and the flag is read by framework code. Application code and test code can either use `ParagraphBuilder.setDisableRoundingHack` or `--dart-define="SKPARAGRAPH_REMOVE_ROUNDING_HACK=1"` to opt-in.
Once the internal migration is finished the default value of the flag will be set to true.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
This implements https://github.com/flutter/flutter/issues/126340
For now, I am keeping this implementation simple without any extra caching. I may add caching later based on profiling results.
We want to start from a place of small size, and optimize from there with skwasm. Since there is no baseline expectation of performance at `-O3`, let's start with the smallest possible binary.
This implements full text rendering with the Skwasm renderer.
The font fallback logic has been refactored to decouple the font fallback manager from any CanvasKit-specific types and functions.
A chunk of CanvasKit text rendering tests have been ported over to the renderer-agnostic `ui` tests.
Fixes https://github.com/flutter/flutter/issues/126339
This implements font loading for the skwasm renderer.
In addition, it does some pretty major refactors:
1) Simplified the font collection interface to just have a single `loadAssetFonts` call, without the separate registration and debug fonts loading stuff
2) Debug fonts load now through http/asset mocking mechanisms instead of having a separate `downloadDebugTestFonts` call
3) Consolidated a few of our different unit test setup functions into a single `setUpUnitTests` function
These keep turning up, so I did a little bash-fu to find them all.
```bash
grep -lL "#pragma once" $(grep -lL "#ifndef .*_H_" $(find . | grep "\.h$")) | cut -c 3-
```
These subclasses were previously made very generic to work with both the
pre-sk_sp version of SkFontMgr and the using-sk_sp version of SkFontMgr.
Now that SkFontMgr uses sk_sp for return types, simplify the subclasses.
Skia is changing SkFontMgr and SkFontStyleSet methods to consistently
return sk_sp<SkTypeface> and sk_sp<SkFontStyleSet> instead of
SkTypeface* and SkFontStyleSet*. The pointers returned always needed to
be SkSafeUnref'ed but with sk_sp this ownership is now explicit.
Flutter subclasses both SkFontMgr and SkFontStyleSet and overrides
affected methods. Normally Skia would roll out this change behind a
build flag which would first be set in Flutter (to hold out the change),
Skia then rolled into Flutter, then the build flag removed from Flutter
(along with updating the subclasses). However, this is made quite
difficult and slow because of the need to also be compatible with
Flutter in other repositories at the same time. Instead, this change
updates the subclasses to infer the correct return types in a way that
will work both with and without the Skia change. After the Skia change
is landed and rolled into Flutter the subclasses will be re-simplified
to match the new method signatures.
[0] https://skia-review.googlesource.com/c/skia/+/659856
