This reverts commit 670b5bd8e714a1fd2bd137aa2c5b36760b8ce89b.
With https://github.com/flutter/engine/pull/46911, the Platform.script API will return a URI for the compiled temporary wrapper script generated by "flutter test". This will cause issues for tests that expect Platform.script to reflect the directory where "flutter test" was launched.
This addresses the problem in https://github.com/flutter/flutter/issues/12847 which changed slightly over time.
Today, `Platform.script` does not give an empty `file` URI, it gives something like `file://path/to/package/main.dart` _regardless of how the file is actually named_.
After this change, it will give the absolute path to the file being run under test.
So before this change, the new test would have a URI like
```
file:///Users/dnfield/src/flutter/engine/src/main.dart
```
And now it has
```
file:///Users/dnfield/src/flutter/engine/src/out/host_debug_unopt_arm64/gen/platform_test.dart.dill
```
This is going to be helpful in generating relative paths from the test file.
The Impeller ContextVK contains a ConcurrentMessageLoop whose threads may invoke Dart timeline APIs. The Dart APIs will create a thread-local object that will be deleted during thread shutdown. Therefore, these threads should not outlive the engine/Shell and Dart VM.
Previously, RunTester held the ImpellerVulkanContextHolder on the stack, and its reference to the ContextVK would be dropped while exiting the function after the Shell is destructed.
This PR moves the contents of the tester's ImpellerVulkanContextHolder out of the instance on the stack and into a lambda owned by the Shell.
It also reenables the flutter_tester Impeller tests in the run_tests script.
When objcopy is used to embed data into a linkable object file, that
object file will only have default bits set in its header for ABI etc..
If the linker doesn't cooperate by ignoring ABI mismatches on object
files without code, then linking will fail. This PR stops using objcopy
to create an object file that embeds icudtl.dat into the Android
embedder, and instead uses the `bin_to_assembly.py` script that we're
already using for Dart VM snapshot data.
Context in https://github.com/llvm/llvm-project/issues/68915
Instead, this PR copies the frontend server from the Dart SDK (whether prebuilt or not) into the location expected by internal engine tests and artifact construction. This PR also consolidates the three GN templates that invoked the frontend server down to one.
Framework presubs: https://github.com/flutter/flutter/pull/135836
Related: https://github.com/flutter/flutter/issues/60007
This patch does the following:
- Updates `flutter_tester` to set up an Impeller rendering context and surface if `--enable-impeller` is set to true, using the Vulkan backend with Swiftshader.
- Updates `run_tests.py` to run all tests except the smoke test (that one really has no rendering impact whatsoever) with and without `--enable-impeller`.
- Updates a few tests to work that were trivial:
- A couple tests needed updated goldens for very minor rendering differences. Filed https://github.com/flutter/flutter/issues/135684 to track using Skia gold for this instead.
- Disabled SKP screenshotting if Impeller is enabled, and updated the test checking that to verify an error is thrown if an SKP is requested.
- The Dart GPU based test now asserts that the gpu context is available if Impeller is enabled, and does not deadlock if run in a single threaded mode.
- We were missing some trace events around `Canvas::SaveLayer` for Impeller as compared to Skia.
- A couple other tests had strict checks about exception messages that are slightly different between Skia and Impeller.
- I've filed bugs for other tests that may require a little more work, and skipped them for now. For FragmentProgram on Vulkan I reused an existing bug.
This is part of my attempt to address https://github.com/flutter/flutter/issues/135693, although @chinmaygarde and I had slightly different ideas about how to do this.
The goals here are:
- Run the Dart unit tests we already have with Impeller enabled.
- Enable running more of the framework tests (including gold tests) with Impeller enabled.
- Run all of these tests via public `dart:ui` API rather than mucking around in C++ internals in the engine.
Switch the impeller testing in rendertests to use TextFrame objects rather than TextBlob objects when drawing to an Impeller-managed destination.
Previously the DrawText tests would generate over 200 errors because none of the attribute variants would render anything at all. After this fix the number of failures in the DrawText calls is down to 1 which points out that wrapping a DrawTextFrame call in a nothing saveLayer somehow affects the glyph positioning (in a not very appealing manner).
This PR creates a new test type for `run_tests.py` called `dart-host`.
The tests remaining under the `dart` type are tests that run in
`flutter_tester`. The `dart-host` tests run in the Dart CLI. This allows
`run_tests.py` to be simplified a bit, and while doing this I spotted a
couple of mistakes that were causing some tests not to run.
This PR also makes a couple of small style fixes to the build config
json files, converting tabs to spaces, and moving some "script" fields
before the "parameters" fields.
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.
TwoPlatformViewsWithOtherBackDropFilterTests is failing on macOS 13 with the same simulator version used in macOS 12. The image diff looks identical and slightly above the threshold. This PR adjusts the threshold for this test temporarily so our CI can run on both macOS 13 and 12. This change can be reverted when we move all our CI to macOS 13
Fixes https://github.com/flutter/flutter/issues/134740
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
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 changes run_tests.py to use the python logging library to report
results instead of direct prints or writes to stdout/stderr. This change
simplifies adding a `--quiet` flag that causes the script to only
generate output if a log is emitted at WARNING or above.
Overall this is a bit of progress toward landing something like
https://github.com/flutter/engine/pull/45595
flutter/flutter/#134220
Command required to update lock files required not using the custom task and required matching the version of gradle in ci and passing a custom version of java.
`~/.gradle/wrapper/dists/gradle-7.5.1-bin/7jzzequgds1hbszbhq3npc5ng/gradle-7.5.1/bin/gradle -D=org.gradle.java.home=/Library/Java/JavaVirtualMachines/zulu-11.jdk/Contents/Home app:dependencies --write-locks` for app:dependencies and dependencies.
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,
```
1. New Mac version seems to have updated the locale data, to match the mac change, we can dynamically read the data when testing instead of hard coding.
2. "we are sending a UIImage type as parameter to `XCTAttachment attachmentWithScreenshot:`, which takes a `XCUIScreenshot` as parameter, this is not supposed to pass in old iOS versions." In this PR we updated the parameter to use the `XCUIScreenshot` type.
Fixes https://github.com/flutter/flutter/issues/133783
[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.
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.
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.
As discussed offline, this is best deleted when Skia-gold is used for
all of our engine tests.
However, this will be useful for unblocking some PRs until then :)
See README.md for details!
Partial work towards re-landing #44936.
Both the `clang_tidy` and `githooks` passage could benefit from being
able to automatically find the latest `compile_commands.json` output,
which means that some common code should exist in the `tools/`
directory.
This is a very minimal (but tested) library for doing exactly that.
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.
The expected color values appear to match the output of Skia's raster
backend's blur. Historically, that doesn't support any tile mode other
than decal, so while the `makeBlur()` function defaulted to clamp
tiling, the output was decal and thus showed the blur fading to
transparent.
The test draws a 1x1 green rectangle in the center of a 3x3 image. Clamp
tiling would actually cause the output of the blur to just copy the
central green color to the remaining 8 pixels. This is the output of
Skia's GPU blur. I am working to land changes in Skia that make the
raster backend handle all tile modes, which then has it match the
existing GPU blur's behavior of a constant output for clamp tiling in
this test (so it then fails).
Decal tiling appears to be more useful for this test case anyways
because it creates per-pixel variations that can be validated against.
This is needed to land Skia-side fixes for skbug.com/40039877 and
skbug.com/40039025
## 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
