[Impeller] Flutter GPU: Add HostBuffer. #44696

bdero · 2023-08-14T19:54:58Z

Add impeller::HostBuffer wrapper to Flutter GPU.

Allows for lazy batch uploads of sparse host data to the GPU.
Handles platform alignment requirements.
API returns buffer view handles that will be fed to commands.

jonahwilliams · 2023-08-14T20:15:37Z

lib/gpu/lib/src/buffer.dart

+}
+
+/// A buffer that can be referenced by commands on the GPU.
+mixin Buffer {}


Generally speaking, a completely empty interface is a pretty big code smell.

What other kinds of buffers are we going to have? Are device buffers going to be usable in the same context as host buffers?

Yeah, DeviceBuffer is the other kind of buffer we need to support, and they will always be usable in the same context as HostBuffer.

I don't really know if this is how the final API is going to end up. My guess is that I'll work out something better when we add actual commands.

If you don't need it yet then don't add the interface yet.

jonahwilliams · 2023-08-14T20:16:24Z

lib/gpu/lib/src/buffer.dart

+  ///
+  /// The updated buffer will be uploaded to the GPU if the returned
+  /// [BufferView] is used by a rendering command.
+  BufferView emplaceBytes({required ByteData bytes}) {


Are there going to be any other arguments here? Can we make this a positional parameter instead so there is less ceremony?

Sure, done.

jonahwilliams · 2023-08-14T20:16:47Z

lib/gpu/lib/src/buffer.dart

+  /// Append byte data to the end of the [HostBuffer] and produce a [BufferView]
+  /// that references the new data in the buffer.
+  ///
+  /// This method automatically inserts padding in-between emplaced data if


in-between - can you clarify that this is in between subsequent emplace calls?

jonahwilliams · 2023-08-14T20:16:59Z

lib/gpu/lib/src/buffer.dart

+  ///
+  /// The updated buffer will be uploaded to the GPU if the returned
+  /// [BufferView] is used by a rendering command.
+  BufferView emplaceBytes({required ByteData bytes}) {


Nit: can we call this "emplace"?

Sure, done.

jonahwilliams · 2023-08-14T20:17:38Z

lib/gpu/lib/src/context.dart

+
+  /// Create a new [HostBuffer] that can be used for staging and transferring
+  /// data from the host to the GPU.
+  HostBuffer createHostBuffer() {


Does this need to be on the GPUContext? All it does is call the the HostBuffer constructor, which developers could just do themselves.

Removed for now. I added this here in the API draft because the other resource types (Texture and DeviceBuffer) need to be built with constructor methods.

jonahwilliams · 2023-08-14T20:18:10Z

lib/gpu/lib/src/buffer.dart

+/// A buffer that can be referenced by commands on the GPU.
+mixin Buffer {}
+
+/// [HostBuffer] is a [Buffer] which is allocated on the host and lazily


I would clarify what "host" means in this context, specifically: CPU resident memory native heap, not GPU memory or Dart heap.

jonahwilliams · 2023-08-14T20:18:30Z

lib/gpu/lib/src/buffer.dart

+
+  /// Create a new view into a buffer on the GPU.
+  const BufferView(
+      {required this.buffer,


nit: make the buffer argument positional, leave the others are required named parameters.

Done. Your suggestion is nicer IMO, but do we happen to have any guidelines for this kind of thing I should seek to align myself with?

I'm not sure that there is a hard and fast rule for this, but I would think about named parameters in cases where the literal value or variable name itself does not give enough clarirty on what the argument is. The canonical example would be something like:

void foo(true, false)

What the heck are true / false doing here? So similar to how we might add a comment in the c++ code like

void foo(/*arg_name=*/true, /*arg_name=*/false), then we can use named arguments to make the Dart code clearer:

void foo(argName: true, argName2, false).

So using the buffer constructor argument above, we've got something like:

BufferView(buffer, 0, 12). The buffer itself is obvious, but the numbers benefit from clarification.

jonahwilliams · 2023-08-14T20:18:43Z

lib/gpu/lib/src/buffer.dart

+
+import 'context.dart';
+
+/// A buffer data range.


This is just restating the name of the class.

Made this comment a bit more informative.

bdero · 2023-08-15T00:16:03Z

FYI @Hixie, I'm starting to land prototype patches for Flutter GPU this week. Roping you in since you mentioned you'd like to review the API. :)

Hixie · 2023-08-15T00:20:06Z

Indeed! Is there a general overview of the whole intended API surface? Is this PR the place to start? Provide me with context. :-)

jonahwilliams · 2023-08-15T01:24:37Z

lib/gpu/lib/src/context.dart

+import 'buffer.dart';
+


nit: unused

jonahwilliams · 2023-08-15T01:25:49Z

lib/gpu/lib/src/buffer.dart

+  /// The length of the view.
+  final int lengthInBytes;
+
+  /// Create a new view into a buffer on the GPU.


its not really "on the gpu though"? For more functional pieces like the buffer view it may be easier to document once there are things that can be done with it. for example, "a view into a buffer that can be used for binding uniform data yada yada"

I think BufferViews are effectively GPU-only because they'll only be used for binding buffers to Commands. I don't think we'd use a BufferView for updating a range on the host buffer, for example.

jonahwilliams

LGTM

bdero · 2023-08-15T03:48:43Z

@Hixie Sure, the intention is for the API to just be a subset of Impeller's hardware abstraction layer (located under impeller/renderer).

A good jumping off point would be the Detailed Design/Discussion part of the doc. I started appending some more detail above the usage example today.

The entire initial API surface needed for implementing a renderer like the one built for the FF Demo is covered by the usage example, although it has a couple of lingering TODOs floating around for minor tricky bits.

I also added a few designs for compositing Flutter GPU textures into the widget tree here. Higher up is more preferred.

Hixie · 2023-08-15T15:59:05Z

I'm in meetings non-stop today but I've scheduled some time on Wednesday to study this in depth.

bdero · 2023-08-16T16:56:18Z

@Hixie Cool with you if I continue merging things and prototyping the API in the meantime? Everything is still subject to change as none of this stuff is exposed to Flutter users. I'll continue CCing you on PRs as well, etc.

bdero · 2023-08-16T17:32:08Z

Ah, I just noticed the block of time on your cal for this. I'll leave it be for now

jonahwilliams · 2023-08-16T18:13:10Z

lib/gpu/lib/src/buffer.dart

+///       accessing device buffer data. The [HostBuffer] takes these
+///       requirements into account and automatically inserts padding between
+///       emplaced data if necessary.
+class HostBuffer extends NativeFieldWrapperClass1 {


Something that might be worth covering in the design, but I think almost all of these NativeFieldWrapper classes should be sealed. This is a design defect with the current dart:ui implementation, because classes which implement the native interface may cause no such method errors/crashes if they flow to real implemenations (i.e. drawPicture on a picture that implements ui.Picture).

Oh, do you mean mark NativeFieldWrapper classes as base to avoid the problems mentioned in: flutter/flutter#123756? I think the weird class hierarchy solution that ui.Picture moved to is something we could safely migrate to if we wind up having the same testing needs.
Marked HostBuffer as base.

Hixie · 2023-08-16T18:44:41Z

lib/gpu/lib/src/buffer.dart

+/// This is useful for efficiently chunking sparse data uploads, especially
+/// ephemeral uniform data that needs to change from frame to frame.
+///
+/// Note: Different platforms have different data alignment requirements for


trivial nit: per https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-empty-prose, avoid the Note bit. (Also I suspect indenting things like this will have a very different effect in the output than you might expect.)

bdero · 2023-08-23T17:08:56Z

Merging this to unblock the rest of this prototyping surface. This is experimental and not accessible by users.

…133211) flutter/engine@58dc868...27d75f6 2023-08-23 [email protected] Roll Skia from 76898dad9fda to a631fefdba37 (2 revisions) (flutter/engine#45027) 2023-08-23 [email protected] Revert "FontVariation.lerp, custom FontVariation constructors, and more documentation" (flutter/engine#45023) 2023-08-23 [email protected] [Impeller] Dat rvalue reference (fix engine head) (flutter/engine#45024) 2023-08-23 [email protected] Revert "Enable clang-tidy for pre-push (opt-out), exclude `performance-unnecessary-value-param`" (flutter/engine#45020) 2023-08-23 [email protected] Roll Skia from 4e42b51cfe27 to 76898dad9fda (1 revision) (flutter/engine#45019) 2023-08-23 [email protected] [Impeller] Add STB text backend. (flutter/engine#44887) 2023-08-23 [email protected] Followup to flutter/engine#44982 (flutter/engine#45018) 2023-08-23 [email protected] Roll Skia from 5428f147e632 to 4e42b51cfe27 (4 revisions) (flutter/engine#45016) 2023-08-23 [email protected] Eliminate android test log spam (flutter/engine#44982) 2023-08-23 [email protected] [web] Remove some unused functions (flutter/engine#44505) 2023-08-23 [email protected] Use decal TileMode in blur image_filter_test.dart (flutter/engine#45004) 2023-08-23 [email protected] FontVariation.lerp, custom FontVariation constructors, and more documentation (flutter/engine#44996) 2023-08-23 [email protected] [impeller] combine sampler and texture maps. (flutter/engine#44990) 2023-08-23 [email protected] [Impeller] Flutter GPU: Add HostBuffer. (flutter/engine#44696) 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://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

Resolves flutter/flutter#132516. Add `impeller::HostBuffer` wrapper to Flutter GPU. * Allows for lazy batch uploads of sparse host data to the GPU. * Handles platform alignment requirements. * API returns buffer view handles that will be fed to commands.

[Impeller] Flutter GPU: Add HostBuffer.

b47554b

bdero added the e: impeller label Aug 14, 2023

bdero requested review from chinmaygarde, jonahwilliams and zanderso August 14, 2023 19:54

bdero self-assigned this Aug 14, 2023

jonahwilliams reviewed Aug 14, 2023

View reviewed changes

bdero added 2 commits August 14, 2023 15:22

Licenses

871262d

Address comments

0841143

bdero requested a review from jonahwilliams August 14, 2023 22:25

bdero force-pushed the bdero/fluttergpu-host-buffer branch 2 times, most recently from 1624ba0 to 2a4cc46 Compare August 14, 2023 23:09

bdero requested a review from Hixie August 15, 2023 00:16

jonahwilliams reviewed Aug 15, 2023

View reviewed changes

jonahwilliams approved these changes Aug 15, 2023

View reviewed changes

Remove unused import

31e8f79

bdero force-pushed the bdero/fluttergpu-host-buffer branch from 2a4cc46 to 31e8f79 Compare August 15, 2023 03:51

jonahwilliams reviewed Aug 16, 2023

View reviewed changes

Hixie reviewed Aug 16, 2023

View reviewed changes

bdero force-pushed the bdero/fluttergpu-host-buffer branch from 7d95ad3 to 073507d Compare August 23, 2023 01:06

Address comments

631778d

bdero force-pushed the bdero/fluttergpu-host-buffer branch from 073507d to 631778d Compare August 23, 2023 01:35

bdero merged commit f9264f0 into flutter:main Aug 23, 2023

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Roll Flutter Engine from 58dc868c26cb to 5a4bbd0d25ad (2 revisions) flutter/flutter#133172

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

f9264f01b [Impeller] Flutter GPU: Add HostBuffer. (flutter/engine#44696)

5aa0d4f

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Roll Flutter Engine from 58dc868c26cb to ac1ad18131f1 (3 revisions) flutter/flutter#133174

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

f9264f01b [Impeller] Flutter GPU: Add HostBuffer. (flutter/engine#44696)

4785ef9

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Roll Flutter Engine from 58dc868c26cb to 25fb06ebdb36 (4 revisions) flutter/flutter#133177

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

f9264f01b [Impeller] Flutter GPU: Add HostBuffer. (flutter/engine#44696)

816e3bb

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Roll Flutter Engine from 58dc868c26cb to 591bf9f64bc6 (5 revisions) flutter/flutter#133184

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

f9264f01b [Impeller] Flutter GPU: Add HostBuffer. (flutter/engine#44696)

a2feaae

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Roll Flutter Engine from 58dc868c26cb to d35b01f16761 (6 revisions) flutter/flutter#133185

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

f9264f01b [Impeller] Flutter GPU: Add HostBuffer. (flutter/engine#44696)

e535027

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Roll Flutter Engine from 58dc868c26cb to a2adaa39a2c3 (7 revisions) flutter/flutter#133187

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

f9264f01b [Impeller] Flutter GPU: Add HostBuffer. (flutter/engine#44696)

079738d

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Roll Flutter Engine from 58dc868c26cb to a42ddc1962c4 (8 revisions) flutter/flutter#133192

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

f9264f01b [Impeller] Flutter GPU: Add HostBuffer. (flutter/engine#44696)

5e9198b

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Roll Flutter Engine from 58dc868c26cb to a42ddc1962c4 (8 revisions) flutter/flutter#133197

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

f9264f01b [Impeller] Flutter GPU: Add HostBuffer. (flutter/engine#44696)

5d34aa4

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Roll Flutter Engine from 58dc868c26cb to be7c280b0e12 (13 revisions) flutter/flutter#133205

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

f9264f01b [Impeller] Flutter GPU: Add HostBuffer. (flutter/engine#44696)

200f6cc

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Roll Flutter Engine from 58dc868c26cb to 27d75f6221d2 (14 revisions) flutter/flutter#133211

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

f9264f01b [Impeller] Flutter GPU: Add HostBuffer. (flutter/engine#44696)

2011f22

reidbaker mentioned this pull request Aug 25, 2023

Update to very new gradle version #45124

Closed

8 tasks

		import 'buffer.dart';

[Impeller] Flutter GPU: Add HostBuffer. #44696

[Impeller] Flutter GPU: Add HostBuffer. #44696

Uh oh!

Conversation

bdero commented Aug 14, 2023

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!

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!

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!

bdero commented Aug 15, 2023

Uh oh!

Hixie commented Aug 15, 2023

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!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

bdero commented Aug 15, 2023

Uh oh!

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

Uh oh!

Uh oh!

bdero commented Aug 16, 2023

Uh oh!

bdero commented Aug 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdero Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Hixie commented Aug 15, 2023 •

edited

Loading

bdero Aug 23, 2023 •

edited

Loading