Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use decal TileMode in blur image_filter_test.dart #45004

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

lhkbob
Copy link
Contributor

@lhkbob lhkbob commented Aug 23, 2023

The expected color values appear to match the output of Skia's raster backend's blur. Historically, that doesn't support any tile mode other than decal, so while the makeBlur() function defaulted to clamp tiling, the output was decal and thus showed the blur fading to transparent.

The test draws a 1x1 green rectangle in the center of a 3x3 image. Clamp tiling would actually cause the output of the blur to just copy the central green color to the remaining 8 pixels. This is the output of Skia's GPU blur. I am working to land changes in Skia that make the raster backend handle all tile modes, which then has it match the existing GPU blur's behavior of a constant output for clamp tiling in this test (so it then fails).

Decal tiling appears to be more useful for this test case anyways because it creates per-pixel variations that can be validated against.

This is needed to land Skia-side fixes for skbug.com/40039877 and skbug.com/40039025

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.

The expected color values appear to match the output of Skia's raster backend's blur. Historically, that doesn't support any tile mode other than decal, so while the `makeBlur()` function defaulted to clamp tiling, the output was decal and thus showed the blur fading to transparent.

The test draws a 1x1 green rectangle in the center of a 3x3 image. Clamp tiling would actually cause the output of the blur to just copy the central green color to the remaining 8 pixels. This is the output of Skia's GPU blur. I am working to land changes in Skia that make the raster backend handle all tile modes, which then has it match the existing GPU blur's behavior of a constant output for clamp tiling in this test (so it then fails).

Decal tiling appears to be more useful for this test case anyways because it creates per-pixel variations that can be validated against.
@zanderso zanderso requested a review from flar August 23, 2023 16:31
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brianosman brianosman merged commit 25fb06e into flutter:main Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 23, 2023
…133211)

flutter/engine@58dc868...27d75f6

2023-08-23 [email protected] Roll Skia from 76898dad9fda to a631fefdba37 (2 revisions) (flutter/engine#45027)
2023-08-23 [email protected] Revert "FontVariation.lerp, custom FontVariation constructors, and more documentation" (flutter/engine#45023)
2023-08-23 [email protected] [Impeller] Dat rvalue reference (fix engine head) (flutter/engine#45024)
2023-08-23 [email protected] Revert "Enable clang-tidy for pre-push (opt-out), exclude `performance-unnecessary-value-param`" (flutter/engine#45020)
2023-08-23 [email protected] Roll Skia from 4e42b51cfe27 to 76898dad9fda (1 revision) (flutter/engine#45019)
2023-08-23 [email protected] [Impeller] Add STB text backend. (flutter/engine#44887)
2023-08-23 [email protected] Followup to flutter/engine#44982 (flutter/engine#45018)
2023-08-23 [email protected] Roll Skia from 5428f147e632 to 4e42b51cfe27 (4 revisions) (flutter/engine#45016)
2023-08-23 [email protected] Eliminate android test log spam (flutter/engine#44982)
2023-08-23 [email protected] [web] Remove some unused functions (flutter/engine#44505)
2023-08-23 [email protected] Use decal TileMode in blur image_filter_test.dart (flutter/engine#45004)
2023-08-23 [email protected] FontVariation.lerp, custom FontVariation constructors, and more documentation (flutter/engine#44996)
2023-08-23 [email protected] [impeller] combine sampler and texture maps. (flutter/engine#44990)
2023-08-23 [email protected] [Impeller] Flutter GPU: Add HostBuffer. (flutter/engine#44696)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023
The expected color values appear to match the output of Skia's raster
backend's blur. Historically, that doesn't support any tile mode other
than decal, so while the `makeBlur()` function defaulted to clamp
tiling, the output was decal and thus showed the blur fading to
transparent.

The test draws a 1x1 green rectangle in the center of a 3x3 image. Clamp
tiling would actually cause the output of the blur to just copy the
central green color to the remaining 8 pixels. This is the output of
Skia's GPU blur. I am working to land changes in Skia that make the
raster backend handle all tile modes, which then has it match the
existing GPU blur's behavior of a constant output for clamp tiling in
this test (so it then fails).

Decal tiling appears to be more useful for this test case anyways
because it creates per-pixel variations that can be validated against.

This is needed to land Skia-side fixes for skbug.com/40039877 and
skbug.com/40039025

## 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.

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
@lhkbob lhkbob deleted the blur-decal-test-fix branch January 5, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants