Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] Add support to embedder for Impeller on GL (via Angle on Windows). #43388

Merged
merged 32 commits into from
Jul 13, 2023

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Jul 3, 2023

Actually works now, though there is some issue with the default fbo stencil so I've filled flutter/flutter#130048

Other issues:

  • Rendering looks wrong
  • Resizing window hangs
  • Reactor isn't set up correctly and all blit passes are currently failing.
  • Needs to handle falling back to sample count of 1 like we do on Android.

@chinmaygarde chinmaygarde changed the title [Impeller] support Impeller via GL/Angle on Windows. [Impeller] Support Impeller via GL/Angle on Windows. Jul 3, 2023
// |EmbedderSurface|
sk_sp<GrDirectContext> EmbedderSurfaceGLImpeller::CreateResourceContext()
const {
sk_sp<GrDirectContext> resource_context;
Copy link
Member Author

Choose a reason for hiding this comment

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

This seems wrong but I'm not sure how else to make it work. 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need this.

You should just see if you can make the resource context current, and if you can mark the worker as allowed to react on this thread. And then always return nullptr.

Returning a valid context from here should not be required and we should not be creating a GrContext when using Impeller.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh OK, that works and makes much more sense. I briefly read through the code and thought this still needed to be constructed

Copy link
Contributor

Choose a reason for hiding this comment

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

The resource context is a performance improving optional thing. Impeller handles things differently by just using worker threads and the reactor mechanism - Skia OTOH expects you to manage a context for the IO thread.

Returning nullptr here should be tolerated anywhere. We may eventually want some better way to signal what this method is actually doing but right now it's a little hard to think about the best way to clean up all the duplicated branches where we've baked Skia concepts into things (like methods that return nullptr for GrContexts in various spots, or nullptr for impeller::Contexts in others).

@@ -80,10 +80,14 @@ std::unique_ptr<SurfaceFrame> GPUSurfaceGLImpeller::AcquireFrame(
return nullptr;
}

GLFrameInfo frame_info = {static_cast<uint32_t>(size.width()),
static_cast<uint32_t>(size.height())};
const GLFBOInfo fbo_info = delegate_->GLContextFBO(frame_info);
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding this is enough to get window resizing working.

@jonahwilliams jonahwilliams marked this pull request as ready for review July 10, 2023 00:53
@jonahwilliams
Copy link
Member Author

Ahh the problem was the #ifdef instead of #if

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 12, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 12, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 12, 2023

auto label is removed for flutter/engine, pr: 43388, due to - The status or check suite Linux Fuchsia FEMU has failed. Please fix the issues identified (or deflake) before re-applying this label.

@jonahwilliams
Copy link
Member Author

ahh I guess not, linux fuchsia seems to be trying to enable impeller :(

@jonahwilliams jonahwilliams changed the title [Impeller] Support Impeller via GL/Angle on Windows. [Impeller] Add support to embedder for Impeller on GL (via Angle on Windows). Jul 12, 2023
@jonahwilliams
Copy link
Member Author

@dnfield I put the nognchecks back. I'm not sure exactly how to untangle this, but it seems like its mostly a problem with however we're configuring fuchsia builds. Impeller ends up in a partially enabled state and that is confusing the build rules.

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 13, 2023
@auto-submit auto-submit bot merged commit e478f1b into flutter:main Jul 13, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2023
fluttermirroringbot pushed a commit to flutter/flutter that referenced this pull request Jul 13, 2023
…130496)

Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions)

flutter/engine@16e2ab7...b408036

2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from e5ec341bc3ca to 56b68ce6196c (1 revision) (flutter/engine#43633)
2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from f499e91e8cb2 to ade4dae923f3 (1 revision) (flutter/engine#43632)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 811b046c673b to e5ec341bc3ca (1 revision) (flutter/engine#43631)
2023-07-13 ian@hixie.ch Make GOMA state automatic by default (flutter/engine#43584)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from c8da0c657c4e to 811b046c673b (3 revisions) (flutter/engine#43630)
2023-07-13 chinmaygarde@google.com [Impeller] Remove unactionable error logs and use structure chains for instance creation. (flutter/engine#43629)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 7f391ea9164e to c8da0c657c4e (1 revision) (flutter/engine#43628)
2023-07-13 chinmaygarde@google.com [Impeller] Add RAII wrappers for VMA objects. (flutter/engine#43626)
2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 1STsUj0X5YgpiSNEb... to xBJq6PsO5ebblODMe... (flutter/engine#43627)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 6ed93436d57c to 7f391ea9164e (1 revision) (flutter/engine#43625)
2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from 8f8f281ccdc6 to f499e91e8cb2 (3 revisions) (flutter/engine#43623)
2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 0fvk838jTDNQ_l43k... to 3C7P0w8ySmtqpyi3S... (flutter/engine#43622)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from bedc92598644 to 6ed93436d57c (3 revisions) (flutter/engine#43621)
2023-07-13 jonahwilliams@google.com [Impeller] Add support to embedder for Impeller on GL (via Angle on Windows). (flutter/engine#43388)
2023-07-12 jonahwilliams@google.com [Impeller] Allocate buffers out of a pool on the raster thread. (flutter/engine#43564)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617)
2023-07-12 flar@google.com move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614)
2023-07-12 ian@hixie.ch Document (and assert) that channel names can't contains nulls (flutter/engine#43593)
2023-07-12 58529443+srujzs@users.noreply.github.com Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363)
2023-07-12 31859944+LongCatIsLooong@users.noreply.github.com Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118)
2023-07-12 john@johnmccutchan.com [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609)
2023-07-12 flar@google.com Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606)
2023-07-12 skia-flutter-autoroll@skia.org Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604)
2023-07-12 jonahwilliams@google.com [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 1STsUj0X5Ygp to xBJq6PsO5ebb
  fuchsia/sdk/core/mac-amd64 from 0fvk838jTDNQ to 3C7P0w8ySmtq

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
...
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 13, 2023
)

All current desktop backends will support Impeller after flutter/engine#43388 lands.
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 13, 2023
…lutter#130496)

Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions)

flutter/engine@16e2ab7...b408036

2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from e5ec341bc3ca to 56b68ce6196c (1 revision) (flutter/engine#43633)
2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from f499e91e8cb2 to ade4dae923f3 (1 revision) (flutter/engine#43632)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 811b046c673b to e5ec341bc3ca (1 revision) (flutter/engine#43631)
2023-07-13 ian@hixie.ch Make GOMA state automatic by default (flutter/engine#43584)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from c8da0c657c4e to 811b046c673b (3 revisions) (flutter/engine#43630)
2023-07-13 chinmaygarde@google.com [Impeller] Remove unactionable error logs and use structure chains for instance creation. (flutter/engine#43629)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 7f391ea9164e to c8da0c657c4e (1 revision) (flutter/engine#43628)
2023-07-13 chinmaygarde@google.com [Impeller] Add RAII wrappers for VMA objects. (flutter/engine#43626)
2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 1STsUj0X5YgpiSNEb... to xBJq6PsO5ebblODMe... (flutter/engine#43627)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 6ed93436d57c to 7f391ea9164e (1 revision) (flutter/engine#43625)
2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from 8f8f281ccdc6 to f499e91e8cb2 (3 revisions) (flutter/engine#43623)
2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 0fvk838jTDNQ_l43k... to 3C7P0w8ySmtqpyi3S... (flutter/engine#43622)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from bedc92598644 to 6ed93436d57c (3 revisions) (flutter/engine#43621)
2023-07-13 jonahwilliams@google.com [Impeller] Add support to embedder for Impeller on GL (via Angle on Windows). (flutter/engine#43388)
2023-07-12 jonahwilliams@google.com [Impeller] Allocate buffers out of a pool on the raster thread. (flutter/engine#43564)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617)
2023-07-12 flar@google.com move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614)
2023-07-12 ian@hixie.ch Document (and assert) that channel names can't contains nulls (flutter/engine#43593)
2023-07-12 58529443+srujzs@users.noreply.github.com Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363)
2023-07-12 31859944+LongCatIsLooong@users.noreply.github.com Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118)
2023-07-12 john@johnmccutchan.com [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609)
2023-07-12 flar@google.com Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606)
2023-07-12 skia-flutter-autoroll@skia.org Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604)
2023-07-12 jonahwilliams@google.com [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 1STsUj0X5Ygp to xBJq6PsO5ebb
  fuchsia/sdk/core/mac-amd64 from 0fvk838jTDNQ to 3C7P0w8ySmtq

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
...
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 13, 2023
kjlubick pushed a commit to kjlubick/engine that referenced this pull request Jul 14, 2023
…indows). (flutter#43388)

Actually works now, though there is some issue with the default fbo stencil so I've filled flutter/flutter#130048

Other issues:
* ~~Rendering looks wrong~~
* ~~Resizing window hangs~~
* ~~Reactor isn't set up correctly and all blit passes are currently failing.~~
* ~~Needs to handle falling back to sample count of 1 like we do on Android~~.
harryterkelsen pushed a commit to harryterkelsen/engine that referenced this pull request Jul 20, 2023
…indows). (flutter#43388)

Actually works now, though there is some issue with the default fbo stencil so I've filled flutter/flutter#130048

Other issues:
* ~~Rendering looks wrong~~
* ~~Resizing window hangs~~
* ~~Reactor isn't set up correctly and all blit passes are currently failing.~~
* ~~Needs to handle falling back to sample count of 1 like we do on Android~~.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller platform-windows
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants