-
Notifications
You must be signed in to change notification settings - Fork 6k
[impeller] Implement an OpenGL ES 2.0 backend. #33084
Conversation
dnfield
left a comment
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 LGTM to land and iterate on. I have not closely read every line of the patch though.
|
License bot is angry |
bdero
left a comment
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.
Only minor nits and questions -- everything is looking great. Extremely excited to land this!
| // |DeviceBuffer| | ||
| DeviceBufferGLES::~DeviceBufferGLES() { | ||
| if (!reactor_) { | ||
| reactor_->CollectHandle(handle_); |
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 seems like a pretty common pattern to have these handles tied 1-1 with the corresponding resource class. Would it make sense to have a special CreateHandleUnique which returns a unique_ptr<Handle, Deleter> that can perform this collection?
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.
Agreed. Fixed.
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.
On second thought. I am having trouble with the ownership of the reactor when this handle is vended. There are only a couple of uses of this pattern. I think we should think of the unique_ptr at a later point.
| kSubtract, | ||
| kReverseSubtract, | ||
| kMin, | ||
| kMax, |
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 reminds me: At some point we gotta talk about a strategy for extensions. We can make all the advanced blends done in-pipeline with EXT_blend_minmax + KHR_blend_equation_advanced for GLES and avoid the extra render passes down the road.
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.
Thats a good point. From reading the spec, I believe KHR_blend_equation_advanced_coherent will be necessary right? In any case, I left these out because I could not find anything similar for Metal or Vulkan (and hadn't considered other API's like DX12). If we are going to need to work with these extensions anyway, I'd rather leave them out now and devise a separate strategy for extensions. I really don't want to be super prescriptive about not using extensions though. If they are available and avoid render passes, then I'm all for it. Could you please file a bug for this?
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.
SGTM, this is mentioned in flutter/flutter#100086 but probably belongs in a separate issue.
This is the first pass at implementing an Impeller backend that restricts itself to the feature set available in OpenGL ES 2.0. This means there is no use of features that we will end up using where available. These features include vertex array objects, uniform buffer objects, frame buffer discard operations, etc.. As written, this backend should support 100% of the Android devices out in the wild. Using VAO's and UBO's (in a future patch) where available should benefit 90.91% of the devices in the wild. Beyond that, I don't believe it makes too much sense to invest in this backend. This was a fairly straightforward implementation of an Impeller backend with the following items that are perhaps unique: * Since there are no uniform buffer objects in ES2, the offline reflector in `impellerc` generates additional metadata for backends like ES2 to find and bind uniform data in existing device buffers. Commands reference this data in their stage bindings but only via a pointer to a const object. Backends that don't need this metadata don't see any additional overhead. In the OpenGL ES backend, deletion of the buffers that contain uniform data is delayed till uniform data is bound via the glUniform* family of functions. * Unlike Skia objects that reference GPU resources, Impeller objects can be created, used, and collected on any thread. All OpenGL operations happen in the `impeller::ReactorGLES`. This makes it easy to deal with context loss as we don't have to chase references to resources or ensure resources are collected on the right thread. Impeller resources (like textures, pipelines, buffers, etc.) that reference GPU resources will just reference dead handles on context loss and can be collected when convenient. * Instead of not being able to use settings in high level API based on the backend in use, the specific settings (which were as yet unused) have been removed. This include `BlendOperation::kMin` and `BlendOperation::kMax` which require extensions in OpenGL ES 2.0. The addition of this new backend doesn't affect how the higher levels in the stack (like the renderer, entity, aiks, display_list, etc.) function. * This backend performs an additional copy of buffer data since it need to push operations into the reactor. This copy will be removed when the appropriate APIs are updated (in a later patch) to use mappings and buffer views (flutter/flutter#102262). Memory usage of this backend will go down significantly after that patch. Even more still on devices where UBO's are available. * This backend will generate separate program objects for pipeline variants. This makes no sense for OpenGLES as that state is set separately per draw call. Reusing of the same program handle for pipeline variants is as yet unimplemented. * The playground test harness that was previously parameterized now has the `PlaygroundBackend::kOpenGLES` parameter. This effectively doubles the playground tests corpus. * There is error validation after each call to OpenGL. Ideally, this should be moved to debug runtime modes. * Capturing RenderDoc traces is hard in the playground because both SwiftShader is also linked in. Devising a strategy for capturing RenderDoc traces from the playgrounds is pending. TL;DR: Backend is ready for review. Needs debugging and additional refactors (already listed) to bring memory usage back down to baseline (relative to the Metal backend). Existing API and backends unaffected.
This is the first pass at implementing an Impeller backend that restricts itself
to the feature set available in OpenGL ES 2.0. This means there is no use of
features that we will end up using where available. These features include
vertex array objects, uniform buffer objects, frame buffer discard operations,
etc..
As written, this backend should support 100% of the Android devices out in the

wild. Using VAO's and UBO's (in a future patch) where available should benefit
90.91% of the devices in the wild. Beyond that, I don't believe it makes too
much sense to invest in this backend.
This was a fairly straightforward implementation of an Impeller backend with the
following items that are perhaps unique:
impellercgenerates additional metadata for backends like ES2 to find andbind uniform data in existing device buffers. Commands reference this data in
their stage bindings but only via a pointer to a const object. Backends that
don't need this metadata don't see any additional overhead. In the OpenGL ES
backend, deletion of the buffers that contain uniform data is delayed till
uniform data is bound via the glUniform* family of functions.
created, used, and collected on any thread. All OpenGL operations happen in
the
impeller::ReactorGLES. This makes it easy to deal with context loss aswe don't have to chase references to resources or ensure resources are
collected on the right thread. Impeller resources (like textures, pipelines,
buffers, etc.) that reference GPU resources will just reference dead handles
on context loss and can be collected when convenient. The reactor is still
WIP and needs additional synchronization and tests for thread safety.
backend in use, the specific settings (which were as yet unused) have been
removed. This include
BlendOperation::kMinandBlendOperation::kMaxwhichrequire extensions in OpenGL ES 2.0. The addition of this new backend doesn't
affect how the higher levels in the stack (like the renderer, entity, aiks,
display_list, etc.) function.
operations into the reactor. This copy will be removed when the appropriate
APIs are updated (in a later patch) to use mappings and buffer views. Memory
usage of this backend will go down significantly after that patch. Even more still
on devices where UBO's are available.
This makes no sense for OpenGLES as that state is set separately per draw
call. Reusing of the same program handle for pipeline variants is as yet
unimplemented.
PlaygroundBackend::kOpenGLESparameter. This effectively doubles theplayground tests corpus.
moved to debug runtime modes.
is also linked in. Devising a strategy for capturing RenderDoc traces from the
playgrounds is pending.
TL;DR: Backend is ready for review. Needs debugging and additional refactors

(already listed) to bring memory usage back down to baseline (relative to the
Metal backend). Existing API and backends unaffected.