[beta] Fix EmbedderTest.CanRenderTextWithImpellerMetal test breakage#186828
Merged
auto-submit[bot] merged 1 commit intoMay 20, 2026
Merged
Conversation
…ter#186262) Fix `EmbedderTest.CanRenderTextWithImpellerMetal` test breakage. This broke due to flutter#186074. That PR modified how light text is rendered on macOS, but did not update this test's golden image. This updates the image. It also changes the test to explicitly render dark text on a light background and light text on a dark background. Previously it rendered white text on a transparent background. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] 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 --------- Co-authored-by: Jason Simmons <jsimmons@google.com>
|
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. |
30 tasks
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the render_impeller_text_test fixture to render multiple text styles and modifies the image comparison utility to support a configurable threshold for differing pixels. Feedback highlights a potential null pointer dereference when accessing normalized image data and recommends using uint32_t instead of uint64_t for pixel comparisons to ensure correct alignment and handling of images with odd pixel counts.
Comment on lines
+79
to
+81
| if (normalized_a->size() != normalized_b->size()) { | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
The NormalizeImage function can return nullptr if readPixels fails (e.g., if the image is on the GPU and not readable). Accessing size() on a null sk_sp<SkData> will cause a crash. A null check should be added before accessing the size.
Suggested change
| if (normalized_a->size() != normalized_b->size()) { | |
| return false; | |
| } | |
| if (!normalized_a || !normalized_b || normalized_a->size() != normalized_b->size()) { | |
| return false; | |
| } |
| if (normalized_a->size() != normalized_b->size()) { | ||
| return false; | ||
| } | ||
| using NormalizedPixel = uint64_t; |
Contributor
There was a problem hiding this comment.
Using uint64_t for NormalizedPixel is problematic for several reasons:
- Correctness:
NormalizeImageproduces 4-byte-per-pixel data (kN32_SkColorType). Using an 8-byte type meansallowable_different_pixelsactually counts 8-byte chunks (pairs of pixels), which makes the threshold inconsistent with its name. - Robustness: The
FML_CHECKon line 83 will fail for any image with an odd number of total pixels (e.g., a 1x1 image), as the total size in bytes will not be a multiple of 8. - Alignment: Dereferencing a
uint64_t*cast from a byte buffer requires 8-byte alignment, which is not strictly guaranteed bySkData(though common).
Switching to uint32_t matches the actual pixel size and resolves these issues.
Suggested change
| using NormalizedPixel = uint64_t; | |
| using NormalizedPixel = uint32_t; |
13 tasks
ac63f7b
into
flutter:flutter-3.45-candidate.0
8 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This cherry-picks #186262 which will unblock CI for the 3.45 beta candidate branch. At the time of the branch cut, this failure was in the master branch, but
mac_unoptwas suppressed. Including this fix should unblock CI on the beta candidate branch.