[Impeller] Fix 1d threadgroup dispatch size, Implement drawPoint geometry computation with compute, fallback to specialized CPU. by jonahwilliams · Pull Request #42060 · flutter/engine

jonahwilliams · 2023-05-15T22:14:56Z

Compute shader implementation of #41803

I found this to be generally more straightforward, and we can revisit the vertex shader approach with GLES.

Benchmark is pending here: flutter/flutter#126728

Note: rendering behavior is identical even though this is now a single draw call. This is becuase strokes have overdraw prevention enabled by default, while this new contents does not

dnfield · 2023-05-18T20:14:02Z

impeller/entity/shaders/geometry/points.comp

+layout(local_size_x = 16) in;
+
+layout(std430) readonly buffer PointData {
+  vec2 points[];


You should include a uint count here to guard against OOB access.

Oh, I see, you put it as a uniform.

(Consider adding a comment that the count for this is in the uniform)

dnfield · 2023-05-18T20:14:11Z

impeller/entity/shaders/geometry/points.comp

+point_data;
+
+layout(std430) writeonly buffer GeometryData {
+  vec2 geometry[];


Probably want a count here too?

See comment

dnfield · 2023-05-18T20:14:55Z

impeller/entity/shaders/geometry/points.comp

+  // TODO(dnfield): https://github.com/flutter/flutter/issues/112683
+  // We should be able to use length here instead of an extra arrgument.


I kind of doubt this will ever get fixed, we should probably just remove these.

We can actually ifdef/function constant these out based on a runtime flag if the device supports non-uniform threadgroup sizing.

dnfield

Neat! Mostly nits

…pute

…nto draw_with_compute

jonahwilliams · 2023-05-19T04:12:03Z

I found a problem with how dispatch was being done. We were setting the threadGroup size as if it were the threadsize, which resulted in us doing way more work than necessary. I've now fixed this for one dimension, so that the thread group size is correctly computed as the units of work / maxThreadgroupSize. This is still broken for 2D. fyi @dnfield

jonahwilliams · 2023-05-19T04:14:14Z

impeller/renderer/backend/metal/compute_pass_mtl.mm


    // For now, check that the sizes are uniform.
-    FML_DCHECK(grid_size == thread_group_size);
+    // FML_DCHECK(grid_size == thread_group_size);


I think we should rethink this API. What would make sense to me is 1. specifying the units of work in a grid size.

Maybe there are cases where we need to control threadgroup size? not sure

dnfield · 2023-05-19T16:34:19Z

impeller/renderer/backend/metal/compute_pass_mtl.mm

+      }
+
+      auto size = MTLSizeMake(width, height, 1);
+      [encoder dispatchThreadgroups:size threadsPerThreadgroup:size];


This is ok but we should really fix this in a separate PR

…int geometry computation with compute, fallback to specialized CPU. (flutter/engine#42060)

flutter/engine@2b14f8a...3267fa2 2023-05-19 bdero@google.com [Impeller] Limit blur kernel to 1000x1000 pixels (flutter/engine#42154) 2023-05-19 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from IGeTvUK2wkajmQQ2k... to pwdDQgM88sqLmZczj... (flutter/engine#42163) 2023-05-19 skia-flutter-autoroll@skia.org Roll Skia from 0a63dbe8cd15 to 7202b405f061 (1 revision) (flutter/engine#42162) 2023-05-19 jonahwilliams@google.com [Impeller] Fix 1d threadgroup dispatch size, Implement drawPoint geometry computation with compute, fallback to specialized CPU. (flutter/engine#42060) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from IGeTvUK2wkaj to pwdDQgM88sqL 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…7224) flutter/engine@2b14f8a...3267fa2 2023-05-19 bdero@google.com [Impeller] Limit blur kernel to 1000x1000 pixels (flutter/engine#42154) 2023-05-19 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from IGeTvUK2wkajmQQ2k... to pwdDQgM88sqLmZczj... (flutter/engine#42163) 2023-05-19 skia-flutter-autoroll@skia.org Roll Skia from 0a63dbe8cd15 to 7202b405f061 (1 revision) (flutter/engine#42162) 2023-05-19 jonahwilliams@google.com [Impeller] Fix 1d threadgroup dispatch size, Implement drawPoint geometry computation with compute, fallback to specialized CPU. (flutter/engine#42060) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from IGeTvUK2wkaj to pwdDQgM88sqL 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

jonahwilliams added 30 commits May 7, 2023 12:34

[Impeller] compute circles in vertex shader

961008c

++

56638b0

capabilities and license

be1332a

add support for square caps

7a83b0d

++

d36a452

++

69ea8a9

++

6a830a7

switch to enum class and add docs

5cec1a4

fix offsets

de05b5c

Merge branch 'master' of github.com:flutter/engine into draw_circles

94b0808

use different render pass

f0a8297

Merge branch 'master' of github.com:flutter/engine into draw_circles

96fea2f

fix merge

faba06f

add CPU fallback

a8e4106

++

8b8f4f8

fix off by one

b23b616

non working index buffer removal

0591bf1

fix no index

0fc1ee2

++

6d979a2

Merge branch 'master' of github.com:flutter/engine into draw_circles

75ce9fb

++

a2f218d

Rename field

cbb0aa6

Merge branch 'master' of github.com:flutter/engine into draw_circles

542b465

improve efficiency

8799795

++

b715d40

disable on desktop

7693925

Merge branch 'master' of github.com:flutter/engine into draw_circles

8cc4e81

fix geometry computation bug

6f46186

malioc update

cca59fe

++

f08fff2

dnfield reviewed May 18, 2023

View reviewed changes

jonahwilliams added 4 commits May 18, 2023 15:11

dnfield review

c907260

Merge branch 'main' into draw_with_compute

1f0a9b6

++

40ef2b4

Merge branch 'master' of github.com:flutter/engine into draw_with_com…

e788b29

…pute

jonahwilliams requested a review from dnfield May 18, 2023 22:11

Merge branch 'draw_with_compute' of github.com:jonahwilliams/engine i…

ca70a31

…nto draw_with_compute

dnfield approved these changes May 19, 2023

View reviewed changes

fix threadgroup size for 1d dispatch

14e76fc

jonahwilliams changed the title ~~[Impeller] Implement drawPoint geometry computation with compute, fallback to specialized CPU.~~ [Impeller] Fix 1d threadgroup dispatch size, Implement drawPoint geometry computation with compute, fallback to specialized CPU. May 19, 2023

jonahwilliams commented May 19, 2023

View reviewed changes

jonahwilliams added 4 commits May 18, 2023 21:14

Remove commented out code

6e82927

dont use std numeric limits

96921e2

++

0275a10

++

e4fae27

jonahwilliams requested a review from dnfield May 19, 2023 15:28

dnfield reviewed May 19, 2023

View reviewed changes

dnfield approved these changes May 19, 2023

View reviewed changes

Merge branch 'main' into draw_with_compute

f9e57a2

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label May 19, 2023

auto-submit bot merged commit 0166141 into flutter:main May 19, 2023

engine-flutter-autoroll mentioned this pull request May 19, 2023

Roll Flutter Engine from 2b14f8a1f21c to 3267fa29491a (4 revisions) flutter/flutter#127224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

[Impeller] Fix 1d threadgroup dispatch size, Implement drawPoint geometry computation with compute, fallback to specialized CPU.#42060

[Impeller] Fix 1d threadgroup dispatch size, Implement drawPoint geometry computation with compute, fallback to specialized CPU.#42060
auto-submit[bot] merged 50 commits intoflutter:mainfrom
jonahwilliams:draw_with_compute

jonahwilliams commented May 15, 2023 •

edited

Loading

Uh oh!

dnfield May 18, 2023

Uh oh!

dnfield May 18, 2023

Uh oh!

dnfield May 18, 2023

Uh oh!

jonahwilliams May 18, 2023

Uh oh!

dnfield May 18, 2023

Uh oh!

jonahwilliams May 18, 2023

Uh oh!

dnfield May 18, 2023

Uh oh!

jonahwilliams May 18, 2023

Uh oh!

dnfield left a comment

Uh oh!

jonahwilliams commented May 19, 2023

Uh oh!

jonahwilliams May 19, 2023

Uh oh!

dnfield May 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// TODO(dnfield): https://github.com/flutter/flutter/issues/112683
		// We should be able to use length here instead of an extra arrgument.

Comments

Conversation

jonahwilliams commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented May 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonahwilliams commented May 15, 2023 •

edited

Loading