Reverts flutter/engine#47825
Initiated by: zanderso
This change reverts the following previous change:
Original Description:
Looks like this was proactively added in https://github.com/flutter/engine/pull/20496, but never wired up to anything on any platform. It is also unused in framework and customer code; we never exposed this on e.g. MediaQuery.
Related framework PR: https://github.com/flutter/flutter/pull/138103 (Checks will fail until that PR is submitted).
The new `BUILD.gn` files in the Engine tree can't go under
`build/secondary` because Skia still has its own, and they'd be selected
first. So, this PR puts the new `BUILD.gn` files under `flutter/skia`.
As part of eliminating the Flutter buildroot
(https://github.com/flutter/flutter/issues/67373), we are moving all
third-party dependencies from //third_party to //flutter/third_party.
Once all third-party dependencies have been migrated, tooling and config
will be moved and the buildroot will be eliminated altogether.
No tests changed because there is no semantic change to this PR. This is
simply relocating a dependency.
When we enter hybrid composition mode we 'pause' the default RenderSurface (implemented by SurfaceView or TextureView) and swap to an ImageReader based RenderSurface.
When we return from hybrid composition mode we recreate and re-initialize the real RenderSurface as if it was being used for the first time.
This broke Platform Views in an internal app b/306122497 because we would incorrectly tell the texture to attach when it was never detached.
This CL changes the protocol so that when we return from hybrid composition mode we only swap the RenderSurface and do not re-create it. This avoids doing a bunch of unnecessary work and fixes the logic error of re-attaching textures that were never detached.
*Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.*
*List which issues are fixed by this PR. You must list at least one issue.*
*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Reverts flutter/engine#46171
Initiated by: jonahwilliams
This change reverts the following previous change:
Original Description:
The "rasterizer screenshot" VM service API uses the Skia software backend, even when running with impeller. At best, this leads to confusing half working screenshots that are missing images.
See https://github.com/flutter/flutter/issues/135052
We should disable this functionality when running with impeller. Perhaps this could be reworked to use picture toImage instead of the offscreen so that it uses the "real" backend regardless of the platform it is running on.
Fixes https://github.com/flutter/flutter/issues/135052
The "rasterizer screenshot" VM service API uses the Skia software backend, even when running with impeller. At best, this leads to confusing half working screenshots that are missing images.
See https://github.com/flutter/flutter/issues/135052
We should disable this functionality when running with impeller. Perhaps this could be reworked to use picture toImage instead of the offscreen so that it uses the "real" backend regardless of the platform it is running on.
Fixes https://github.com/flutter/flutter/issues/135052
This PR relands https://github.com/flutter/engine/pull/44473.
The previous PR was immediately reverted after merging because we found that the PR could cause illegal renders to be skipped on debug builds but crash the app on release builds. This PR makes the `Animator::Render` skip illegal renders as well. This should not be the final shape of this feature, and thus a TODO is added.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
As far as I can tell, this functionality has _always_ been falling back to software rendering with Skia when Impeller was enabled. This is at best extremely misleading. Since I started putting Impeller specific objects in the display list it introduced crashes, and must be disabled.
Fixes https://github.com/flutter/flutter/issues/136847
Trace GPU execution time on GLES using GL_EXT_disjoint_timer_query. This requires a per-app opt in from the Android Manifest with the key `"io.flutter.embedding.android.EnableOpenGLGPUTracing` set to true.
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
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.
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.
