Closes https://github.com/flutter/flutter/issues/132860.
- If `setDither(true)` is called, and an existing `setColorSource` is a gradient, it is ignored.
- If `setColorSource(...)` is called, and it is a gradient, and dithering was previously set, it is cleared.
I'm not sure this is fool proof.
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.
In the Impeller backend, we **only** support dithering of _gradients_. In addition, it will be the default (and only option).
In the [process of enabling dithering by default](https://github.com/flutter/engine/pull/44705), i.e.
```diff
class Paint {
- static bool enableDithering = false;
+ static bool enableDithering = true;
}
```
... we realized with internal Google testing this will now apply dithering on more than just gradients, i.e. images in the Skia backend. Since we won't support dithering of images in the Impeller backend, this PR gives a "hint" on whether the `colorSource` (if one is set) can be dithered by the contrived rules we've created.
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.
When a saveLayer is rendered with an ImageFilter that modifies the bounds of the rendered pixels, and some of the content of that saveLayer did not intersect the clip, but the filtered output of that content did intersect the clip, we might not accumulate the bounds of those rendering operations into the DisplayList bounds.
This bug was not encountered during practical testing, but showed up on some testing with the new NOP culling code.
For now, this bug only affects the accuracy of the reported bounds of the DisplayList, but that can affect raster caching and potentially the layer culling done in the LayerTree. It might also affect the accuracy of the RTree associated with the DisplayList, which would only show up in rare circumstances when the indicated operation was used in the presence of an embedded view.
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.
Most of the #include directives for SkPicture are removed except where they are still functional. Many comments rewritten to no longer be SkPicture-centric.
- DL unit tests still use it for consistency testing
- rasterizer/engine still use it for screen shot support
- Fuchsia still uses it extensively
Addresses most of https://github.com/flutter/flutter/issues/128060
Most of the uses of SkPicture and Recorder are removed from the engine sources. The few that remain are:
- DisplayList <-> Skia consistency testing code
- Legacy code only used from Fuchsia
- Dart CanvasKit uses which aren't actually using the local Skia sources or libraries
These are all comment and include file changes and so the testing is in the building.
Reverts flutter/engine#42584. (Thanks to @jonahwilliams for bisecting)
With this change, layers are getting clipped incorrectly when rendering
platform views in Wondrous.
Update flutter engine includes to be more specific about use of Skia includes.
These changes are required to unblock the Skia roller that has new streamlined include files.
Fixes https://github.com/flutter/flutter/issues/128412
Adds
- `DlRegion DlRegion::MakeUnion(const Region &, const DlRegion &)`
- `DlRegion DlRegion::MakeIntersection(const Region &, const DlRegion
&)`
- `bool DlRegion::intersects(const DlRegion &)`
- `bool DlRegion::intersects(const SkIRect &)`
Instead of per span line vector all spans are stored in continuous
buffer.
Complete benchmarks:
```
-----------------------------------------------------------------------------------------------
Benchmark Time CPU Iterations
-----------------------------------------------------------------------------------------------
BM_DlRegion_IntersectsSingleRect/Tiny 2688 ns 2687 ns 258580
BM_SkRegion_IntersectsSingleRect/Tiny 85889 ns 85877 ns 8092
BM_DlRegion_IntersectsSingleRect/Small 4814 ns 4813 ns 142874
BM_SkRegion_IntersectsSingleRect/Small 101102 ns 101102 ns 6833
BM_DlRegion_IntersectsSingleRect/Medium 2329 ns 2329 ns 302911
BM_SkRegion_IntersectsSingleRect/Medium 60436 ns 60183 ns 11156
BM_DlRegion_IntersectsSingleRect/Large 1243 ns 1243 ns 565209
BM_SkRegion_IntersectsSingleRect/Large 2813 ns 2813 ns 252187
BM_DlRegion_IntersectsRegion/Tiny 38.9 ns 38.9 ns 17913855
BM_SkRegion_IntersectsRegion/Tiny 203 ns 203 ns 3480855
BM_DlRegion_IntersectsRegion/Small 306 ns 306 ns 2295413
BM_SkRegion_IntersectsRegion/Small 1057 ns 1057 ns 660826
BM_DlRegion_IntersectsRegion/Medium 8.83 ns 8.83 ns 79128233
BM_SkRegion_IntersectsRegion/Medium 43.3 ns 43.3 ns 16076912
BM_DlRegion_IntersectsRegion/Large 6.96 ns 6.96 ns 101646676
BM_SkRegion_IntersectsRegion/Large 31.8 ns 31.8 ns 22121517
BM_DlRegion_IntersectsRegion/TinyAsymmetric 54.2 ns 54.2 ns 12890870
BM_SkRegion_IntersectsRegion/TinyAsymmetric 4575 ns 4574 ns 155368
BM_DlRegion_IntersectsRegion/SmallAsymmetric 190 ns 189 ns 3748547
BM_SkRegion_IntersectsRegion/SmallAsymmetric 6157 ns 6157 ns 114403
BM_DlRegion_IntersectsRegion/MediumAsymmetric 20.9 ns 20.9 ns 33523941
BM_SkRegion_IntersectsRegion/MediumAsymmetric 3247 ns 3247 ns 214694
BM_DlRegion_IntersectsRegion/LargeAsymmetric 8.97 ns 8.97 ns 76827676
BM_SkRegion_IntersectsRegion/LargeAsymmetric 154 ns 154 ns 4757924
BM_DlRegion_Operation/Union_Tiny 26.3 us 26.3 us 24534
BM_SkRegion_Operation/Union_Tiny 37.9 us 37.9 us 17973
BM_DlRegion_Operation/Union_Small 64.4 us 64.4 us 10657
BM_SkRegion_Operation/Union_Small 105 us 105 us 6278
BM_DlRegion_Operation/Union_Medium 22.0 us 22.0 us 31631
BM_SkRegion_Operation/Union_Medium 64.8 us 64.8 us 10744
BM_DlRegion_Operation/Union_Large 1.00 us 1.00 us 697406
BM_SkRegion_Operation/Union_Large 1.29 us 1.29 us 547089
BM_DlRegion_Operation/Union_TinyAsymmetric 10.3 us 10.3 us 68647
BM_SkRegion_Operation/Union_TinyAsymmetric 20.6 us 20.6 us 33282
BM_DlRegion_Operation/Union_SmallAsymmetric 14.0 us 14.0 us 49944
BM_SkRegion_Operation/Union_SmallAsymmetric 34.4 us 34.4 us 19618
BM_DlRegion_Operation/Union_MediumAsymmetric 5.24 us 5.24 us 134097
BM_SkRegion_Operation/Union_MediumAsymmetric 12.7 us 12.7 us 55069
BM_DlRegion_Operation/Union_LargeAsymmetric 0.376 us 0.376 us 1808589
BM_SkRegion_Operation/Union_LargeAsymmetric 0.533 us 0.532 us 1283674
BM_DlRegion_Operation/Intersection_Tiny 8.13 us 8.13 us 87199
BM_SkRegion_Operation/Intersection_Tiny 31.8 us 31.8 us 21864
BM_DlRegion_Operation/Intersection_Small 55.9 us 55.9 us 11888
BM_SkRegion_Operation/Intersection_Small 98.4 us 98.3 us 6963
BM_DlRegion_Operation/Intersection_Medium 40.0 us 40.0 us 17667
BM_SkRegion_Operation/Intersection_Medium 69.8 us 69.8 us 9910
BM_DlRegion_Operation/Intersection_Large 1.06 us 1.06 us 650957
BM_SkRegion_Operation/Intersection_Large 1.26 us 1.26 us 559624
BM_DlRegion_Operation/Intersection_TinyAsymmetric 2.62 us 2.62 us 264565
BM_SkRegion_Operation/Intersection_TinyAsymmetric 15.3 us 15.3 us 45528
BM_DlRegion_Operation/Intersection_SmallAsymmetric 7.15 us 7.15 us 93482
BM_SkRegion_Operation/Intersection_SmallAsymmetric 27.5 us 27.5 us 24450
BM_DlRegion_Operation/Intersection_MediumAsymmetric 2.95 us 2.95 us 235133
BM_SkRegion_Operation/Intersection_MediumAsymmetric 10.5 us 10.5 us 65925
BM_DlRegion_Operation/Intersection_LargeAsymmetric 0.165 us 0.165 us 4016433
BM_SkRegion_Operation/Intersection_LargeAsymmetric 0.409 us 0.409 us 1719716
BM_DlRegion_Operation/Intersection_SingleRect_Tiny 0.105 us 0.105 us 7403099
BM_SkRegion_Operation/Intersection_SingleRect_Tiny 10.8 us 10.8 us 64185
BM_DlRegion_Operation/Intersection_SingleRect_Small 0.410 us 0.410 us 1724524
BM_SkRegion_Operation/Intersection_SingleRect_Small 16.2 us 16.2 us 43707
BM_DlRegion_Operation/Intersection_SingleRect_Medium 0.458 us 0.458 us 1540049
BM_SkRegion_Operation/Intersection_SingleRect_Medium 7.54 us 7.54 us 93407
BM_DlRegion_Operation/Intersection_SingleRect_Large 0.175 us 0.175 us 3984926
BM_SkRegion_Operation/Intersection_SingleRect_Large 0.351 us 0.351 us 1931946
BM_DlRegion_FromRects/Tiny 154 us 154 us 4383
BM_SkRegion_FromRects/Tiny 69429 us 69419 us 10
BM_DlRegion_FromRects/Small 369 us 369 us 1932
BM_SkRegion_FromRects/Small 117584 us 117578 us 6
BM_DlRegion_FromRects/Medium 475 us 475 us 1477
BM_SkRegion_FromRects/Medium 21611 us 21610 us 33
BM_DlRegion_FromRects/Large 1329 us 1329 us 533
BM_SkRegion_FromRects/Large 1409 us 1409 us 501
BM_DlRegion_GetRects/Tiny 39.2 us 39.2 us 18030
BM_SkRegion_GetRects/Tiny 84.2 us 84.2 us 9971
BM_DlRegion_GetRects/Small 88.9 us 88.9 us 7873
BM_SkRegion_GetRects/Small 212 us 212 us 3598
BM_DlRegion_GetRects/Medium 0.845 us 0.813 us 881224
BM_SkRegion_GetRects/Medium 3.10 us 3.09 us 223483
BM_DlRegion_GetRects/Large 0.120 us 0.120 us 5954761
BM_SkRegion_GetRects/Large 0.337 us 0.336 us 2068656
```
## 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
Fixes: https://github.com/flutter/flutter/issues/128159
In diagnosing test failures for https://github.com/flutter/engine/pull/42330 we discovered that the DisplayList code was not always backwards compatible with computing the bounds of inverted rectangles (where left > right or top > bottom). Historically such rectangles were always rendered as if they were sorted (i.e. `SkRect::makeSorted()`), but we computed bounds as if the bounds only mattered if the supplied rectangle was ordered. So, we would sometimes render a rectangle for which we mis-computed the bounds.
This would rarely surface in the current code as most rendered rectangles would pass through `SkMatrix::mapRect()` which implicitly orders the rectangle as it transforms it, but any attributes applied to the bounds before that method may have been applied "in the wrong direction" - such as:
- stroke width padding
- mask blur padding
- image filter padding
Fixes https://github.com/flutter/flutter/issues/116070
Fixes https://github.com/flutter/flutter/issues/126202
Introduces `DlRegion` class which implements subset of `SkRegion`
required to get non-overlapping rectangles from region.
The implementation is different and faster than `SkRegion` for this
particular use-case (`display_list_region_benchmarks`):
Edit: Updated benchmark to latest revision and natively (initial run
went through rosetta)
```
----------------------------------------------------------------------------
Benchmark Time CPU Iterations
----------------------------------------------------------------------------
BM_RegionBenchmarkDlRegion/Tiny 616 us 616 us 908
BM_RegionBenchmarkSkRegion/Tiny 70559 us 70557 us 10
BM_RegionBenchmarkDlRegion/Small 1315 us 1314 us 537
BM_RegionBenchmarkSkRegion/Small 121736 us 121717 us 6
BM_RegionBenchmarkDlRegion/Medium 1079 us 1079 us 650
BM_RegionBenchmarkSkRegion/Medium 22039 us 22035 us 32
BM_RegionBenchmarkDlRegion/Large 399 us 399 us 1763
BM_RegionBenchmarkSkRegion/Large 1510 us 1510 us 466
```
## 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
The original PR caused some golden test failures down the line, likely due to bad analysis of when the combined BlendMode and color would result in a NOP situation.
This PR adds tests that go through every BlandMode and pair it with a variety of colors and Color/ImageFilters to verify that the operations are only omitted when they actually produce no change in the output. It also checks the validity of the "modifies_transparent_black" property of DisplayLists which can be used in place of the current CanvasSpy/DlOpSpy classes.
The description from the [previous PR](https://github.com/flutter/engine/pull/41463) updated with the new name of the DL property:
---------------------------------
This optimization avoids recording unnecessary render operations that will not affect the output and also eliminates the need for "draw detection" mechanisms like `DlOpSpy` and `CanvasSpy` by remembering if any non-transparent operations were included. The `DlOpSpy` unit tests were updated to check if the results from that object match the new `DisplayList::modifies_transparent_black()` method.
Fixes https://github.com/flutter/flutter/issues/125338
In addition, this change will unblock some other Issues:
- https://github.com/flutter/flutter/issues/125318
- https://github.com/flutter/flutter/issues/125403
Part of an ongoing set of efforts to address https://github.com/flutter/flutter/issues/106448
Move the checkerboard layer unit tests onto the DisplayList version of the paint contexts and fix some bugs in the reusability of the DisplayListBuilder that this migration uncovered.
This optimization avoids recording unnecessary render operations that will not affect the output and also eliminates the need for "draw detection" mechanisms like `DlOpSpy` and `CanvasSpy` by remembering if any non-transparent operations were included. The `DlOpSpy` unit tests were updated to check if the results from that object match the new `DisplayList::affects_transparent_surface()` method.
Fixes https://github.com/flutter/flutter/issues/125338
In addition, this change will unblock some other Issues:
- https://github.com/flutter/flutter/issues/125318
- https://github.com/flutter/flutter/issues/125403
Part of an ongoing set of efforts to address https://github.com/flutter/flutter/issues/106448
MockTexture now executes a call on the supplied DlCanvas object rather than creating its own list of MockCanvas-style draw calls. The TextureLayer tests now no longer use MockCanvas.
Part of an ongoing set of efforts to address https://github.com/flutter/flutter/issues/106448
The layer unittests have been using a MockCanvas class to record the painting of trees of layers and then testing for the expected output.
A while back a similar mechanism was created to compare DisplayList output and to print out a human-friendly version of the differences found, but it was only used in a few tests written at the time it was created and a few since then.
This is the first in a series of PRs that will move all the rest of the unit tests onto the new DL comparison mechanism, starting with the layer types that just do basic drawing. Some of the remaining layers will require creating new hooks in, for instance, the Texture registry, the performance overlay TextBlob generation, etc.
These tests break under Skia's new behaviors when evaluating the bounds of ill-defined filtering operations such as ColorFilters that affect even transparent pixels.
The Flutter engine is currently building with a backwards compatibility flag which we should be able to remove with the new versions of these tests.
Switching the calls to dispatch into an Impeller Dispatcher to use a cull rect to enable pre-culling of the out-of-bounds ops.
This change showed an improvement of around 2x on the rendering performance of the non-intersecting platform view benchmark, but that was measured without the recent changes to the destructive blend modes in Impeller renderer.
With this PR we no longer need to hold DisplayLists in GPUObject wrappers and they can be disposed instantly instead of queueing on the Unref thread.
This will definitely be a win for Impeller as none of the objects used in a frame now require queueing, but the performance impact on apps running on top of skia is less clear if they depend on a lot of images inside their DisplayLists that still need to be queued to be freed. After getting further in the work, it looks like only decoded images need to use the protected DlImage wrappers and most of those should survive many frames before they are disposed. That should hopefully leave very few unrefs happening per frame.
~There are 3 unit tests in `shell_unittests.cc` and `embedder_metal_unittests.mm` that are now GSKIP'd as they now invoke code that needs a fully initialized UIDartState in order to protect their images. I will look into fixing the tests and/or making the code they invoke provide protection without relying on UIDartState.~ (This looks to be fixed in the latest commit by simply not creating DlImageGPUs all over the source base and simply catching only those that end up in UI data structures. There is actually existing code in one of the modules that feeds ui.Image with an answer to wrap the image in a DlImageGPU if it has a skia image anyway, so most of these additional uses of DlImageGPU that were having trouble getting the Skia unref queue just didn't need it anyway.)
