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

[Impeller] added a fallback that will make sure the blur fragment shader doesn't overflow #53466

Merged
merged 5 commits into from
Jun 24, 2024
Merged

[Impeller] added a fallback that will make sure the blur fragment shader doesn't overflow #53466

merged 5 commits into from
Jun 24, 2024

Conversation

gaaclarke
Copy link
Member

issue: flutter/flutter#150462

This makes that usage never crash. I would like to reevaluate for a better fix later though. Something like:

  1. Rework the downsample logic to make sure this gap doesn't exist.
  2. Simply increase the kernel size. We already did that before and technically we should be clamped to the 500 sigma so we won't have to do it forever.

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 requested a review from jonahwilliams June 18, 2024 22:31
jonahwilliams
jonahwilliams approved these changes Jun 18, 2024
View reviewed changes
Copy link
Member

@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
Copy link
Member Author

I'm not sure if this is going to pass CI. The blur tests run fine, the added unit test is getting a stack overflow locally for some reason.

@gaaclarke
Copy link
Member Author

Oh! we are doing a stack overflow since we temporarily will have more samples than will be supported, but then we use the lerp hack to make it fit. I'm going to have to rework this, not minutes before my vacation.

@gaaclarke
Copy link
Member Author

I got rid of the stack overflow. I really would like more time to study this when I get back. I'm going to refrain from landing it before I head out. It's here if for some reason it becomes a priority.

@gaaclarke gaaclarke requested a review from jonahwilliams June 24, 2024 17:24
jonahwilliams
jonahwilliams reviewed Jun 24, 2024
View reviewed changes
impeller/entity/contents/filters/gaussian_blur_filter_contents.cc
// are around 53 before the down-sampling and max sigma of 500 kick in.
//
// TODO(https://github.com/flutter/flutter/issues/150462): Come up with a more
// wholistic remedy for this. A proper downsample size should not make this
Copy link
Member

Choose a reason for hiding this comment

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

holistic ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Both spellings are accepted. After a slight rabbit hole I can report that it isn't some sort of british-ism. "Wholistic" came about about 100 years ago for some goofy reason, probably to sell snake oil.

jonahwilliams
jonahwilliams approved these changes Jun 24, 2024
View reviewed changes
Copy link
Member

@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
Copy link
Member Author

@jonahwilliams I tweaked this a bit since your approval if you want to give it another look. Changes:

  1. I created a new kernel samples struct that can hold 2x the samples (since the lerp hack will reduce it by 2).
  2. I fixed an assert off-by-one error.
  3. I added an assert in lerp hack.

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 24, 2024
@auto-submit auto-submit bot merged commit 2badafd into flutter:main Jun 24, 2024
29 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 24, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 24, 2024
@engine-flutter-autoroll
2badafd33 [Impeller] added a fallback that will make sure the blur fr…
cac9123 
…agment shader doesn't overflow (flutter/engine#53466)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 24, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 24, 2024
@engine-flutter-autoroll
2badafd33 [Impeller] added a fallback that will make sure the blur fr…
b3f789c 
…agment shader doesn't overflow (flutter/engine#53466)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 25, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2024
@engine-flutter-autoroll
2badafd33 [Impeller] added a fallback that will make sure the blur fr…
e92dc33 
…agment shader doesn't overflow (flutter/engine#53466)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 25, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2024
@engine-flutter-autoroll
2badafd33 [Impeller] added a fallback that will make sure the blur fr…
abac895 
…agment shader doesn't overflow (flutter/engine#53466)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 25, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2024
@engine-flutter-autoroll
2badafd33 [Impeller] added a fallback that will make sure the blur fr…
3c9f477 
…agment shader doesn't overflow (flutter/engine#53466)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 25, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2024
@engine-flutter-autoroll
2badafd33 [Impeller] added a fallback that will make sure the blur fr…
f9afaba 
…agment shader doesn't overflow (flutter/engine#53466)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 25, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2024
@engine-flutter-autoroll
2badafd33 [Impeller] added a fallback that will make sure the blur fr…
2bb8b46 
…agment shader doesn't overflow (flutter/engine#53466)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 25, 2024
@engine-flutter-autoroll
Roll Flutter Engine from be7db94196fe to afa7ce19bca8 (18 revisions) (#…
c9f72d5 
…150762)

flutter/engine@be7db94...afa7ce1

2024-06-25 [email protected] Roll Dart SDK from bb18127b2a8e to b5fc85cfcf1b (1 revision) (flutter/engine#53552)
2024-06-25 [email protected] Roll Skia from 5feca3095719 to 335200e57c26 (1 revision) (flutter/engine#53549)
2024-06-25 [email protected] Roll Dart SDK from c187d4b3ec88 to bb18127b2a8e (1 revision) (flutter/engine#53547)
2024-06-25 [email protected] Roll Fuchsia Linux SDK from _6HNhJ6G59VMceKoN... to WUN7NQK04NjF9fRmf... (flutter/engine#53545)
2024-06-25 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Re-re-land "Upgrade all[most] androidx dependencies to latest" (#53532)" (flutter/engine#53546)
2024-06-25 [email protected] Bump impeller-cmake-example (flutter/engine#53538)
2024-06-24 [email protected] Roll Skia from e20c8b0bac0c to 5feca3095719 (1 revision) (flutter/engine#53544)
2024-06-24 [email protected] [web] Reland "Fix focus management for text fields (#51009)" (flutter/engine#53537)
2024-06-24 [email protected] Roll Dart SDK from 5df89347bddf to c187d4b3ec88 (1 revision) (flutter/engine#53542)
2024-06-24 [email protected] [macOS] Generate universal gen_snapshots (flutter/engine#53524)
2024-06-24 [email protected] Re-re-land "Upgrade all[most] androidx dependencies to latest" (flutter/engine#53532)
2024-06-24 [email protected] Roll Skia from 1948fd53e280 to e20c8b0bac0c (1 revision) (flutter/engine#53540)
2024-06-24 [email protected] Roll Skia from 0fa58b6ddba0 to 1948fd53e280 (2 revisions) (flutter/engine#53536)
2024-06-24 [email protected] Roll Skia from ea84df425483 to 0fa58b6ddba0 (3 revisions) (flutter/engine#53535)
2024-06-24 [email protected] [Impeller] added a fallback that will make sure the blur fragment shader doesn't overflow (flutter/engine#53466)
2024-06-24 [email protected] [fuchsia] Update Fuchsia API level to 19 (flutter/engine#53494)
2024-06-24 [email protected] Roll Dart SDK from 95470b2cac1f to 5df89347bddf (1 revision) (flutter/engine#53534)
2024-06-24 [email protected] Roll Skia from f6b4344d73cc to ea84df425483 (1 revision) (flutter/engine#53531)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from _6HNhJ6G59VM to WUN7NQK04NjF

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

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gaaclarke @jonahwilliams