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

Conversation

@chinmaygarde
Copy link
Member

@chinmaygarde chinmaygarde commented May 3, 2022

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.
Screen Shot 2022-05-03 at 2 32 38 PM

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. The reactor is still
    WIP and needs additional synchronization and tests for thread safety.
  • 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. 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.
Screen Shot 2022-05-04 at 12 43 33 PM

Copy link
Contributor

@dnfield dnfield left a 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.

@dnfield
Copy link
Contributor

dnfield commented May 3, 2022

License bot is angry

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.

Only minor nits and questions -- everything is looking great. Extremely excited to land this!

// |DeviceBuffer|
DeviceBufferGLES::~DeviceBufferGLES() {
if (!reactor_) {
reactor_->CollectHandle(handle_);
Copy link
Member

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?

Copy link
Member Author

@chinmaygarde chinmaygarde May 4, 2022

Choose a reason for hiding this comment

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

Agreed. Fixed.

Copy link
Member Author

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,
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.
@chinmaygarde chinmaygarde added waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. and removed waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. labels May 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

e: impeller waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants