Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Jul 26, 2024

This removes jitter in the gaussian blur.

Previous version of the shimmer test show this drops the average RMSE 30%.

testing:

  • positive change shown in average RMSE in ShimmerTest
  • existing golden tests

Part of a series of gaussian blur changes:

  1. Revert "[Impeller] Use downsample shader for blur instead of mip levels. (#53760)" #54148
  2. [impeller] adds test for catching shimmer in gaussian blur #54116
  3. [Impeller] makes the gaussian down sample scalar fixed by adjusting the downsample padding #54150
  4. Reland: [Impeller] Use downsample shader for blur instead of mip levels. #54149

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

@gaaclarke gaaclarke changed the title [Impeller] made the gaussian downsample scalar fixed by adjusting the padding [Impeller] makes the gaussian down sample scalar fixed by adjusting the downsample padding Jul 26, 2024
@gaaclarke gaaclarke requested review from bdero and jonahwilliams July 26, 2024 20:42
@gaaclarke gaaclarke marked this pull request as ready for review July 26, 2024 20:42
@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #54150 at sha 8c95230

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

gaaclarke added a commit that referenced this pull request Jul 26, 2024
…ls. (#53760)" (#54148)

This is reverted since it introduces shimmering when the downsample
scalar goes to 1/16th. It was reverted instead of fixed to allow the
test that should have blocked this to land in the meantime.

reverts #53760
fixed and relanded in #54149

Part of a series of gaussian blur changes:
1) #54148
1) #54116
1) #54150
1) #54149

## 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.
- [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
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[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
gaaclarke added a commit that referenced this pull request Jul 26, 2024
fixes flutter/flutter#152195

Part of a series of gaussian blur changes:
1) #54148
1) #54116
1) #54150
1) #54149

Note: this test won't pass until
#54148 lands.

## 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.
- [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
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[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
@gaaclarke gaaclarke force-pushed the divisible-gaussian branch from 8c95230 to 0c13fab Compare July 26, 2024 22:02
@gaaclarke gaaclarke force-pushed the divisible-gaussian branch from 0c13fab to a31c346 Compare July 26, 2024 22:14
@gaaclarke
Copy link
Member Author

This creates artifacts around the edges of blurs in some cases. It looks like we get sampling from outside of the blur region a bit when in the past they were clipped and things were stretched a bit to account for the integer boundaries of the subpass's output.

before

trimbefore.mov

after

trimafter.mov

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54150 at sha 9655541

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 30, 2024
@auto-submit auto-submit bot merged commit 4713bf8 into flutter:main Jul 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 30, 2024
auto-submit bot pushed a commit that referenced this pull request Jul 30, 2024
…ls. (#54149)

relands #53760
reverted in #54148

The fix for this was found with the help of Jonah.

Part of a series of gaussian blur changes:
1) #54148
1) #54116
1) #54150
1) #54149

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 30, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 30, 2024
…152573)

flutter/engine@31bb9f9...a6c5ff2

2024-07-30 [email protected] Roll Skia from a4d59988bde0 to c1fcf8e5a55c (1 revision) (flutter/engine#54220)
2024-07-30 [email protected] Reland: [Impeller] Use downsample shader for blur instead of mip levels. (flutter/engine#54149)
2024-07-30 [email protected] Roll Skia from 2cb7f5443a48 to a4d59988bde0 (1 revision) (flutter/engine#54218)
2024-07-30 [email protected] [Impeller] makes the gaussian down sample scalar fixed by adjusting the downsample padding (flutter/engine#54150)
2024-07-30 [email protected] Roll Dart SDK from ad4d2e8b2c65 to 14b51d32e3a6 (1 revision) (flutter/engine#54216)

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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
TytaniumDev pushed a commit to TytaniumDev/flutter that referenced this pull request Aug 7, 2024
…lutter#152573)

flutter/engine@31bb9f9...a6c5ff2

2024-07-30 [email protected] Roll Skia from a4d59988bde0 to c1fcf8e5a55c (1 revision) (flutter/engine#54220)
2024-07-30 [email protected] Reland: [Impeller] Use downsample shader for blur instead of mip levels. (flutter/engine#54149)
2024-07-30 [email protected] Roll Skia from 2cb7f5443a48 to a4d59988bde0 (1 revision) (flutter/engine#54218)
2024-07-30 [email protected] [Impeller] makes the gaussian down sample scalar fixed by adjusting the downsample padding (flutter/engine#54150)
2024-07-30 [email protected] Roll Dart SDK from ad4d2e8b2c65 to 14b51d32e3a6 (1 revision) (flutter/engine#54216)

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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
…lutter#152573)

flutter/engine@31bb9f9...a6c5ff2

2024-07-30 [email protected] Roll Skia from a4d59988bde0 to c1fcf8e5a55c (1 revision) (flutter/engine#54220)
2024-07-30 [email protected] Reland: [Impeller] Use downsample shader for blur instead of mip levels. (flutter/engine#54149)
2024-07-30 [email protected] Roll Skia from 2cb7f5443a48 to a4d59988bde0 (1 revision) (flutter/engine#54218)
2024-07-30 [email protected] [Impeller] makes the gaussian down sample scalar fixed by adjusting the downsample padding (flutter/engine#54150)
2024-07-30 [email protected] Roll Dart SDK from ad4d2e8b2c65 to 14b51d32e3a6 (1 revision) (flutter/engine#54216)

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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller will affect goldens

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants