Since we can't see the screenshot we can't really evaluate if this is failing for good/bad reasons. This also uses
the whoe convertToImageREader / readback debugging API that I want to delete.
This is placeholder stuff that I added before the rest of the API to prove that we can call the engine symbols.
Today this is totally redundant as Flutter GPU has a bunch of automated tests which exercise every FFI call.
Part of https://github.com/flutter/flutter/issues/150953.
Provide a way to get the required minimum uniform byte alignment when referencing uniform blocks in a device buffer. Allow the user to explicitly flush DeviceBuffers (necessary for devices without shared memory).
ui.Canvas and ui.SceneBuilder now use the DlPath object directly from the ui.Path object. This results in increased sharing of the wrapper objects which then increases the sharing of both the converted Impeller paths and Skia's volatile flag.
The VolatilePathTracker mechanism is deleted and rather than count the number of frames that a path is stable for, instead we count the number of times it is used for rendering. If a path is used 100 times in a single frame, it will become non-volatile and start being cached almost immediately. The cached Impeller paths are now also tracked for all instances of the same path, rather than for each call site that originated from a DisplayList dispatch.
Part 1/5 for re adding tests documented in flutter/flutter/issues/154746
## 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.
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [x] All existing and new tests are passing.
... instead of some home-grown framework.
This is the last test in the engine that uses custom command line arguments to start. As of this PR, `dart test` including full debugging in an IDE works across all Dart tooling code.
I tried to make this idiomatic as much as possible without changing the logic.
Switch from using the clumsy manual CacheablePath object to a more automatic DlPath object for holding paths in DisplayLists and dispatching them to either Skia or Impeller with auto-conversion.
For now DlPath is just a wrapper around SkPath with an auto-generating Impeller Path object which is very similar in design from what was done with the CacheablePath object except that it manages the caching of the Impeller path internally without extra burden on Impeller or Skia. There is also no need to communicate with the Dispatch method as to which type of path you prefer, they're all "auto-converting" DlPath objects now.
For now, ui.Path still generates an SkPath and so we wrap it when we record it into a DisplayList, just like the former CacheablePath mechanism. It will be a simple conversion to create the DlPath wrapper in ui.Path, though, so as to maintain the cached Impeller paths across frames even if the DisplayList itself is not preserved.
Eventually DlPath will take on more of a role of hiding the construction and internal representation of the paths so that we could be using SkPath, impeller::Path, or some other internal storage. For now, SkPath will likely remain primary storage for a while so that we can deal with PathOps.
The `FLUTTER_ENGINE` has a `/` in it: `ci/ios_debug_unopt_sim`.
```
path.join(storePath, '$iosEngineVariant.zip');
```
was resolving to `path_to_output/ci/ios_debug_unopt_sim.zip`. `path_to_output` existed, but the `ci` directory didn't:
> zip error: Could not create output file (/Volumes/Work/s/w/ir/x/w/rc/flutter_logs_dir/ci/ios_debug_unopt_sim.zip)
Change the output zip path to `path_to_output/ci_ios_debug_unopt_sim.zip` with an underscore instead.
Fixes https://github.com/flutter/flutter/issues/154956
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
The iOS golden scenario tests are supposed to run on Skia and Impeller, but are actually running on Skia twice.
#### Issue 1: script-side
Regression from https://github.com/flutter/engine/pull/46329.
Currently, the tests log "Running simulator tests with Impeller" but then you see that it's not:
```
Running simulator tests with Impeller
...
2024-09-06 13:38:07.692810-0700 Scenarios[73857:470266] [IMPORTANT:flutter/shell/common/shell.cc(456)] [Action Required] The application opted out of Impeller by either using the --no-enable-impeller flag or FLTEnableImpeller=false plist flag. This option is going to go away in an upcoming Flutter release. Remove the explicit opt-out. If you need to opt-out, report a bug describing the issue.
```
https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8737514337106072353/+/u/test:_Scenario_App_Integration_Tests/stdout
`INFOPLIST_FILE="Scenarios/Info_Impeller.plist"` isn't passed into the xcodebuild any more. I confirmed #46329 caused this by inserting a comment in the middle of the `xcodebuild` command, which caused the last argument after the comment to not be passed in.
#### Issue 2: test-side
Regression from https://github.com/flutter/engine/pull/45093.
[The logic deciding whether to use Skia or Impeller goldens is incorrect](1da5dd68fd/testing/scenario_app/ios/Scenarios/ScenariosUITests/GoldenTestManager.m (L84-L90)) since `FLTEnableImpeller` is set in the app Info.plist, not the UI test, so the bundle should be "dev.flutter.Scenarios". That means even if the script was passing in the right Info.plist, the tests would fail since they would have compared the Skia screenshots.
#### This PR
1. Once https://github.com/flutter/engine/pull/55016 merges (edit: done), these tests will no longer be running with the software renderer. Remove all the test skips so they all run on Impeller.
2. Add the missing Impeller golden screenshots.
3. Update the default Info.plist to use Impeller, so if another script mishap happens, it will default to testing Impeller twice, and not Skia. Add a Skia Info.plist variant instead.
4. Update the test logic to check the right bundle ID Info.plist to decide whether to compare against the Skia or Impeller screenshots. Prefer Impeller so if another test-side mishap happens, it will also default to testing Impeller and not Skia. It will only use the Skia goldens if the bool is set in the Info.plist, and that bool is NO (not just if it's missing).
5. All this made the now-default Impeller tests pass, but when passing in `INFOPLIST_FILE="Scenarios/Info_Skia.plist"` the app wasn't launching, with the error:
> Scenarios encountered an error (Failed to install or launch the test runner. (Underlying Error: Simulator device returned an error for the requested operation. Failed to create promise. (Underlying Error: Failed to create app extension placeholder for /Users/chrome-bot/Library/Developer/Xcode/DerivedData/Scenarios-aypjgouuxctxctfazxalyegcximf/Build/Products/Debug-iphonesimulator/Scenarios.app/PlugIns/ScenariosShare.appex. Failed to create promise. (Underlying Error: Failed to set placeholder attributes dev.flutter.Scenarios.ScenariosShare. Failed to create promise. (Underlying Error: extensionDictionary must be set in placeholder attributes for an app extension placeholder. Invalid placeholder attributes.)))))
This is identical to the error in https://github.com/flutter/engine/pull/53717. I finally realized it was because the ScenariosShare app extension requires specific extension keys in its plist, which weren't present in the app Info.plist at `Scenarios/Info_Skia.plist`. So I changed the `INFOPLIST_FILE` path to `$(TARGET_NAME)/Info_Skia.plist` so it would resolve to target-specific copies like `Scenarios/ScenariosShare/Info_Skia.plist`. That meant I had to add a few more copies where they didn't exist in the target path.
Dependent on https://github.com/flutter/engine/pull/55016 landing.
Fixes https://github.com/flutter/flutter/issues/131888
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Wean the DlOpReceiver interface and implementations off of using the SkScalar, Sk[I]Rect, and SkPoint objects in favor of our own DL/Impeller versions.
The start of an ongoing effort to eventually compartmentalize all use of Skia interfaces into a single backend rendering module that is one of 2 semi-pluggable renderers.
It turns out that when there are multiple paths to clip, they are unioned together, rather than intersected. But clipping paths need to be intersected.
There isn't any good way to intersect arbitrary paths. However, it is easy to intersect rect paths, which is the most common use case. Then we simply fallback to software rendering if we have to intersect non-rect paths. That is:
**Case 1** Only 1 clipping path (either rect path or arbitrary path):
Hardware rendering. This should be the most common use case
**Case 2** Multiple rect clipping path:
Hardware rendering. This is also common, and it's the linked issue that we are fixing.
**Case 3** Other complex case (multiple non-rect clipping path)
Fallback to software rendering. This should be rare.
After https://github.com/flutter/engine/pull/53826, we don't have a working benchmark that measures the main thread anymore. However, this PR shouldn't impact our ad benchmark, since it only has 1 clipping path. I will verify manually by checking Instruments and make sure no software rendering is happening. But we really should make the benchmark working again, not just for performance improvement, but also for monitoring regression.
*List which issues are fixed by this PR. You must list at least one issue.*
Fixes https://github.com/flutter/flutter/issues/153904
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Reverts: flutter/engine#54737
Initiated by: chingjun
Reason for reverting: Breaking internal tests. See b/363125155
Original PR Author: gaaclarke
Reviewed By: {matanlurey, jonahwilliams}
This change reverts the following previous change:
[This PR](https://github.com/flutter/engine/pull/54415) was reverted because it requires a manual roll into the framework.
issue: https://github.com/flutter/flutter/issues/127855
integration test: https://github.com/flutter/engine/pull/54415
This does the preliminary work for implementing wide gamut colors in the Flutter framework. Here are the following changes: 1) colors now specify a colorspace with which they are to be interpreted 1) colors now store their components as floats to accommodate bit depths more than 8
The storage of this Color class is weird with float/int storage but that is a temporary solution to support a smooth transition. Here is the plan for landing this:
1) Land this PR
1) Wait for it to roll into the Framework
1) Land https://github.com/flutter/flutter/pull/153938 which will make CupertinoDynamicColor implement Color
1) Land another engine PR that rips out the int storage: https://github.com/flutter/engine/pull/54714
Here are follow up PRs:
1) https://github.com/flutter/engine/pull/54473 - changes DlColor so the wide gamut colors are rendered
1) https://github.com/flutter/engine/pull/54567 - Hooks up these changes to take advantage of wide DlColor
1) https://github.com/flutter/flutter/pull/153319 - the integration test for the framework repo
There are some things that have been left as follow up PRs since they are technically breaking:
1) The math on `lerp` hasn't been updated to take advantage of the higher bit depth
1) `operator==` hasn't been updated to take advantage of the higher bit depth
1) `hashCode` hasn't been updated to take advantage of the higher bit depth
1) `alphaBlend` hasn't been updated to take advantage of the higher bit depth
1) `toString` hasn't been updated to take advantage of the higher bit depth
[This PR](https://github.com/flutter/engine/pull/54415) was reverted because it requires a manual roll into the framework.
issue: https://github.com/flutter/flutter/issues/127855
integration test: https://github.com/flutter/engine/pull/54415
This does the preliminary work for implementing wide gamut colors in the Flutter framework. Here are the following changes: 1) colors now specify a colorspace with which they are to be interpreted 1) colors now store their components as floats to accommodate bit depths more than 8
The storage of this Color class is weird with float/int storage but that is a temporary solution to support a smooth transition. Here is the plan for landing this:
1) Land this PR
1) Wait for it to roll into the Framework
1) Land https://github.com/flutter/flutter/pull/153938 which will make CupertinoDynamicColor implement Color
1) Land another engine PR that rips out the int storage: https://github.com/flutter/engine/pull/54714
Here are follow up PRs:
1) https://github.com/flutter/engine/pull/54473 - changes DlColor so the wide gamut colors are rendered
1) https://github.com/flutter/engine/pull/54567 - Hooks up these changes to take advantage of wide DlColor
1) https://github.com/flutter/flutter/pull/153319 - the integration test for the framework repo
There are some things that have been left as follow up PRs since they are technically breaking:
1) The math on `lerp` hasn't been updated to take advantage of the higher bit depth
1) `operator==` hasn't been updated to take advantage of the higher bit depth
1) `hashCode` hasn't been updated to take advantage of the higher bit depth
1) `alphaBlend` hasn't been updated to take advantage of the higher bit depth
1) `toString` hasn't been updated to take advantage of the higher bit depth
There are almost no behavioral changes.
Because `dart test` runs with assertions, and the former command does not, I had to tweak some of the engine tool tests because they would assert that there were duplicate names - but I didn't change any actual code besides the test expectations themselves/the fixtures.
This is not all of the engine, but is approximately 1/3 of all imports of `package:litetest` migrated.
Fixes https://github.com/flutter/flutter/issues/153964
Changing the origin of the rect used to render a shader could break shaders that expect to render at particular coordinates based on the input vertices. The snapshot functionality correctly handles translating a texture, so the translation was never necessary to begin with.
For each clip scope, draw opaque items in reverse order and
translucent/backdrop-independent items in their original order
afterwards. Clips are treated as translucent by the parent scope.
Respects clips, subpass collapse, and the clear color optimization.
Now also fixes: https://github.com/flutter/flutter/issues/153737
Being able to reorder rendering commands leads to optimization opportunities in the graphics package. A graphics package being fed from a DisplayList either has to take the commands in the order given or implement their own storage format for the rendering data.
With this new dispatching mechanism, the graphics package can both query basic information about the recorded ops and even dispatch them by the index into the list. Query information includes either the "category" of the op (clip/transform/render, etc.) or a specific op type enum. The package can dispatch some categories (or ops) immediately and remember other categories (or ops) along with their state for dispatching later.
