[Impeller] Expose a single-header C API to Impellers Display-List layer. #55238

chinmaygarde · 2024-09-16T19:37:19Z

This (impeller.h) is a versioned, single-header, dependency-free, C API that exposes Display-Lists in almost their entirety.

The anticipated users of this API do not expect ABI or API stability guarantees at this time but those can be added between specific versions as the API matures.

Testing this API can be done via a playgrounds harness. One has been setup with rudimentary tests that will be filled out.

A simple C example (example.c) using GLFW with OpenGL ES has been setup. Only OpenGL ES has been exposed at this time but additional backend will be added.

This API is meant to be easy to bind to using automated tools for access in different languages and runtimes. Consequently, the API follows a strict convention for object naming, creating, and reference-counting.

All typedefs and method names have the “Impeller” prefix.
Most objects are reference counted. Methods ending with “New” return a new instance of the object with reference count of 1.
Each object has a “Retain” and “Release” method to modify reference counts.

For instance, with Rust bindgen, the following invocation generates usable bindings:

bindgen impeller.h -o impeller.rs
  --no-layout-tests
  --rustified-enum “Impeller.*”
  --allowlist-item “Impeller.*”

It is expected that wrapped over these generated bindings will be written to make the usage more idiomatic. The C API itself is fairly verbose as well.

This is a versioned, single-header, dependency-free, C API that exposes Display-Lists in almost their entirety. The anticipated users of this API do not expect ABI or API stability guarantees at this time but those can be added between specific versions as the API matures. Testing this API can be done via a playgrounds harness. One has been setup with rudimentary tests that will be filled out. A simple C example using GLFW with OpenGL ES has been setup. Only OpenGL ES has been exposed at this time but additional backend will be added. This API is meant to be easy to bind to using automated tools for access in different languages and runtimes. Consequently, the API follows a strict convention for object naming, creating, and reference-counting. All typedefs and method names have the “Impeller” prefix. Most objects are reference counted. Methods ending with “New” return a new instance of the object with reference count of 1. Each object has a “Retain” and “Release” method to modify reference counts. For instance, with Rust bindgen, the following invocation generates usable bindings: ```sh bindgen impeller.h -o impeller/rs --no-layout-tests --rustified-enum “Impeller.*” --allowlist-item “Impeller.*” ``` It is expected that wrapped over these generated bindings will be written to make the usage more idiomatic. The C API itself is fairly verbose as well.

chinmaygarde · 2024-09-16T19:43:17Z

I'll followup on feedback from the user to see if there is anything to drastically rework. I've also used the new "experimental" canvas dispatcher with no fallback. Working on documentation right now.

impeller/toolkit/interop/texture.h

impeller/toolkit/interop/paint.h

chinmaygarde · 2024-09-16T20:52:21Z

Initial user feedback: Looks good except they will need a way to supply OpenGL textures to use as ImpellerTexture backing stores (they are not expecting these to be the texture_external_oes type textures). That can be a subsequent addition. I'm going to remove the SetContents call and make textures immutable as well.

chinmaygarde · 2024-09-16T20:58:39Z

cc @lyceel

jonahwilliams · 2024-09-16T23:21:58Z

impeller/toolkit/interop/surface.cc

+  display_list->Dispatch(impeller_dispatcher, skia_cull_rect);
+  impeller_dispatcher.FinishRecording();
+  content_context.GetLazyGlyphAtlas()->ResetTextFrames();
+  content_context.GetTransientsBuffer().Reset();


Need to file an issue for this, but since we don't control the compositor we likely can't assume a particular number of Frames in flight, and will need to use a different host buffer solution.

Actually for gles it doesn't really matter since we don't actually support buffet objects, all host buffet data is copied out each frame

Yeah, this was a last minute update after I last pulled where the experimental canvas stuff became default. Will file an issue for this.

This doesn't really have anything to do with experimental canvas, the old path has the same behavior.

impeller/toolkit/interop/impeller.h

jonahwilliams · 2024-09-16T23:25:24Z

impeller/toolkit/interop/impeller.cc

