This fixes the `WindowsTest.EngineCanTransitionToHeadless` flakiness reported by @matanlurey.
EGL surfaces can only be used by a single thread at a time. Concurrent operations are unsafe.
Previously, the EGL surface was destroyed on the platform thread. This was safe as this always happened after the engine was shutdown and the raster thread was stopped. However, in a multi-view world a view can be destroyed while the engine is running. There may be pending raster tasks that operate on the render surface. Thus, the EGL surfaces should be destroyed on the raster thread when it is available.
This bug was introduced by https://github.com/flutter/engine/pull/51681
Part of https://github.com/flutter/flutter/issues/142845
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Further work towards https://github.com/flutter/flutter/issues/141641.
I could imagine making things `presubmit: false` if we think it's not
worth the presubmit capacity.
Removed `:copy_dart_sdk` which seems (a) not to work and (b) not needed
to use the pre-builts.
Closes https://github.com/flutter/flutter/issues/145957.
As @jonahwilliams and @johnmccutchan and I discussed (https://github.com/flutter/flutter/issues/144407), the functionality that was being tested was actually _Android's_ ability to rotate and crop `SurfaceTexture`-backed textures. This same functionality doesn't even exist in the `ImageReader`-based textures (read: modern Android devices):
> Due to an oversight by Android, ImageReader backed surfaces do not respect metadata applied to the surface (rotation & crop). Rotation information is not available at all and crop information is corrupted by the ImageReader (only the width/height is propagated the origin offset is not).
We might decide to re-add this functionality in the Dart `Texture` widget, but given we'll be migrating our plugins to `SurfaceProducer` (again, read: using `ImageTexture` for most Android phones), it's pointless to test this (and isn't even testing Flutter's code).
This reduces our test suite significantly (8 tests down to 2), which should also help with runtime and flakiness.
/cc @zanderso who I'm sure will be stoked.
Work towards https://github.com/flutter/flutter/issues/133569.
This PR is a proof of concept that shows we're able to use `package:test` in `flutter/engine` instead of `package:litetest`.
I think it also shows that, if we're going to continue to vend dependencies this way, we might want to re-think our strategy in terms of using `pub` as a management tool - it's quite unwieldy already. For example, here is every `pubspec.yaml` file in the repo:
```sh
$ find . -name 'pubspec.yaml' -exec sh -c 'echo "$0 $(wc -l < "$0")"' {} \;
# Some files omitted in third_party or similar.
./impeller/tessellator/dart/pubspec.yaml 11
./tools/const_finder/pubspec.yaml 35
./tools/api_check/pubspec.yaml 90
./tools/build_bucket_golden_scraper/pubspec.yaml 47
./tools/licenses/pubspec.yaml 53
./tools/path_ops/dart/pubspec.yaml 26
./tools/engine_tool/pubspec.yaml 76
./tools/dir_contents_diff/pubspec.yaml 19
./tools/compare_goldens/pubspec.yaml 3
./tools/golden_tests_harvester/pubspec.yaml 55
./tools/gen_web_locale_keymap/pubspec.yaml 37
./tools/githooks/pubspec.yaml 63
./tools/android_lint/pubspec.yaml 35
./tools/clang_tidy/pubspec.yaml 76
./tools/pkg/engine_repo_tools/pubspec.yaml 41
./tools/pkg/process_fakes/pubspec.yaml 36
./tools/pkg/engine_build_configs/pubspec.yaml 73
./tools/pkg/git_repo_tools/pubspec.yaml 60
./tools/header_guard_check/pubspec.yaml 70
./sky/packages/sky_engine/pubspec.yaml 8
./shell/vmservice/pubspec.yaml 8
./ci/pubspec.yaml 57
./testing/benchmark/pubspec.yaml 77
./testing/skia_gold_client/pubspec.yaml 66
./testing/pkg_test_demo/pubspec.yaml 116
./testing/smoke_test_failure/pubspec.yaml 31
./testing/dart/pubspec.yaml 71
./testing/android_background_image/pubspec.yaml 22
./testing/litetest/pubspec.yaml 33
./testing/symbols/pubspec.yaml 24
./testing/scenario_app/pubspec.yaml 67
./web_sdk/web_engine_tester/pubspec.yaml 14
./web_sdk/web_test_utils/pubspec.yaml 22
./web_sdk/pubspec.yaml 60
./lib/snapshot/pubspec.yaml 8
./lib/gpu/pubspec.yaml 14
./lib/web_ui/pubspec.yaml 60
./flutter_frontend_server/pubspec.yaml 39
```
I'll file a follow-up issue to discuss pub-package management in the engine.
Work towards https://github.com/flutter/flutter/issues/144835.
Doc (_sorry, internal only_): [go/flutter-engine-goldens-workflow](http://goto.google.com/flutter-engine-goldens-workflow).
This implements the majority of the proposed workflow, that is, optionally having a plain-text version at the root of the directory, and using it to apply a unique suffix we can review in release branches. As it stands, this is a NO-OP outside of tests (it will have no impact, and can be ignored).
What's missing before using this feature in release branches:
- Optimization work with the infra team (not sure if blocking or not):
https://github.com/flutter/flutter/issues/145842
- A dry-run of this with the release team to make sure it works as intended
@gaaclarke As implemented, I _think_ we don't need anything special for [`dir_contents_diff`](286169bb52/tools/dir_contents_diff), but maybe I'm wrong - I think only the _test_ names are being changed, not the names on disk.
/cc @zanderso as well.
Uses the mipmap implementation from https://vulkan-tutorial.com/Generating_Mipmaps .
Today we generate all mip levels from mip level 0, which results in lost data. Instead we need to use the previous mip level
This fixes https://github.com/flutter/flutter/issues/141639
Most of this was previously unimplemented. It turns out the reason for the hang described in the github issue was that there was a typo in the name of the `getMiterLimit` C function, so if the client actually called that method the Wasm module failed to compile, as it couldn't find an import with the misspelled name.
Previously the keyboard was initialized after the view is created. This used to be necessary as the keyboard & text input plugins were strongly tied to a view (and would crash in headless modes). This is no longer necessary, and the keyboard can now be initialized normally as part of the engine initialization.
Part of https://github.com/flutter/flutter/issues/142845
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Currently destroying a view also shuts down the engine. This makes sense as in single-view world where the view also always owns the engine. However, in a multi-view world the views will share the engine. Destroying one view shouldn't necessarily shut down the engine unless that view owns the engine.
Part of https://github.com/flutter/flutter/issues/142845
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
This PR groups per-view information in `FlutterCompositor` into a private class, `ViewPresenter`. This makes it easier to manage per-view data and write view operations.
Part of https://github.com/flutter/flutter/issues/145874.
Currently, view presenters are never removed once created, since the macOS runner doesn't support removing views. This will be added in the future.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Closes https://github.com/flutter/flutter/issues/141641.
Basically, this should verify "it's possible to use VSCode+LSC, at least in theory".
I'm open to writing a test, but given it _is_ a test I'm less sure it's valuable. Feel free to push back.
Closes https://github.com/flutter/flutter/issues/144064.
From a conversation with @camsim99 on Discord:
> Ah this was to debug that missing emulator issue (https://github.com/flutter/flutter/issues/137947) that Ricardo and I were seeing. We wanted to see if it was specific to the API 34, and if so, escalate the issue internally. @ricardoamador Would you like to keep running the tests with API 33? You mentioned you have found a workaround, and the test runs seem to not have yielded any evidence against API 34.
Relands https://github.com/flutter/engine/pull/51589
The fix is in 74397bc171c74d2bfb24e82b47f2aa29d70c1711. I couldn't
figure out how to get a test in the engine to cover it. The test is in
the devicelab.
Here's what I attempted:
```c++
TEST_P(AiksTest, BlendModePlusAlphaColorFilterAlphaWideGamut) {
if (GetParam() != PlaygroundBackend::kMetal) {
GTEST_SKIP_("This backend doesn't yet support wide gamut.");
}
EXPECT_EQ(GetContext()->GetCapabilities()->GetDefaultColorFormat(),
PixelFormat::kR16G16B16A16Float);
Canvas canvas;
canvas.Scale(GetContentScale());
canvas.DrawPaint({.color = Color(0.1, 0.2, 0.1, 0.5)});
canvas.SaveLayer({
.color_filter = ColorFilter::MakeBlend(BlendMode::kPlus,
Color(Vector4{1, 0, 0, 0.5})),
});
Paint paint;
paint.color = Color(1, 0, 0, 0.5);
canvas.DrawRect(Rect::MakeXYWH(100, 100, 400, 400), paint);
paint.color = Color::White();
canvas.Restore();
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}
```
## 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 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
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[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
I tested against a few example apps and found this case is actually very common. These full screen/pass clips render nothing to the depth buffer can be particularly troublesome in the presence of backdrop filters (due to clip replay).
While this case doesn't actually result in anything getting written to the depth buffer, we still end up overwriting the entire stencil buffer twice: Once for the preparation draw, and again for the depth buffer transfer/stencil cleanup draw.
I had failed to take position affinity into account when calculating word boundaries on skwasm. This brings skwasm's behavior in line with canvaskit's.
At some point, we inherited a change from the repo-level `analysis_options.yaml` that [allowed deprecated](https://github.com/flutter/engine/pull/50575) members to be used. We rely on that analyzer error to prevent `solo: true` from being committed in our tests ([example](c935c3ba36/lib/web_ui/test/ui/line_metrics_test.dart (L179)) of one that slipped recently).
This PR overrides the `deprecated_member_use` error to enable it inside the web engine.
This PR connects the view ID provided by the rasterizer to `EmbedderExternalViewEmbedder`'s presenting call (see `embedder_external_view_embedder.cc`).
This PR also contains a refactor (which actually takes the majority of LOC) that moves the specification of view ID from `PrepareFlutterView` to `SubmitFlutterView`. The `flutter_view_id` parameter in `PrepareFlutterView` was added in https://github.com/flutter/engine/pull/46169. It turns out that we don't need the view ID throughout the preparation phase, so we can delay the specification to submission, which makes it feel cleaner.
### Tests
Existing tests are changed to verify that `SubmitFlutterView` receive the correct view ID.
The feature that `EmbedderExternalViewEmbedder` sends the view ID to the presenting API is not tested for now, because `EmbedderExternalViewEmbedder` is typically tested in `embedder_unittests`, but it requires the embedder API `AddView`. It will be tested in later changes to `EmbedderExternalViewEmbedder`.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
