After having run a memory audit on Flutter Gallery I believe we can turn
on wide gamut support by default for impeller. Here are the
ramifications of this change:
1) If an opaque surface is used and no transparent wide gamut images are
used, there should be no change in performance or memory usage.
1) If a transparent surface is used; the surface will take up 2x memory
(ex 4MB on iPhone 7) and will have a runtime performance cost.
1) If wide gamut **transparent** images are used, they will take up 2x
memory. Opaque wide-gamut images will take up the same amount of memory.
## Flutter gallery test results
### Test 1
Steps:
* Tap "Material"
* Scroll down and back to "Cards"
* Tap "Cards"
* Scroll to bottom of "Cards" example
Notice that the card images are wide gamut, AdobeRGB.
**Memory increase:** 10.401 MB (+8.7%)
### Test 2
Steps:
* Just launch the gallery
Notice that there shouldn't be any wide gamut images.
**Memory Increase:** 1.22 MB (+1%, this is probably a fluke of
measurement)
## 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].
- [ ] 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.
- [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
This change migrates `touch-input` integration tests from the
`gfx-root-presenter-test-ui-stack` UI test realm variant to run
parameterized tests of two types: `gfx-scene-manager-test-ui-stack` and
`flatland-scene-manager-test-ui-stack`. Both are exercised for the
generic tap test, and only GFX is exercised for embedded view cases.
This will enable fuchsia.git to remove the
`gfx-root-presenter-test-ui-stack` variant, which will no longer be
supported.
This change also:
- removes or updates all remaining references of root presenter, which
manifested in the form of inline code comments
- adds a TODO to update the child view app for the embedded touch input
test cases
Note that this change does _not_ modify the `embedder` integration
tests, which use a generic `test-ui-stack` variant as defined by the
fuchsia pkg url:
`fuchsia-pkg://fuchsia.com/test-ui-stack#meta/test-ui-stack.cm`. Since
the contents of this package is defined in fuchsia.git, the realm under
test can adapt to changes in fuchsia.git so long as they update the
test-ui-stack build target (which they do in https://fxrev.dev/831359,
the change that relies on the touch changes in this repo described
above.)
Fixes https://fxbug.dev/125304
## 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.
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.)
The "size interpolation" solution didn't go well (more context [here](https://github.com/flutter/engine/pull/40412#issuecomment-1485938933)). Then a new solution came to my mind, and I call it **"delayed swap"**:
In the originally behavior, we swap the width/height immediately before the rotation, resulting in roughly ~4x distortion in the beginning. With "delayed swap" solution, we **swap the width/height right in the middle of the rotation** (i.e. delay the swap for half of the transition duration).
This new "delayed swap" solution gives us the same benefit as the "snapshot" solution:
- reducing ~4x distortion to ~2x
- most distorted frames occur in the middle of rotation when it's moving the fastest, making it hard to notice
And it fixes the drawback of "snapshot" solution:
- it works well with dynamic content like animation or video
- it doesn't have a ~0.5 second penalty when taking the snapshot
Looks pretty good on flutter gallery:
https://user-images.githubusercontent.com/41930132/228383137-7cd09982-89a9-4c83-bf55-9431de708278.mp4
*List which issues are fixed by this PR. You must list at least one issue.*
Fixes https://github.com/flutter/flutter/issues/16322
*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 fixed by this PR: https://github.com/flutter/flutter/issues/110138
This PR fixes an issue which is causing app crash when User tries to navigate to a new instance **FlutterFragment** (whose old instance is already present in the fragment backstack) in an Activity (which was restored to saved state after being killed in background due to memory pressure).
Detailed case to reproduce the crash and identify its' root cause:
Setup: Let's say we've an Activity1 which has a bottom nav bar with 3 tabs. Each of this 3 tabs are **FlutterFragment** i.e. Fragment1, Fragment2 & Fragment3 and all of them are using separate **FlutterEngine** but all of them will be cached. e.g. Multiple instances of **Fragment1** will be going to use same cached **FlutterEngine1**.
1. When User opens the app, Fragment1 gets added to fragment backstack
2. Then User navigates to Fragment2 (gets added to backstack as well)
3. Then User navigates to Fragment3 (gets added to backstack as well)
4. Then User puts the app in background. Due to memory pressure OS/platform kills the Activity1 and all 3 FlutterFragments while the app is in background.
5. Then after sometime User tries to bring the app to foreground from the app stack. Since Activity1 was killed by the OS/platform the app process will try to restore the Activity1 in the same state it was before it got killed. This leads to all 3 fragments present in backstack to get instantiated and then the **onAttach()** gets called for all 3, but only Fragment3 gets **onCreateView()** lifecycle event as it was the top most visible Fragment before the FragmentManager saved the state and app went into background. All 3 FlutterFragment goes through following function calls.
FlutterFragment.onAttach() -> FlutterActivityAndFragmentDelegate.onAttach(). There is a one-to-one mapping between **FlutterFragment <-> FlutterActivityAndFragmentDelegate**
<img width="977" alt="1" src="https://user-images.githubusercontent.com/8373036/231222642-1596c77c-d127-476b-9bce-8ad2e9cd3639.png">
FlutterActivityAndFragmentDelegate.onAttach() -> FlutterEngineConnectionRegistry.attachToActivity(). There is a one-to-one mapping between **FlutterEngine <-> FlutterEngineConnectionRegistry**. _**NOTE**: THIS IS VERY IMPORTANT POINT TO KEEP IN MIND TO UNDERSTAND THE ROOT CAAUSE OF THIS CRASH._
<img width="962" alt="2" src="https://user-images.githubusercontent.com/8373036/231222672-016ee708-c310-49c8-8016-070b6057af7b.png">
Since all the 3 **FlutterFragment** were just instantiated on activity restore **exclusiveActivity** will be null and exclusiveActivity will be assigned the host **FlutterActivityAndFragmentDelegate**.
<img width="880" alt="3" src="https://user-images.githubusercontent.com/8373036/231241491-c47f5aa6-96e9-4c1f-b92e-7cfed67381e2.png">
6. Then FlutterFragment.onCreateView() will be called only for Fragment3 and it will be visible without an issue as its' state gets restored properly.
7. Then if User tries to navigate to Fragment2 via instantiating new instance of it (this means that now there will be two instances of Fragment2 in the backstack), then there will be crash as it'll go through following function calls.
FlutterFragment.onAttach() -> FlutterActivityAndFragmentDelegate.onAttach().
<img width="977" alt="1" src="https://user-images.githubusercontent.com/8373036/231222642-1596c77c-d127-476b-9bce-8ad2e9cd3639.png">
FlutterActivityAndFragmentDelegate.onAttach() -> FlutterEngineConnectionRegistry.attachToActivity().
<img width="962" alt="2" src="https://user-images.githubusercontent.com/8373036/231222672-016ee708-c310-49c8-8016-070b6057af7b.png">
THIS IS WHERE THE CRASH STARTS. Since this is the second instance of Fragment2 and both instances are going to use the same cached **FlutterEngine** and hence same **FlutterEngineConnectionRegistry**, this time around **exclusiveActivity** will be non-null as it was assigned during step 5. And since exclusiveActivity will be be non null it'll try to detach from the old **ExclusiveAppComponent** via calling **exclusiveActivity.detachFromFlutterEngine()**.
<img width="878" alt="7" src="https://user-images.githubusercontent.com/8373036/231241550-09908108-1747-4ec3-bc70-145446b86eae.png">
FlutterActivityAndFragmentDelegate.detachFromFlutterEngine() -> FlutterFragment.detachFromFlutterEngine()
<img width="905" alt="4" src="https://user-images.githubusercontent.com/8373036/231222702-5ee739f2-fd71-4e20-807d-e3786e6f27fc.png">
FlutterFragment.detachFromFlutterEngine() -> FlutterActivityAndFragmentDelegate.onDestroyView(). This ideally should not be called if the hosts' **FlutterFragments.onCreateView()** was not called in the first place. Also since the previous author has added **// Redundant calls are ok.** comment, I'm guessing that this is just a fallback cleanup.
<img width="902" alt="5" src="https://user-images.githubusercontent.com/8373036/231222719-2ea28157-bd62-45fe-89ef-dd41ee7b3cca.png">
THIS IS WHERE THE CRASH HAPPENS. FlutterActivityAndFragmentDelegate.onDestroyView() -> FlutterView.detachFromFlutterEngine(). Since the lifecycle of older instance of this FlutterFragment2 was capped at onAttach(), it's **FlutterView** property will be null and calling FlutterView.detachFromFlutterEngine() will throw NPE.
<img width="902" alt="6" src="https://user-images.githubusercontent.com/8373036/231222735-55b63911-6a2d-4967-8043-298f4d413f2a.png">
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
In Skia, there is such a timeline event in `SnapshotControllerSkia::DoMakeRasterSnapshot`. Therefore, since Impeller wants to mimic Skia and this event does take a long time sometimes, it seems reasonable to add this.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
FlutterMutatorViewTest.TransformedFrameIsCorrect and
FlutterMutatorViewTest.RoundRectClipsToPath both apply transforms then
do exact comparisons on the resulting 4x4 matrices of CGFloat values.
There are very small differences between the elements in the resulting
matrices, on the order of 1e-15, depending on the CPU architecture on
which the floating point operations were run.
Similar to elsewhere in the framework and engine, we now perform
floating point equality tests within some small error bound -- I've
arbitrarily selected 1e-10.
Issue: https://github.com/flutter/flutter/issues/124840
Reverts flutter/engine#40746
Googler bug: b/278174021
Failing on
```
shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java:239: Error: This method should only be accessed from tests or within private scope [VisibleForTests]
imeSyncCallback.remove();
```
Prepare Flutter's Fuchsia port for removal of the
fuchsia.ui.app.ViewProvider.CreateView() API.
The Flutter engine's implementation of the API, which has long been
deprecated, is removed.
Calls to the deprecated CreateView() in tests are replaced with
CreateViewWithViewRef().
Bug fxbug.dev/81285
This CL changes ERROR to WARNING for the logs when a child disconnects
and ChildViewWatcher is closed. This may happen due to other apps and
doesn't necessarily indicate a flutter error.
Bug:fxb/125256
Resolves https://github.com/flutter/flutter/issues/124612.
Doing this would also allow us to remove the final blit on iOS when
advanced blends/backdrop filters are present (all of the facilities
would remain in place for GLES + Vulkan though).
Fixes https://github.com/flutter/flutter/issues/124269
Manually verified this is safe on an iPhone 11.
We're not creating/encoding command buffers in this path so it's ok.
AFAICT the test I added would fail if we started doing that because it
doesn't provide any real command buffer interfaces. Most of the code
here is related to tests.
Moves application termination handling to FlutterAppDelegate.
Previously, we required macOS applications using Flutter to ensure their
main application class was FlutterApplication. Instead, we now do all
handling in FlutterAppDelegate and FlutterEngine.
There are two termination workflows to consider:
* Termination requested from framework side: In this case, then engine
receives a `System.exitApplication` method call, and starts the app
termination workflow via `[FlutterEngine
requestApplicationTermination:exitType:result]`.
* Termination requested from macOS (e.g. Cmd-Q): In this case,
`FlutterAppDelegate`'s `applicationShouldTerminate:` handler is invoked
by AppKit, and the delegate starts the app termination workflow via
`[FlutterEngine requestApplicationTermination:exitType:result]`.
In either case, at this point, if the request is not cancellable, the
app immediately exits. If it is cancellable, the embedder sends a
`System.requestAppExit` method channel invocation to the framework,
which responds with either `exit` or `cancel`. In the case of `exit` we
immediately exit, otherwise we do nothing and the app continues running.
This is a minor refactoring of the original approach we took in:
https://github.com/flutter/engine/pull/39836
This does not remove the FlutterApplication class, since the framework
migration from NSApplication to FlutterApplication still depends on it.
A followup patch with replace the migration with a reverse migration
will land, then FlutterApplication will be removed.
Issue: https://github.com/flutter/flutter/issues/30735
No new tests since this refactors existing behaviour while retaining the
same app semantics as before.
With this approach, when the last window of an app is closed, the engine
sends a message to the framework to check if it is allowed to exit. If
it may, the windows lifecycle manager synthesizes a second `WM_CLOSE`
message that it will ignore, and so the next registered top level window
proc delegate, if any, will be able to process the message. If none do
so, the message will be handled by the default window proc, so the app
will be able to close.
I was not able to get a full system tray example application running to
test this, but I could get an application that stays open when its
window is closed and can be seen as a system tray icon as long as it is
running, albeit the icon was non-functional. As this repro app still
exhibited this behavior when using this engine build, I am reasonably
confident in concluding that applications that want to be able to run
headless when their windows close will function properly.
Addresses https://github.com/flutter/flutter/issues/123654.
## 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 `///`).
- [ ] 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
---------
Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>
This PR makes view ID signed from unsigned int64.
Initially, I made view IDs unsigned because they were opaque anyway. As
I'm working deeper into multiview, I found some issues that made me
think signed is better:
* Unsigned integers are worse
* Sometimes you want negative values to represent special values.
* Unsigned integers are dangerous (if compared with signed ones by
mistake.)
* Unsigned integers are not needed
* We're very unlikely to reach that big anyway.
* Almost all other languages support only signed integers.
* Also JavaScript only supports up to 51 bits of integer.
Therefore I think it's better to change them to signed int64, especially
before these APIs are widely used by developers.
## 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
