[Impeller] Flutter GPU: Add GpuContext. #44359

bdero · 2023-08-03T22:50:03Z

Move the GpuContext to its new home. I added a flutter_tester test that just verifies an exception is gracefully thrown when Impeller isn't available.

In a later patch, I'll land a way to eagerly supply the Impeller context on the cpp side to enable testing through the playground (as mentioned in flutter/flutter#127712).

chinmaygarde · 2023-08-03T22:55:56Z

lib/gpu/context.cc


-std::string GpuContext::InitializeDefault(Dart_Handle wrapper) {
-  auto dart_state = UIDartState::Current();
+GpuContext::GpuContext(std::shared_ptr<impeller::Context> context)


Nit: Can the name of this class just be Context? Like the impeller:: counterpart?

jonahwilliams · 2023-08-04T01:33:34Z

lib/gpu/context.dart

+GpuContext? _defaultGpuContext;
+
+/// Returns the default graphics context.
+GpuContext getGpuContext() {
+  _defaultGpuContext ??= GpuContext._createDefault();
+  return _defaultGpuContext!;
+}


This is equivalent to:

Suggested change

GpuContext? _defaultGpuContext;

/// Returns the default graphics context.

GpuContext getGpuContext() {

_defaultGpuContext ??= GpuContext._createDefault();

return _defaultGpuContext!;

}

final GpuContext gpuContext = GpuContext._createDefault();

As Dart static fields are already lazily evaluated.

Neat! Done.

jonahwilliams · 2023-08-04T01:35:16Z

lib/gpu/context.dart

+
+part of gpu;
+
+class GpuContextException implements Exception {


Nit: In general I would stick with the default Exception class unless you're adding way more details or you want to extensively document the circumstances under which a developer would catch GpuContextException over just a catch Exception

jonahwilliams · 2023-08-04T16:28:52Z

lib/gpu/context.dart

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+part of gpu;


Since the flutter gpu stuff is being developed as its own package, you don't necessarily need to use part/part of to force it into a single library. You can follow a regular import/export pattern like the framework does and it will just work.

I keep going back and forth on this TBH. I really like the idea of importing one unit as gpu. Not married to it, though.

Maybe we can get the best of both worlds by structuring everything into separately importable files, but have an extra one that exports all of the others. I could do something like that in a follow-up. Thoughts?

You can absolutely do that with regular libraries, see https://github.com/flutter/flutter/blob/master/packages/flutter/lib/widgets.dart

You would create a single flutter_gpu/lib/flutter_gpu.dart library that re-exports the types you want to be public from flutter_gpu/lib/src/**

In fact, should we be fully structuring this like a package in all respects, with a lib directory and a pubspec.yaml that specifies the flutter SDK as a dependency?

Generally yes - Do you depend on the flutter_sdk (package:flutter) or do you depend on dart:ui?

My bad, I meant sky_engine in this case.

Yeah, in that case I would add the following to the pubspec :

sky_engine: sdk: flutter

I reworked it to as an actual package and changed the import scheme.

jonahwilliams · 2023-08-04T18:43:05Z

lib/gpu/context.dart

+/// A handle to a graphics context. Used to create and manage GPU resources.
+///
+/// To obtain the default graphics context, use [getContext].
+class Context extends NativeFieldWrapperClass1 {


I think you should retain GpuContext for this class. Dart itself does not have namespaces, and namespaced imports import "package:flutter_gpu/flutter_gpu" as gpu are uncommon.

Context will commonly collide with other Context types, forcing developers to add import namespaces to disambiguate which will be annoying.

Good point. Done.

lib/gpu/lib/src/context.dart

Co-authored-by: Jonah Williams <[email protected]>

jonahwilliams

LGTM

…132108) flutter/engine@c271092...9c83d90 2023-08-07 [email protected] Roll Dart SDK from 0816d590a220 to f664f4b9c50d (1 revision) (flutter/engine#44462) 2023-08-07 [email protected] [Impeller] Flutter GPU: Add GpuContext. (flutter/engine#44359) 2023-08-07 [email protected] Fix use-after-free crash in glfw embedder (flutter/engine#44358) 2023-08-07 [email protected] Roll Skia from 9fbd7296de9a to d1ada6624536 (1 revision) (flutter/engine#44447) 2023-08-07 [email protected] Revert clang back to 6d667d4b261e81f325756fdfd5bb43b3b3d2451d (flutter/engine#44442) 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

Move the GpuContext to its new home. I added a `flutter_tester` test that just verifies an exception is gracefully thrown when Impeller isn't available. In a later patch, I'll land a way to eagerly supply the Impeller context on the cpp side to enable testing through the playground (as mentioned in flutter/flutter#127712).

[Impeller] Flutter GPU: Add GpuContext.

965d2fd

bdero added the impeller label Aug 3, 2023

bdero self-assigned this Aug 3, 2023

bdero requested review from chinmaygarde and zanderso August 3, 2023 22:53

chinmaygarde reviewed Aug 3, 2023

View reviewed changes

jonahwilliams reviewed Aug 4, 2023

View reviewed changes

chinmaygarde added e: impeller and removed impeller labels Aug 4, 2023

jonahwilliams reviewed Aug 4, 2023

View reviewed changes

github-actions bot removed the e: impeller label Aug 4, 2023

Address comments

5c49f37

bdero force-pushed the bdero/add-gpucontext branch from c40ac50 to 5c49f37 Compare August 4, 2023 18:41

jonahwilliams reviewed Aug 4, 2023

View reviewed changes

Address comments

2c3fb81

chinmaygarde added the e: impeller label Aug 4, 2023

Restructure as an actual dart package

213f4d1

github-actions bot removed the e: impeller label Aug 6, 2023

Licenses

db8072e

bdero requested review from chinmaygarde and jonahwilliams August 7, 2023 17:43

jonahwilliams reviewed Aug 7, 2023

View reviewed changes

lib/gpu/lib/src/context.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Aug 7, 2023

View reviewed changes

lib/gpu/lib/src/context.dart Outdated Show resolved Hide resolved

bdero and others added 2 commits August 7, 2023 11:13

Update lib/gpu/lib/src/context.dart

2500a9a

Co-authored-by: Jonah Williams <[email protected]>

Update lib/gpu/lib/src/context.dart

1f5ca19

Co-authored-by: Jonah Williams <[email protected]>

bdero requested a review from jonahwilliams August 7, 2023 19:06

jonahwilliams approved these changes Aug 7, 2023

View reviewed changes

bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 7, 2023

auto-submit bot merged commit 6ec3566 into flutter:main Aug 7, 2023

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

Roll Flutter Engine from c27109291e22 to 9c83d90b01bd (5 revisions) flutter/flutter#132108

Merged

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

6ec35667c [Impeller] Flutter GPU: Add GpuContext. (flutter/engine#44359)

9dfa79d

reidbaker mentioned this pull request Aug 15, 2023

i82973 scroll mouse wheel support #44724

Merged

14 tasks


		part of gpu;

		class GpuContextException implements Exception {

[Impeller] Flutter GPU: Add GpuContext. #44359

[Impeller] Flutter GPU: Add GpuContext. #44359

Uh oh!

Conversation

bdero commented Aug 3, 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!

bdero Aug 4, 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!

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!

Uh oh!

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bdero Aug 4, 2023 •

edited

Loading