[CP-beta]Fix sdfs being enabled for MacOS regardless of FLTEnableSDFs value#185680
Conversation
…lutter#185565) This changes the `_project.enableSDFs` check in the MacOS FlutterEngine.mm to be the same as the checks for `_project.enableImpeller` and `_project.enableFlutterGPU` next to it. Instead of setting `--impeller-use-sdfs=true` or `--impeller-use-sdfs=false` depending on `_project.enableSDFs`, only set `--impeller-use-sdfs=true` for true, and leave the flag out when it is false. The flag is checked by https://github.com/flutter/flutter/blob/d3757dafb0f28934fe2312fd3d50a721b03bfd8b/engine/src/flutter/shell/common/switches.cc#L553-L560 which uses `command_line.HasOption()`. This checks only for the existence of the flag. The value of the flag (true or false) is entirely ignored. So this would enable SDFs whether we passed in `--impeller-use-sdfs=true` or `--impeller-use-sdfs=false`. If we actually wanted to check the value of the flag, we should use `CommandLine::GetOptionValue()` or `CommandLine::GetOptionValueWithDefault()` instead. It's misleading to pass the flag with a true/false value that is ignored and make the switch get set to true for either one. For consistency with the other flag's I'm leaving this behavior as-is. Also for consistency with the other flags, I'm leaving the `|| std::find(switches.begin(), switches.end(), "--impeller-use-sdfs=true") != switches.end()` check. But this check seems completely redundant/useless to me. Fixes flutter#185562 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
|
@b-luk please fill out the PR description above, afterwards the release team will review this request. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new devicelab test for Impeller SDFS on macOS and updates the macOS engine to handle the --impeller-use-sdfs flag. The review feedback identifies naming inconsistencies in .ci.yaml and TESTOWNERS where the task name and file path do not match the actual filename of the newly created test script, which would lead to CI execution failures.
camsim99
left a comment
There was a problem hiding this comment.
@b-luk I think the Gemini bot is right. I don't think that the devicelab test is currently running: https://ci.chromium.org/ui/p/flutter/builders/luci.flutter.staging/Mac%20hello_world_impeller_macos_sdfs. You'll need to fix this on master too.
There's a fix for this on master that's currently in the merge queue: #185681 What's the guideline for what to do about this in this release branch?
|
Ok great! Since the fix is straightforward and the cherry-pick seems low risk, I think it's fine to just update this PR and then we can land it. I'll make sure not to publish the release until the fix lands in master and the test successfully runs. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a new macOS device lab test for Impeller Signed Distance Fields (SDFs) and updates the macOS engine to handle the --impeller-use-sdfs flag. A filename mismatch was identified in the TESTOWNERS file, where the entry incorrectly references an iOS test instead of the new macOS test file.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new device lab test to verify Impeller's Signed Distance Field (SDF) support on macOS and updates the engine to handle the --impeller-use-sdfs flag. Feedback includes a recommendation to improve the robustness of the Info.plist XML parsing in the test and a suggestion to prevent duplicate command-line switches in the engine initialization logic.
| // Step 2: Modify Info.plist to enable SDFS | ||
| final String xmlStr = plistFile.readAsStringSync(); | ||
| final xmlDoc = XmlDocument.parse(xmlStr); | ||
| final XmlElement dictNode = xmlDoc.findAllElements('dict').first; |
There was a problem hiding this comment.
Using findAllElements('dict').first to find the root dictionary in Info.plist might be fragile if the file contains nested dictionaries (e.g., within other keys). While it works for the current hello_world example, a more robust approach would be to target the root dictionary specifically.
| if (_project.enableSDFs || | ||
| std::find(switches.begin(), switches.end(), "--impeller-use-sdfs=true") != switches.end()) { | ||
| switches.push_back("--impeller-use-sdfs=true"); | ||
| } |
There was a problem hiding this comment.
The logic for adding the --impeller-use-sdfs=true switch will result in duplicate entries in the switches vector if the flag is already present. While the engine typically handles duplicate flags by taking the last one, it would be cleaner to only append the switch if it's not already present or if the project setting needs to override a previous value.
if (_project.enableSDFs) {
switches.push_back("--impeller-use-sdfs=true");
} else if (std::find(switches.begin(), switches.end(), "--impeller-use-sdfs=true") != switches.end()) {
// The flag is already present in switches, no need to add it again.
}
|
The fix is on master now, and the test is running successfully: https://flutter-dashboard.appspot.com/#/build?repo=flutter&branch=master&showBringup=true&taskFilter=hello_world_impeller_macos_sdfs |
|
Great, thanks for the heads up! Let's land this :) |
6229ad2
into
flutter:flutter-3.44-candidate.0
This pull request is created by automatic cherry pick workflow
Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request.
Issue Link:
#185562
Impact Description:
All MacOS apps using Impeller inadvertently have SDF rendering enabled. SDF rendering is only partially implemented and was intended to be behind a default-false flag. These users will experience rendering artifacts when certain shapes are rendered.
Changelog Description:
Explain this cherry pick:
See best practices for examples.
< Replace with changelog description here >
[flutter/185562] When Impeller is enabled on MacOS, SDF rendering is unintentionally enabled
Workaround:
If the app does not require Impeller, building it for MacOS with Impeller disabled will work around this issue.
If the app requires Impeller, then there is no workaround.
Risk:
What is the risk level of this cherry-pick?
Test Coverage:
Are you confident that your fix is well-tested by automated tests?
Validation Steps:
What are the steps to validate that this fix works?
Building and running an app with "flutter run" for MacOS should show the debug message "Using the Impeller rendering backend (Metal)" rather than "Using the Impeller rendering backend (MetalSDF)".