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

[Impeller] reduce gaussian sampling by 2x #40871

Merged
merged 4 commits into from Apr 4, 2023

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Apr 2, 2023

Uses technique outlined in https://rastergrid.com/blog/2010/09/efficient-gaussian-blur-with-linear-sampling to reduce number of texture samples by 2x. Rather than sampling each texture element with nearest neighbor, sample a lerp'd weighted value that should be identical to the previous measure.

  • Iteration now works from 0 to radius instead of -radius to radius. This allows reuse of the gaussian integral computation since the function is symmetric. Also solves problem where we'd sometimes chop off one end of the iteration (because we started at some value like -9.33 -> 8.33 (because rounding took us up to 9.4 something)

From local testing on an iPhone 13, this is about ~50% faster on the backdropfilter stress test, though no major difference on the animated backdrop filter benchmark as the blur is already fast enough to not cause frame drops.

Most test apps look identical? Malioc results are much worse, but I suspect that it is not really analyzing the number of loops and instead just looking at the content per loop. Because I'm doing more computations per iteration (But fewer iterations) it looks worse.

@jonahwilliams
Copy link
Member Author

This looks about the same as Impeller on ToT, though both look different from Skia:

Using the macrobenchmark test app

Android

Impeller ToT

impeller

Impeller patch

impeller_patch

Skia

skia

iOS

Impeller ToT

impeller_ios

Impeller patch

impeller_patch_ios

Skia

skia_ios

@jonahwilliams jonahwilliams marked this pull request as ready for review April 3, 2023 19:52
@jonahwilliams jonahwilliams requested a review from bdero April 3, 2023 19:52
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

);
total_color += gaussian * Sample(texture_sampler, // sampler
neg_coord // texture coordinates
);
Copy link
Member

@bdero bdero Apr 3, 2023

Choose a reason for hiding this comment

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

Leveraging linear sampling is a great idea. :)

Instead of fanning out the two sides here, I think just taking the original and doing something like for (float16_t i = round(-blur_info.blur_radius); i <= blur_info.blur_radius; i += 2.hf) would be the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

should we just round in the contents so blur_radius is always integral?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah rounding on the CPU seems good

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member Author

Choose a reason for hiding this comment

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

oops, where did it go...

Copy link
Member Author

Choose a reason for hiding this comment

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

oh nvm, its there

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 4, 2023
@auto-submit auto-submit bot merged commit 4e848dc into flutter:main Apr 4, 2023
37 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller needs tests
Projects
No open projects
Archived in project
3 participants