+  ObjectBase::SafeRelease(color_source);
+}
+
+static std::pair<std::vector<Color>, std::vector<Scalar>> ParseColorsAndStops(


It would be preferable if these constructors didn't need to copy all of there color data. Stops at least shoudn't need it since its all floats. Colors, you could make ImpellerColor static castable to DlColor and call it good.

I was doing that but then I ran into the issue of the colors not having a colorspace (fixed now) and the fields being swapped (RGBA vs ARGB). These subtle errors due to mismatched POD layouts freaked me out a bit and decided to copy these out. I believe these are small and callers can build and keep these around if they become expensive. So IMO its safer than reinterpret casts (for now).

zanderso · 2024-09-17T01:27:00Z

Nice!

FYI @jtmcdole

The clang-tidy warning seems pertinent. Does our C library have the replacement suggested?

bdero · 2024-09-17T16:40:25Z

This is awesome.

One step at a time, but what are your current thoughts around text drawing here?

We have a pluggable text backend that is not poked through the DL, but I don't think that's the right abstraction anyway -- we should probably just offer One True Way™️ to draw text.

Should we just provide a C wrapper for SkTextBlob?

jtmcdole · 2024-09-17T19:50:10Z

Nice!

FYI @jtmcdole

The clang-tidy warning seems pertinent. Does our C library have the replacement suggested?

Errors suggests fprintf_s; but I see fprintf used elsewhere in the code.

❌ Failures for clang-tidy on /b/s/w/ir/cache/builder/src/flutter/impeller/toolkit/interop/example.c:
/b/s/w/ir/cache/builder/src/flutter/impeller/toolkit/interop/example.c:12:3: error: Call to function 'fprintf' is insecure as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'fprintf_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling,-warnings-as-errors]
   12 |   fprintf(stderr, "GLFW Error (%d): %s\n", error, description);
      |   ^~~~~~~
/b/s/w/ir/cache/builder/src/flutter/impeller/toolkit/interop/example.c:12:3: note: Call to function 'fprintf' is insecure as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'fprintf_s' in case of C11
   12 |   fprintf(stderr, "GLFW Error (%d): %s\n", error, description);
      |   ^~~~~~~

chinmaygarde · 2024-09-17T20:45:20Z

The clang-tidy warning seems pertinent.

That is a usage in example code which assumes older C standards. Interestingly, the clang-tidy on MacOS doesn't throw the same error suggesting version skew (I'll follow up separately). I suppressed it since that code is meant to be a single file example that can't use our own loggers.

Also, based on feedback from @lyceel, I've gated IMPELLER_NODISCARD behind a check for C23. Rust BindGen defaults to C99 and the expanded [[nodiscard]] keels over on that.

bdero

Took a closer look and I think this is great.

It looks like there will be some follow-up work here but it doesn't necessarily need to be done in this pass.

So LGTM!

chinmaygarde · 2024-09-17T21:11:17Z

One step at a time, but what are your current thoughts around text drawing here?

Discussed this a bit during the weekly. But the plan is to offer a text shaping API (simple "text in a box"). While the user has their own backend that hooks into the typographer, their experience has been that this has been hard to get right (which makes sense as we have spent a lot of effort on ours). So I will work on exposing what we have. But, without "a viable other", it is unclear if having a pluggable shaper offers us any benefits 🤷🏽‍♂️

jonahwilliams

LGTM

…y-List layer. (flutter/engine#55238)

…155371) flutter/engine@4bdcbf3...15e9e31 2024-09-18 [email protected] [Impeller] Expose a single-header C API to Impellers Display-List layer. (flutter/engine#55238) 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://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

chinmaygarde requested review from bdero, flar, jonahwilliams and zanderso September 16, 2024 19:37

github-actions bot added the e: impeller label Sep 16, 2024

jonahwilliams reviewed Sep 16, 2024

View reviewed changes

impeller/toolkit/interop/texture.h Show resolved Hide resolved

jonahwilliams reviewed Sep 16, 2024

View reviewed changes

impeller/toolkit/interop/paint.h Outdated Show resolved Hide resolved

chinmaygarde added 4 commits September 16, 2024 15:54

Licenses.

47258d7

Jonahs comments.

d696cf4

Format.

baa1586

Tidy.

47e03a5

jonahwilliams reviewed Sep 16, 2024

View reviewed changes

impeller/toolkit/interop/impeller.h Show resolved Hide resolved

jonahwilliams reviewed Sep 16, 2024

View reviewed changes

chinmaygarde added 2 commits September 17, 2024 13:41

Grep away nodiscard on older C stds.

cd0be7a

suppress lint.

bd9d228

bdero approved these changes Sep 17, 2024

View reviewed changes

PR

92b5ec8

jonahwilliams approved these changes Sep 18, 2024

View reviewed changes

chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 18, 2024

auto-submit bot merged commit 15e9e31 into flutter:main Sep 18, 2024

engine-flutter-autoroll mentioned this pull request Sep 18, 2024

Roll Flutter Engine from 4bdcbf39c200 to 15e9e3153266 (1 revision) flutter/flutter#155371

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 18, 2024

15e9e3153 [Impeller] Expose a single-header C API to Impellers Displa…

46736a0

…y-List layer. (flutter/engine#55238)

chinmaygarde deleted the libimpeller branch September 28, 2024 17:33

chinmaygarde added the e: libimpeller The standalone Impeller library with a single-header API. label Oct 3, 2024

chinmaygarde self-assigned this Oct 4, 2024

[Impeller] Expose a single-header C API to Impellers Display-List layer. #55238

[Impeller] Expose a single-header C API to Impellers Display-List layer. #55238

Uh oh!

Conversation

chinmaygarde commented Sep 16, 2024

Uh oh!

chinmaygarde commented Sep 16, 2024

Uh oh!

Uh oh!

Uh oh!

chinmaygarde commented Sep 16, 2024

Uh oh!

chinmaygarde commented Sep 16, 2024

Uh oh!

jonahwilliams Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahwilliams Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

zanderso commented Sep 17, 2024

Uh oh!

bdero commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtmcdole commented Sep 17, 2024

Uh oh!

chinmaygarde commented Sep 17, 2024

Uh oh!

bdero left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Sep 17, 2024

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bdero commented Sep 17, 2024 •

edited

Loading