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] new blur: refactored math and fixed expanded padding size #49206

Merged
merged 15 commits into from
Dec 20, 2023

Conversation

gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Dec 19, 2023

This refactors the math so that it makes it easier to conditionally add padding. That optimization was removed for now since it wasn't quite working satisfactorily yet.

This also fixes the math used to expand the coverage hint. There is no visual test for it since it would only ever result in wasteful expansion that would show up in the benchmarks.

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

Here is the stats as this pr stands now:

    "average_vsync_transitions_missed": 5.6770428015564205,
    "90th_percentile_vsync_transitions_missed": 6.0,
    "99th_percentile_vsync_transitions_missed": 6.0,
    "average_vsync_frame_lag": 0.0,
    "90th_percentile_vsync_frame_lag": 0.0,
    "99th_percentile_vsync_frame_lag": 0.0,
    "average_layer_cache_count": 0.0,
    "90th_percentile_layer_cache_count": 0.0,
    "99th_percentile_layer_cache_count": 0.0,
    "average_frame_request_pending_latency": 16637.95840266223,
    "90th_percentile_frame_request_pending_latency": 16726.0,
    "99th_percentile_frame_request_pending_latency": 16788.0,
    "worst_layer_cache_count": 0.0,
    "average_layer_cache_memory": 0.0,
    "90th_percentile_layer_cache_memory": 0.0,
    "99th_percentile_layer_cache_memory": 0.0,
    "worst_layer_cache_memory": 0.0,
    "average_picture_cache_count": 0.0,
    "90th_percentile_picture_cache_count": 0.0,
    "99th_percentile_picture_cache_count": 0.0,
    "worst_picture_cache_count": 0.0,
    "average_picture_cache_memory": 0.0,
    "90th_percentile_picture_cache_memory": 0.0,
    "99th_percentile_picture_cache_memory": 0.0,
    "worst_picture_cache_memory": 0.0,
    "total_ui_gc_time": 2.8120000000000003,
    "30hz_frame_percentage": 0.0,
    "60hz_frame_percentage": 100.0,
    "80hz_frame_percentage": 0.0,
    "90hz_frame_percentage": 0.0,
    "120hz_frame_percentage": 0.0,
    "illegal_refresh_rate_frame_count": 0,
    "average_gpu_frame_time": 83.81782945736434,
    "90th_percentile_gpu_frame_time": 125.0,
    "99th_percentile_gpu_frame_time": 125.0,
    "worst_gpu_frame_time": 125.0,
    "average_cpu_usage": 114.68125052083332,
    "90th_percentile_cpu_usage": 122.700001,
    "99th_percentile_cpu_usage": 124.599999,
    "average_gpu_usage": 100.0,
    "90th_percentile_gpu_usage": 100.0,
    "99th_percentile_gpu_usage": 100.0,
    "average_memory_usage": 76.1376953125,
    "90th_percentile_memory_usage": 81.25,
    "99th_percentile_memory_usage": 83.546875
  },

This may be a slight improvement over previous readings ("average_gpu_frame_time": 87.70)

