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 Feb 1, 2024

This makes the downsampling step of the new blur round to the nearest power of 2. This makes the changes in downsampling less frequent and the output of downsampling hypothetically higher quality since downsampling by a power of 2 is easier.

issue: flutter/flutter#141510

before

pow2blur-before.mp4

after

pow2blur-after.mp4

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 marked this pull request as ready for review February 1, 2024 18:12
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

Does this improve the animated blur case? Follow up question, if we're scaling down by a power of two, doesn't this imply that we're sampling at an exact mip level? Could we bypass the downsample step entirely and configure the first pass of the blur to sample from a specific miplevel?

@gaaclarke
Copy link
Member Author

Does this improve the animated blur case?

Do you mean the animated sigma? That's whats in the video above. Or are you referring to the animated content under the blur? I didn't look into that one but thinking about it now it may be worse since we are potentially downscaling earlier (I used round instead of ceil) but better because of the power of 2.

Follow up question, if we're scaling down by a power of two, doesn't this imply that we're sampling at an exact mip level? Could we bypass the downsample step entirely and configure the first pass of the blur to sample from a specific miplevel?

Yep, brandon has brought up that optimization in the past.

I'm trying to avoid creating a test harness for this case. But if we wanted to we could generate multiple images and use root mean square error to quantify jitters in the animating sigma case. It gets a bit harder in the "animate content under blur" case.

@gaaclarke
Copy link
Member Author

I'll try to modify the EntityTest.GaussianBlurFilter interactive test to have a button that will animate the image under the blur so that we have a more solid reproduction of that case. That's something that should be easy to do.

@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 #50245 at sha eb293e7

@gaaclarke
Copy link
Member Author

@jonahwilliams at first blush, it's looking good

bluranimated.mp4

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 1, 2024
@auto-submit auto-submit bot merged commit 9beb7e8 into flutter:main Feb 1, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 1, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 1, 2024
…142745)

flutter/engine@39415c3...9beb7e8

2024-02-01 [email protected] [Impeller] new blur: round downsample to power of two (flutter/engine#50245)
2024-02-01 [email protected] Provide a more helpful error message in the case of UnsatisfiedLinkError (flutter/engine#50247)
2024-02-01 [email protected] Update expected golden number. (flutter/engine#50249)
2024-02-01 [email protected] [Impeller] remove drawPicture from Aiks Canvas. (flutter/engine#50242)
2024-02-01 [email protected] Reland "[Windows] Introduce egl::Surface and egl::WindowSurface" (flutter/engine#50148)

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

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

2 participants