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

[Impeller] new blur: implemented ping ponging #49252

Merged
merged 8 commits into from
Jan 2, 2024

Conversation

gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Dec 19, 2023

This will reuse the downsample texture for the blur pass.

issue: flutter/flutter#140189

Testing: This has golden image coverage for the refactoring. There is a slight performance difference which will show up in benchmarks. Ideally there would also be a memory test for the blur. I don't think there is one yet.

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 performance measurement here. GPU time doesn't seem much effected. We'll have to measure the memory usage:

    "average_vsync_transitions_missed": 5.671875,
    "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": 16635.254575707153,
    "90th_percentile_frame_request_pending_latency": 16724.0,
    "99th_percentile_frame_request_pending_latency": 16767.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": 3.064,
    "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": 82.51953125,
    "90th_percentile_gpu_frame_time": 125.0,
    "99th_percentile_gpu_frame_time": 125.0,
    "worst_gpu_frame_time": 125.0,
    "average_cpu_usage": 121.87708345833335,
    "90th_percentile_cpu_usage": 127.2,
    "99th_percentile_cpu_usage": 130.499998,
    "average_gpu_usage": 100.0,
    "90th_percentile_gpu_usage": 100.0,
    "99th_percentile_gpu_usage": 100.0,
    "average_memory_usage": 101.67024739583333,
    "90th_percentile_memory_usage": 117.0625,
    "99th_percentile_memory_usage": 121.25

@gaaclarke
Copy link
Member Author

Here the memory usage is much lower than before (644MB vs 914MB):

Screenshot 2023-12-19 at 1 15 53 PM

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. I haven't run this patch locally but how this implementation makes sense to me.

@@ -405,13 +405,12 @@ bool ContentContext::IsValid() const {
return is_valid_;
}

std::shared_ptr<Texture> ContentContext::MakeSubpass(
fml::StatusOr<RenderTarget> ContentContext::MakeSubpass(
Copy link
Member

Choose a reason for hiding this comment

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

I see, this is to make just reusing the same RenderTarget easy in the blur. I think this is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I had to do RenderTarget instead of Texture because of MSAA.


if (!pass3_out.ok()) {
return std::nullopt;
}
Copy link
Member

Choose a reason for hiding this comment

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

Ok, so I believe all of this works out such that all three passes should always wind up attaching color textures of size subpass_size.

Since this optimization is contingent on the sizes being the same in order to render correctly, could you add a DCHECK that verifies the color attachment of the three passes is subpass_size?

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.

@gaaclarke gaaclarke marked this pull request as ready for review January 2, 2024 18:22
@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 2, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 2, 2024
Copy link
Contributor

auto-submit bot commented Jan 2, 2024

auto label is removed for flutter/engine/49252, due to - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

@gaaclarke gaaclarke merged commit 9d89f87 into flutter:main Jan 2, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 2, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 2, 2024
…140818)

flutter/engine@a06926d...9d89f87

2024-01-02 30870216+gaaclarke@users.noreply.github.com [Impeller] new blur: implemented ping ponging (flutter/engine#49252)

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 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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants