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

FFI Design Proposal #1254

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

jwinarske
Copy link
Contributor

Design Proposal for enabling FFI with Rust and Dart (Flutter).

@louwers
Copy link
Collaborator

louwers commented Jun 17, 2023

So basically a C interface to MapLibre Native? Which Rust and Dart then use? Sounds reasonable to me.

Should there be ABI stability?

@birkskyum
Copy link
Member

birkskyum commented Jun 17, 2023

@nyurik - related to #1057

@jwinarske
Copy link
Contributor Author

Yes a C interface, and ABI stable. Both Rust and Dart have FFI generation tools that work directly with the C header. Rust tool chains from the past couple of years will work. Same for Dart/Flutter.

For Dart/Flutter support it would enable removing platform channel support (slow) in https://github.com/maplibre/flutter-maplibre-gl and move to FFI (fast), as well as enabling Linux support.

I finished the basic framework today using API pattern found in the maplibre flutter repo:
https://github.com/meta-flutter/maplibre-native/tree/jw/ffi

To build FFI enabled library one just adds build flag - -DMLN_WITH_FFI=ON

Generates libmbgl_ffi.so

My near term goal (4 months) is to enable Automotive Grade Linux (AGL) with Flutter OSS Navigation support.

@louwers louwers requested a review from nyurik June 17, 2023 19:53
Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

Thank you for sharing more information on this. That sounds awesome.

Yes a C interface, and ABI stable.

I think that may be an important detail to include in the design proposal. ABI stability just means we have to be careful when making changes to the FFI header, right?

What would be the best way to distribute this? Should we make versioned releases of the .so (for various platforms)? Or should we just make versioned releases of the source code and let people compile for whatever platform they need?

I am happy to merge this as it. Though maybe some more details on what the C API would look like could be added. I will have @nyurik take a look as well since he is interested in making Rust bindings for MapLibre Native.

@jwinarske
Copy link
Contributor Author

Additional information added to the design doc, which should help.

Some thoughts on release artifacts:

  • Versioning - Should track a common MBGL release version. Currently MBGL doesn't implement the VERSION field in the top level CMake project. Ideally it should, then the libraries can use this to set the version.
  • Artifact release matrix - Downstream can manage specific build combos. There would need to be some unit test cases added and have CI run to detect regression. This should be enough.

I would consider the "common interface" stable. The runtime APIs were directly modeled after the MapLibre Flutter app. We would need to iterate a few times to see if the listing I have for the runtime API meets all use cases. The goal would be to take a path that minimizes changes to app behavior. It's easy to add to API, difficult to remove.

Signed-off-by: Joel Winarske <[email protected]>
@nyurik
Copy link
Member

nyurik commented Jun 20, 2023

Thanks, I think this is a reasonable way forward. I was a bit surprised that it would be a separate target rather than just a simple standalone usage exampe - Rust can work directly with C++ too, especially if LLVM can compile maplibre-native. We can build Rust wrappers to access MapLibre C++ structs without any marshalling.

See https://cxx.rs/ for an example

@jwinarske
Copy link
Contributor Author

@nyurik Hi there. I'm familiar with cxx.rs. Last I looked it could not do quite a few things, and it was largely in flux with development stalled. Has that improved, and can we now decode a complex Mapbox Value directly in Rust? If cxx.rs has matured, then I can simply implement Dart proxy in Rust using the Membrane Crate, and not implement a FFI layer.

@nyurik
Copy link
Member

nyurik commented Jun 21, 2023

My experience with rust interop is relatively small, and actually I was hoping to gain more of it by working with the native code. Cxx had been very active recently, with the latest release a few days ago. My understanding is that you can access practically anything via cxx, but must ensure that all rust safety assumptions are met before letting it access the data

@jwinarske
Copy link
Contributor Author

If cxx.rs works for all use cases, I would prefer native Rust to FFI maintenance. I have implemented Rust app using FFI shim. It's tedious, but works well.

@nyurik
Copy link
Member

nyurik commented Jun 21, 2023

I wish I could have an answer to that. I would also prefer that approach if possible

@jwinarske
Copy link
Contributor Author

I'll play around with it later this week, and report back. Fingers crossed it's now usable

@louwers
Copy link
Collaborator

louwers commented Jun 21, 2023

How would be ensure that we keep the ABI stable?

CXX has official Bazel support but no official CMake support. So might be worth trying that out, since MapLibre Native has Bazel support as well now: https://cxx.rs/build/bazel.html

@jwinarske
Copy link
Contributor Author

@louwers Let's see how far I get with cxx.rs. I think there's a pretty good gap there to quantify. Last time I tried it (~2 years ago), it was a no go due to lack of features in cxx.rs. If it works, then we just consume maplibre headers as-is, and ABI stability remains up to you.

@nyurik
Copy link
Member

nyurik commented Jun 21, 2023

One more thing: I keep seeing mentions of Google's https://github.com/google/autocxx - it looks like a wrapper on top of cxx to simplify things, similar to bindgen

@jwinarske
Copy link
Contributor Author

jwinarske commented Jun 25, 2023

I am looking at autocxx and currently hitting issue with autocxx and setting -std=c++17. I would think this is a pretty standard use case. Hoping I'm wrong but it looks like autocxx is currently only a possibility for -std=c++14.

@jwinarske
Copy link
Contributor Author

jwinarske commented Jun 27, 2023

The author of autocxx got back to me. Using his suggestion I am able to build a basic library wrapping my FFI Context class; stripped it's ~147k. I would like to see it work first before I update the proposal. More a bit later.

The primary downside with autocxx is that all the build flag variants would need to be replicated in build.rs. Not difficult, just a pain. In C/C++ only minor changes required for CMakeLists.txt to build a self contained library. The extra work would make it much easier to use API in Rust. Using C API in Rust is a bit more work across the board.

@birkskyum
Copy link
Member

This is useful for .NET as well - existing non-FFI wrapper here: https://github.com/tdcosta100/MaplibreNative.NET

@JulianBissekkou
Copy link
Collaborator

@jwinarske
Were you able to render something using ffi in dart? If yes, I would love to check it out and understand how this works.

@jwinarske
Copy link
Contributor Author

@JulianBissekkou not yet. I had to put it in the shelf for a short while. In a couple weeks

@birkskyum
Copy link
Member

@jwinarske , are you still investigating this, or is there a roadblock with autocxx?

@josxha
Copy link
Contributor

josxha commented May 17, 2024

The Flutter team currently pushes towards WebAssembly. See the announcements here:

In the process a substential amount has changed in the world of JS interop on Dart/Flutter. Other dependencies are required to be compatible with WASM and the way to wrap JS Objects has changed.

The Flutter maplibre-gl-js wrapper is currently at a point where a larger amount needs to get migrated and rewritten. This could be a good point to think again about ffi.

@jwinarske
Copy link
Contributor Author

@birkskyum still on the shelf. Been mired in feature dev work. I'm thinking august time window.

* draw_frame - renders a single frame
* resize - resizes the map
* run_task - used for housekeeping tasks independent of render path
* load_gl_functions - function pointer callback for resolving GL functions
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would need to be generalized since we have a Metal backend as well and soon a Vulkan backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. This was just for GL at the time

@jwinarske
Copy link
Contributor Author

@jwinarske , are you still investigating this, or is there a roadblock with autocxx?

@josxha I've not looked further at autocxx; I'm not convinced it's doable based on the variant properties. I do have mature C/C++ already that works quite well. If someone wants to spend the time, please do and report back!

@louwers louwers mentioned this pull request Nov 10, 2024
@jwinarske
Copy link
Contributor Author

So coming back to this. I think Rust is out of the question at this time. My thoughts:

  • The source tree is C++
  • API interface is external C used via shared module
  • What I am proposing already has considerable test mileage
  • Rust would just be an unsafe wrapper, and extra work without a clear benefit.

It would make sense to use a Rust based shim in the Rust renderer source tree.

@CommanderStorm
Copy link

I think Rust is out of the question at this time

I am a bit confused. 🤔
Sorry if this seems obvious. It is not for me 😅

Which of these two are you arguing:

  • FFI for rust users to call into maplibre-native should not be supported (It would also be interesting to know what made you change your mind.) or
  • maplibre-native should stick to cpp, but enabling FFI as noted above is fair game.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants