Small cleanups to render_pass_gles to only set the viewport once or when it cahnges, instead of each draw. Removes scissor validation and pipeline unbind (which is slow due to hash table lookup).
The previous mock required knowing the specific functions used in the
binary messenger, this method instead allows test code to provide
complete platform channel implementation for testing and make simulated
platform channel calls into embedder code.
Incorrect reference counting of GTask objects meant platform channel
method calls would leave tasks alive that would leak memory and leave
unclosed references to the binary messenger.
The same problem with NVIDIA drivers which causes issue [152099](https://github.com/flutter/flutter/issues/152099) occurs with Vivante Corporation drivers.
Quick fix for issue on Vivante drivers:
https://github.com/flutter/flutter/issues/152099
- [ 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.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Fixes running x64 binaries on arm64 hosts, this fixes systems like Raspberry Pi's, Apple Silicon, and Ampere.
Before, I would get something like:
```
Checking GN formatting...
Completed checking 0 GN files with no formatting problems.
Checking Java formatting...
No Java files with changes, skipping Java format check.
Checking Python formatting...
All python files formatted correctly.
Checking for trailing whitespace on 1 source file...
No trailing whitespace found.
No header files with changes, skipping header guard check.
Checking C++/ObjC/Shader formatting...
ERROR: Formatter command '/home/ross/flutter-engine/src/flutter/buildtools/linux-x64/clang/bin/clang-format --style=file shell/platform/linux/fl_application_test.cc' failed with exit code 255. Command output follows:
qemu-x86_64: Could not open '/lib64/ld-linux-x86-64.so.2': No such file or directory
Completed checking 0 C++/ObjC/Shader files with no formatting problems
```
Now it just works, I also changed the shell script to use env since `/bin/bash` doesn't exist on NixOS so using env is a more portable solution.
*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
The wiki hasn't existed for a long time, and the URLs now require manually following through a click to the documents.
*List which issues are fixed by this PR. You must list at least one issue.*
closes https://github.com/flutter/flutter/issues/159514
*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
issue: https://github.com/flutter/flutter/issues/159177
The improvements come from using absl::flat_hash_map.
measurements with std vs absl (`host_profile_arm64`):
```
# Std
0.041124
0.0398002
0.0396546
0.0405552
0.0397742
# Absl
0.0353708
0.0336834
0.0338616
0.033050
0.0331976
```
Benchmark used
```c++
TEST(BufferBindingsGLESTest, BindUniformDataMicro) {
BufferBindingsGLES bindings;
absl::flat_hash_map<std::string, GLint> uniform_bindings;
uniform_bindings["SHADERMETADATA.FOOBAR"] = 1;
bindings.SetUniformBindings(std::move(uniform_bindings));
std::shared_ptr<MockGLES> mock_gl = MockGLES::Init();
MockAllocator allocator;
Bindings vertex_bindings;
ShaderMetadata shader_metadata = {
.name = "shader_metadata",
.members = {ShaderStructMemberMetadata{.type = ShaderType::kFloat,
.name = "foobar",
.offset = 0,
.size = sizeof(float),
.byte_length = sizeof(float)}}};
std::shared_ptr<ReactorGLES> reactor;
std::shared_ptr<Allocation> backing_store = std::make_shared<Allocation>();
ASSERT_TRUE(backing_store->Truncate(Bytes{sizeof(float)}));
DeviceBufferGLES device_buffer(DeviceBufferDescriptor{.size = sizeof(float)},
reactor, backing_store);
BufferView buffer_view(&device_buffer, Range(0, sizeof(float)));
vertex_bindings.buffers.push_back(BufferAndUniformSlot{
.slot =
ShaderUniformSlot{
.name = "foobar", .ext_res_0 = 0, .set = 0, .binding = 0},
.view = BufferResource(&shader_metadata, buffer_view)});
Bindings fragment_bindings;
int32_t count = 5'000'000;
auto start = std::chrono::high_resolution_clock::now();
for (int32_t i = 0; i < count; ++i) {
bindings.BindUniformData(mock_gl->GetProcTable(), allocator,
vertex_bindings, fragment_bindings);
if (i % 100 == 0) {
mock_gl->GetCapturedCalls();
}
}
auto end = std::chrono::high_resolution_clock::now();
auto duration =
std::chrono::duration_cast<std::chrono::microseconds>(end - start)
.count();
std::cout << "Execution time: " << duration / static_cast<double>(count)
<< " microseconds" << std::endl;
}
```
This is one of our hottest symbols on the raster thread:
<img width="1528" alt="Screenshot 2024-11-27 at 2 27 54 PM"
src="https://github.com/user-attachments/assets/a9029d6f-ee96-4612-83f1-6b69f24e6ce8">
## 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
Fixes https://github.com/flutter/flutter/issues/159578
When the screen is shut off, the atlas context is destroyed but the text frames are not. This can lead to a state where we expect the glyph atlas to be populated but it is not. make sure to check if the atlas itself is valid.
Applies non-null by default annotations to
FlutterPlatformViewsController with opt-outs where necessary. Updates unit tests to create graphics contexts where requried.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Should fix https://github.com/flutter/flutter/issues/159520 ~~but I still need to check locally.~~ Seems to do the trick.
All cmd bindings were copying the shader metadata, which meant allocating/de-allocating a lot of strings per draw.
This is a minor refactoring that moves a global bool to a boolean ivar in FlutterPlatformViewsController. The purpose of this variable is simply to avoid the overhead of trying to create backdrop filters if we've ever failed to create one in the past.
Given that this class will only ever have the one instance created and held by per engine with the same duration, and that most apps only ever have one engine, the performance win will be identical for most apps. For the few add-to-app cases with multiple engines either at once or over the course of an app's lifetime, the costs associated with firing up an engine are already a far bigger hit than those being saved by this bool.
Also migrates from C++ style namespace { ... } to Obj-C style static functions. These are entirely equivalent as both restrict symbols to the current translation unit.
This is a reland of https://github.com/flutter/engine/pull/56805, which was reverted as part of https://github.com/flutter/engine/pull/56817.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
This migrates PlatformViewController from C++ to Objective-C. Generally, we try to keep the embedder interfaces and components written in Objective-C except for the few places where C++ interfaces are requried to interface with engine APIs such as Shell and PlatformView (e.g. the PlatformViewIOS subclass). Now that the implementation is Objective-C, the class and file are renamed to match Objective-C naming conventions.
This allows us to take advantage of ARC and weak references, which eliminates the need for std::shared_ptr, fml::WeakPtr etc. Further, this eliminates some particularly unintuitive behaviour wherein this class was owned via a std::shared_ptr held by FlutterEngine, and injected into many other classes (e.g. AccessibilityBridge) via a std::shared_ptr& reference -- such that only one instance of the std::shared_ptr actually ever existed, presumably to avoid std::shared_ptr refcounting overhead. Given that this overhead was only incurred a single time at engine initialisation, this seems like overkill. One might ask why it wasn't therefore held in a `std::unique_ptr` and a `std::unique_ptr&` reference passed around. Likely, this was because we wanted to take a `fml::WeakPtr` reference on it.
Regardless, none of this is necessary any longer now that we can inject `__weak FlutterPlatformViewsController*` instances to classes that use it.
To be clear, this patch makes no attempt whatsoever to simplify or clean up the interface or implementation of this class. This class ties together far too many concepts and is injected into far too many places, and we should break it up and simplify it. However, the goal of this patch was simply to port to an Objective-C interface that plays nicely with the rest of the iOS embedder. This does include a couple minor cleanups in `#include`/`#import` order and usage to match our style guide.
This is a reland with a one-line fix of a lambda-capture block to ensure `self` and any local variables are captured by value rather than by reference:
* In the case where this method is called on the platform thread (i.e. where the UI and platform thread are merged), we use the latch to pause the calling thread until the lambda completes, in which case all locals could be passed by reference since the locals are guaranteed to hang around until the lambda completes and signals the latch.
* In the case where this method is called from the UI thread (i.e. where UI and platform thread are not merged), locals may have gone out of scope by the time the lambda executes, leading to undefined behaviour if passed by reference; thus we always pass by value to be sure; since `latch` must be shared between threads, it's passed held in a `std::shared_ptr` so the underlying latch/mutex is shared but it's kept live until it goes out of scope in both threads.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
While recently updating the DlColorSource sources I noticed some questionably implementation choices in the Color variant of the color sources.
I then realized that there was no public use of these classes (other than mostly their own unit tests) and so they should be deleted to focus on implementing the variants that are actually used by Flutter.
Changes the following shell callbacks to flush the dart event loop:
* OnPlatformViewSetViewportMetrics
* OnPlatformViewDispatchPointerDataPacket
* OnPlatformViewDispatchPlatformMessage
* OnPlatformViewSetSemanticsEnabled
* OnPlatformViewSetAccessibilityFeatures
Using a new TaskRunner API RunNowAndFlushMessages. If the task runner can run tasks on the current thread, this will immediately invoke a callback and then post an empty task to the event loop to ensure dart listeners fire.
Unlike https://github.com/flutter/engine/pull/56738, does not touch the vsync API - which looks like it depends on scheduling behavior today, at least for iOS.
Every text frame must inform the glyph atlas about the glyphs + scale it contains. When this happens, the glyph atlas will populate the glyph and then tell teh text frame about the location in the atlas, so that the text contents shader can sample it correctly.
Once this has been done once for a given text frame + scale + offset, we can actually just keep reusing the same data provided 1) the atlas itself hasn't changed and 2) the scale/offset/properties are the same.
This constitutes a nice CPU usage reduction in places where there is a lot of text that isn't being invalidated, like scrolling on the flutter gallery.
A new source code/header structure was introduced when the DlColorSource and DlImageFilter objects were migrated to Impeller geometry classes. Even though the DlColorFilter objects did not depend on Skia geometry objects, they need to be updated to the new source layout for consistency.
This is a combination of 3 reverts, required to get back to the revert that caused `ios_platform_view_tests` to start failing in the framework repo. In reverse chronological order, this reverts two trivial commits plus the non-trivial commit that likely caused the breakage:
* Revert "iOS: Eliminate global in platformviews controller (#56805)" This reverts commit cea4600caa7098fa7ec109d18b869db46cda726a.
* Revert "iOS: Delete FlutterPlatformViewsController.layerPoolSize (#56806)" This reverts commit 80fa8a590876e0d29055b9ddbfa8670c1f83759e.
* Revert "iOS: Migrate PlatformViewsController to Objective-C (#56790)" This reverts commit afd05afc406deb79fbe9c16684aeeeb19322b288.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Objective-C files should be named to match their header, in this case, //flutter/shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h.
When private API is required, we often create a header named FlutterFoo_Internal.h, but the implementation file is always just FlutterFoo.mm. This brings FlutterPlatformViews back into line with convention.
No test changes since this is just a file rename.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
This is a minor refactoring that moves a global bool to a boolean ivar in FlutterPlatformViewsController. The purpose of this variable is simply to avoid the overhead of trying to create backdrop filters if we've ever failed to create one in the past.
Given that this class will only ever have the one instance created and held by per engine with the same duration, and that most apps only ever have one engine, the performance win will be identical for most apps. For the few add-to-app cases with multiple engines either at once or over the course of an app's lifetime, the costs associated with firing up an engine are already a far bigger hit than those being saved by this bool.
Also migrates from C++ style namespace { ... } to Obj-C style static functions. These are entirely equivalent as both restrict symbols to the current translation unit.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
This migrates PlatformViewController from C++ to Objective-C. Generally, we try to keep the embedder interfaces and components written in Objective-C except for the few places where C++ interfaces are requried to interface with engine APIs such as Shell and PlatformView (e.g. the PlatformViewIOS subclass). Now that the implementation is Objective-C, the class and file are renamed to match Objective-C naming conventions.
This allows us to take advantage of ARC and weak references, which eliminates the need for std::shared_ptr, fml::WeakPtr etc. Further, this eliminates some particularly unintuitive behaviour wherein this class was owned via a std::shared_ptr held by FlutterEngine, and injected into many other classes (e.g. AccessibilityBridge) via a std::shared_ptr& reference -- such that only one instance of the std::shared_ptr actually ever existed, presumably to avoid std::shared_ptr refcounting overhead. Given that this overhead was only incurred a single time at engine initialisation, this seems like overkill. One might ask why it wasn't therefore held in a `std::unique_ptr` and a `std::unique_ptr&` reference passed around. Likely, this was because we wanted to take a `fml::WeakPtr` reference on it.
Regardless, none of this is necessary any longer now that we can inject `__weak FlutterPlatformViewsController*` instances to classes that use it.
To be clear, this patch makes no attempt whatsoever to simplify or clean up the interface or implementation of this class. This class ties together far too many concepts and is injected into far too many places, and we should break it up and simplify it. However, the goal of this patch was simply to port to an Objective-C interface that plays nicely with the rest of the iOS embedder. This does include a couple minor cleanups in `#include`/`#import` order and usage to match our style guide.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