@gaaclarke gaaclarke marked this pull request as ready for review December 19, 2023 22:32
@gaaclarke gaaclarke requested a review from bdero December 19, 2023 22:32
Matrix padding_snapshot_adjustment;
if (!coverage_hint.has_value() ||
(input_snapshot_coverage.has_value() &&
!input_snapshot_coverage->Contains(coverage_hint.value()))) {
Copy link
Member

Choose a reason for hiding this comment

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

A couple of things come to mind while thinking about this:

  1. Did you mean to compare against the expanded coverage hint instead? At the end of the downsample pass, we need to have enough content in the downsample pass to cover sampling from the entire expanded coverage area area for the full area of the expanded coverage hint.
  2. Should this check be flipped? Like if the expanded_coverage_hint is equal to or a subset of the input_snapshot_coverage, then I suppose that would indicate that the snapshot rendered content into a big enough square that we don't need to worry about creating more with the proper tiling mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, the input_snapshot_coverage is incorporating the expanded coverage hint.

This is basically saying: I requested an expanded_coverage_hint and the snapshot you gave me for that doesn't contain all of the area of the coverage_hint. Therefore, some part of the halo will be visible and we'll add padding.

Copy link
Member Author

Choose a reason for hiding this comment

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

Check out the test, that should make this clear. The coverage_hint is saying we are going to render a small portion of the image, so there is no need to add the halo gutter.

Copy link
Member

@bdero bdero Dec 20, 2023

Choose a reason for hiding this comment

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

Ah, I actually missed the not operator in front of the Contains check here.

I still think this isn't quite working for all cases. I made an interactive playground (will push a PR for it) to demonstrate an issue with Decal tiling:

Screen.Recording.2023-12-20.at.5.08.59.AM.mov

The color jumping is explained by flutter/flutter#140193 (comment) and isn't the focus of the video. The main problem is that the edge of the blurred image turns opaque when the clip becomes contained by the input snapshot coverage.

Copy link
Member

Choose a reason for hiding this comment

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

(Made a PR for the interactive toy here: #49283)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh right, the coverage_hint has to be inside of the snapshot image coverage while accounting for the blur radius. I'll fix that although I wonder if that will make this optimization hardly applicable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed the problem, but there is a jump in the rendering because of the downsampling issue. It also catches way less cases now because it isn't cutting off the padding when hitting the edge of the input. I'm going to keep all the refactors and land it as a refactor since I have PRs that depend on this. I can circle back.

I also ran into a fun Rect floating point bug =( flutter/flutter#140464

auto-submit bot pushed a commit that referenced this pull request Dec 20, 2023
I used this interactive toy to inspect the change in #49206. Perhaps this can become our "kitchen sink" interactive toy for debugging blurs in Aiks (where we can easily form clips and add options for other paint state interactions as they become relevant).

Video of `GaussianBlurRotatedAndClippedInteractive`:

https://github.com/flutter/engine/assets/919017/e5be5f38-3644-43c4-a3d0-d08b03fcb7b0

Video of `GaussianBlurFilter` with the "Combined sigma" checkbox to make playing with the new blur easier:

https://github.com/flutter/engine/assets/919017/65bd6567-83be-4337-8827-03e6a3dee9b1
@gaaclarke gaaclarke changed the title [Impeller] new blur: removed padding from backdrop filters [Impeller] new blur: refactored math and fixed expanded padding size Dec 20, 2023
@gaaclarke gaaclarke requested a review from bdero December 20, 2023 19:05
Copy link
Member

@bdero bdero 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 gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2023
@gaaclarke gaaclarke merged commit 7b6227f into flutter:main Dec 20, 2023
27 checks passed
gaaclarke added a commit that referenced this pull request Dec 20, 2023
gaaclarke added a commit to gaaclarke/engine that referenced this pull request Dec 20, 2023
dnfield pushed a commit that referenced this pull request Dec 20, 2023
gaaclarke added a commit to gaaclarke/engine that referenced this pull request Dec 20, 2023
auto-submit bot pushed a commit that referenced this pull request Dec 21, 2023
…ng size` (#49302)

#49206 had to reland because it had integration failures.  The changes are in a separate commit below.

[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 Dec 21, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 21, 2023
…140519)

Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions)

flutter/engine@c70f0a4...1b1b2a1

2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 4a10533a4dc8 to bcf68d22f0fa (1 revision) (flutter/engine#49329)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1d0c3ecd1349 to 4a10533a4dc8 (1 revision) (flutter/engine#49326)
2023-12-21 flar@google.com Ensure sorted rects in ui.Canvas for legacy compatibility (flutter/engine#49309)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 63a452b45026 to 1d0c3ecd1349 (1 revision) (flutter/engine#49318)
2023-12-21 dnfield@google.com [Impeller] Make IPLR files multi-platform (flutter/engine#49253)
2023-12-21 ditman@gmail.com [web] Defer injection of platform views until needed. (flutter/engine#48960)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1aef027ec953 to 63a452b45026 (2 revisions) (flutter/engine#49311)
2023-12-21 skia-flutter-autoroll@skia.org Roll Skia from 29917d8c97ca to 4b16117e94b2 (4 revisions) (flutter/engine#49310)
2023-12-21 737941+loic-sharma@users.noreply.github.com Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49262)
2023-12-21 30870216+gaaclarke@users.noreply.github.com Reland `[Impeller] new blur: refactored math and fixed expanded padding size` (flutter/engine#49302)
2023-12-20 dkwingsmt@users.noreply.github.com Multiview pipeline Pt. 1: Skip illegal render calls (flutter/engine#49266)
2023-12-20 barpac02@gmail.com SemanticsUpdateBuilder: make all args non-null (flutter/engine#49148)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] fixed Rect::Contains (flutter/engine#49294)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 97c3b7e1885a to 1aef027ec953 (1 revision) (flutter/engine#49295)
2023-12-20 30870216+gaaclarke@users.noreply.github.com Revert "[Impeller] new blur: refactored math and fixed expanded padding size" (flutter/engine#49298)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] new blur: refactored math and fixed expanded padding size (flutter/engine#49206)
2023-12-20 dkwingsmt@users.noreply.github.com Multi-view pointer event (flutter/engine#46213)
2023-12-20 1961493+harryterkelsen@users.noreply.github.com [web:multiview] Only call `Renderer.clearFragmentProgramCache` on hot restart (flutter/engine#48758)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 9cb1bb1164ea to 29917d8c97ca (1 revision) (flutter/engine#49289)
2023-12-20 bdero@google.com [Impeller] Add interactive Blur+Clip AiksTest. (flutter/engine#49283)
2023-12-20 sergiy.dubovik@supercell.com [macos] FlutterKeyboardManager memory leak fix (flutter/engine#48824)
2023-12-20 zanderso@users.noreply.github.com Don't guard Windows arm64 Dart SDK download on the release candidate flag (flutter/engine#49244)
2023-12-20 15619084+vashworth@users.noreply.github.com Fix testAppExtensionLaunching for Xcode 15/iOS 17 (flutter/engine#49242)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 8060d6b36066 to 9cb1bb1164ea (2 revisions) (flutter/engine#49288)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from ed415d966d8a to 97c3b7e1885a (1 revision) (flutter/engine#49287)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from d0f09ad481f7 to 8060d6b36066 (1 revision) (flutter/engine#49285)
2023-12-20 kevinjchisholm@gmail.com [release] Update release config (flutter/engine#49254)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 1732c4c92ccd to ed415d966d8a (1 revision) (flutter/engine#49274)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 4c59838945d9 to 1732c4c92ccd (1 revision) (flutter/engine#49269)
2023-12-20 goderbauer@google.com Sync lints with flutter/flutter (flutter/engine#49192)
2023-12-19 1961493+harryterkelsen@users.noreply.github.com [web] Enforce onDrawFrame/onBeginFrame render rule (flutter/engine#49214)
2023-12-19 barpac02@gmail.com [Docs] Add more info about running tests on iOS (flutter/engine#48859)

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 jimgraham@google.com,rmistry@google.com,zra@google.com 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
...
CoderDake pushed a commit to CoderDake/flutter that referenced this pull request Dec 28, 2023
…lutter#140519)

Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions)

flutter/engine@c70f0a4...1b1b2a1

2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 4a10533a4dc8 to bcf68d22f0fa (1 revision) (flutter/engine#49329)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1d0c3ecd1349 to 4a10533a4dc8 (1 revision) (flutter/engine#49326)
2023-12-21 flar@google.com Ensure sorted rects in ui.Canvas for legacy compatibility (flutter/engine#49309)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 63a452b45026 to 1d0c3ecd1349 (1 revision) (flutter/engine#49318)
2023-12-21 dnfield@google.com [Impeller] Make IPLR files multi-platform (flutter/engine#49253)
2023-12-21 ditman@gmail.com [web] Defer injection of platform views until needed. (flutter/engine#48960)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1aef027ec953 to 63a452b45026 (2 revisions) (flutter/engine#49311)
2023-12-21 skia-flutter-autoroll@skia.org Roll Skia from 29917d8c97ca to 4b16117e94b2 (4 revisions) (flutter/engine#49310)
2023-12-21 737941+loic-sharma@users.noreply.github.com Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49262)
2023-12-21 30870216+gaaclarke@users.noreply.github.com Reland `[Impeller] new blur: refactored math and fixed expanded padding size` (flutter/engine#49302)
2023-12-20 dkwingsmt@users.noreply.github.com Multiview pipeline Pt. 1: Skip illegal render calls (flutter/engine#49266)
2023-12-20 barpac02@gmail.com SemanticsUpdateBuilder: make all args non-null (flutter/engine#49148)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] fixed Rect::Contains (flutter/engine#49294)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 97c3b7e1885a to 1aef027ec953 (1 revision) (flutter/engine#49295)
2023-12-20 30870216+gaaclarke@users.noreply.github.com Revert "[Impeller] new blur: refactored math and fixed expanded padding size" (flutter/engine#49298)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] new blur: refactored math and fixed expanded padding size (flutter/engine#49206)
2023-12-20 dkwingsmt@users.noreply.github.com Multi-view pointer event (flutter/engine#46213)
2023-12-20 1961493+harryterkelsen@users.noreply.github.com [web:multiview] Only call `Renderer.clearFragmentProgramCache` on hot restart (flutter/engine#48758)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 9cb1bb1164ea to 29917d8c97ca (1 revision) (flutter/engine#49289)
2023-12-20 bdero@google.com [Impeller] Add interactive Blur+Clip AiksTest. (flutter/engine#49283)
2023-12-20 sergiy.dubovik@supercell.com [macos] FlutterKeyboardManager memory leak fix (flutter/engine#48824)
2023-12-20 zanderso@users.noreply.github.com Don't guard Windows arm64 Dart SDK download on the release candidate flag (flutter/engine#49244)
2023-12-20 15619084+vashworth@users.noreply.github.com Fix testAppExtensionLaunching for Xcode 15/iOS 17 (flutter/engine#49242)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 8060d6b36066 to 9cb1bb1164ea (2 revisions) (flutter/engine#49288)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from ed415d966d8a to 97c3b7e1885a (1 revision) (flutter/engine#49287)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from d0f09ad481f7 to 8060d6b36066 (1 revision) (flutter/engine#49285)
2023-12-20 kevinjchisholm@gmail.com [release] Update release config (flutter/engine#49254)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 1732c4c92ccd to ed415d966d8a (1 revision) (flutter/engine#49274)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 4c59838945d9 to 1732c4c92ccd (1 revision) (flutter/engine#49269)
2023-12-20 goderbauer@google.com Sync lints with flutter/flutter (flutter/engine#49192)
2023-12-19 1961493+harryterkelsen@users.noreply.github.com [web] Enforce onDrawFrame/onBeginFrame render rule (flutter/engine#49214)
2023-12-19 barpac02@gmail.com [Docs] Add more info about running tests on iOS (flutter/engine#48859)

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 jimgraham@google.com,rmistry@google.com,zra@google.com 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 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
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants