-
Notifications
You must be signed in to change notification settings - Fork 6k
[Flutter GPU] Raster encoding. First triangle! #48314
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we're comfortable with the API, there will need to be very thorough docstrings on the public API.
@@ -0,0 +1,124 @@ | |||
#include "flutter/lib/gpu/fixtures.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file needs a copyright header, and a comment with instructions on how to regenerate the arrays below. Instead of hardcoding this file, consider generating at build time like in #46862.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. This is a silly temp measure for provisioning a shader lib before I land the new shader bundle format.
Agreed. I'd like to continue foregoing docstrings for now while we feel out the rest of the API and have some representative examples that we're happy with. |
f5aa61c
to
c108fb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having an increasing fear that added functionality here with light testing will make the refactoring work we'll need to do in the engine later this year and next much harder.
lib/gpu/lib/src/formats.dart
Outdated
enum ShaderStage { | ||
unknown, | ||
vertex, | ||
fragment, | ||
tessellationControl, | ||
tessellationEvaluation, | ||
compute, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid exposing stages we don't have a plan for yet? I think this could just be vertex and fragment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/gpu/lib/src/render_pass.dart
Outdated
ColorAttachment( | ||
{this.loadAction = LoadAction.clear, | ||
this.storeAction = StoreAction.store, | ||
this.clearColor = const ui.Color(0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uber-nit
this.clearColor = const ui.Color(0), | |
this.clearColor = const ui.Color(0x00000000), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/gpu/lib/src/render_pass.dart
Outdated
switch (bufferView.buffer.runtimeType) { | ||
case DeviceBuffer: | ||
success = _bindUniformDevice( | ||
slot.shaderStage.index, | ||
slot.slotId, | ||
bufferView.buffer as DeviceBuffer, | ||
bufferView.offsetInBytes, | ||
bufferView.lengthInBytes); | ||
break; | ||
case HostBuffer: | ||
success = _bindUniformHost( | ||
slot.shaderStage.index, | ||
slot.slotId, | ||
bufferView.buffer as HostBuffer, | ||
bufferView.offsetInBytes, | ||
bufferView.lengthInBytes); | ||
break; | ||
default: | ||
throw Exception("Invalid buffer type"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patterns like this indicate this should probably be a method on Buffer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally I'd agree, but this needs to dispatch to different native methods tied to the RenderPass
handle. So then Buffer
would end up having something like a reverse _bindTo(gpu.RenderPass, gpu.BufferView)
that then calls back into the right RenderPass
native method, which feels a lot less straight forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a visitor pattern 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok ok, I'll bite. Done.
lib/gpu/lib/src/render_pass.dart
Outdated
} | ||
|
||
void bindVertexBuffer(BufferView bufferView, int vertexCount) { | ||
switch (bufferView.buffer.runtimeType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/gpu/lib/src/shader.dart
Outdated
int slotId; | ||
ShaderStage shaderStage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these fields feel like they should be final
and the class
const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
ShaderLibrary._(); | ||
|
||
Shader? operator [](String shaderName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use a method for this. operators, getters, and []
feel cheap even when they aren't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is sufficiently cheap? It just grabs a handle to an already existing object on the C++ side.
Oh, it'll definitely break, but I think it's OK for Flutter GPU to break at this early stage while we're feeling out the API. If you're working on API changes in the HAL, I actually think it would be acceptable to just remove the Flutter GPU target from the build script if fixing it causes too much friction, and I can come around later to fix it up. |
c108fb9
to
40da2c0
Compare
39fd774
to
9c22658
Compare
…138995) flutter/engine@b50fc4a...7743220 2023-11-25 bdero@google.com [Flutter GPU] Raster encoding. First triangle! (flutter/engine#48314) 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 jacksongardner@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
First triangle, in the framework! 🎉 Adds shader libraries, pipelines, command buffers, render passes, etc. * Light pipelines/shader objects. No optimization yet, pipeline warming to come. * "Dynamic" command style. Don't re-send bindings if you don't need to. Essentially: flutter/flutter#133179 * No need to explicitly encode passes. * Minimal descriptor usage. * Nothing is async, except for the optional command buffer completion callback. It took a bunch of experimenting to get here, but I think things are starting to look pretty neat. :) Todo: * Land the shader bundle format/remove the testing hacks & fixtures that piggyback off of the runtime effect system. * Add remaining calls for blend config, clearing bindings, etc. * Inconsistent error handling patterns that need cleanup. * Maybe: Surface exceptions for validation errors. * Handle the texture usage bitmask more elegantly.
…lutter#138995) flutter/engine@b50fc4a...7743220 2023-11-25 bdero@google.com [Flutter GPU] Raster encoding. First triangle! (flutter/engine#48314) 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 jacksongardner@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
This is part of the Flutter GPU MVP umbrella bug: flutter/flutter#130921 |
First triangle, in the framework! 🎉
Screen.Recording.2023-11-22.at.6.04.13.AM.mov
Adds shader libraries, pipelines, command buffers, render passes, etc.
It took a bunch of experimenting to get here, but I think things are starting to look pretty neat. :)
Todo:
This is the test code that's rendering this in the framework:
UnlitVertex:
UnlitFragment: