atsushieno/nfluidsynth

Upstreaming changes from our fork?

PJB3005 opened this issue · 9 comments

PJB3005 commented

So you definitely noticed our fork at https://github.com/space-wizards/nfluidsynth.
We cleaned up basically the entire codebase because it was... uhhh... kind of a mess.

At this point it would be most convenient for us to put the thing on NuGet, so I figured it'd be easier to see if we could upstream the changes and use the existing NuGet package (instead of making our own version).

I didn't put much effort into keeping the commits atomic so most of what I did was:

  • Cleaning up the API surface so you won't segfault as easily when running methods on disposed objects.
  • Support .NET Core 3.
  • Removed nested classes in the FFI layer so that I could use a static constructor for NativeLibrary for the above.
  • Fixed most of the FFI layer to correctly match the Fluidsynth binding. There was a ton of [MarshalAs (UnmanagedType.SysInt)] which straight up wasn't correct as far as I could tell? There were also some cases of methods having incorrect argument/return type definitions.
  • Removed a lot of the marshalling being done by P/Invoke, so no more byte[] in delegates. This is both for using stuff like Span<T> (yay performance) and fixing various double frees and such.
  • Fixed many GC issues with delegates being passed to Fluidsynth.

Any thoughts on getting this upstreamed? Should I just make a PR to upstream the entire thing?

atsushieno commented

Thanks for raising it. I loved the overall improvements on the existing stuff !

I would definitely like to bring in the improvements onto the master tree and publish the updated nuget package. Most of the changes are good, except that we do need the shared project at least for now because it is used by Android support. I'm sure that there is no compelling reason to dare to remove that.

I will be blocked until 28th. and right now I'm in terrible machine trouble to work on any side tasks, so please don't expect much on this until then.

PJB3005 commented

except that we do need the shared project at least for now because it is used by Android support. I'm sure that there is no compelling reason to dare to remove that.

I got rid of the shared project because I had trouble getting it to work sanely with Rider and the new SDK style projects, so I just kinda axed it. I wouldn't have any problem with re-instating them though.

atsushieno commented

I'm getting back here. Can you please create a PR first? It is most likely to happen that I merge them as is (like, no squash) and I deal with the rest for the shared project etc.

  • 👍1
atsushieno commented

Thanks. After my first tryout, these are the required changes to make for mobile support so far:

  • netstandard2.0 has to be added. (Shared project is doable too, but I don't see any particular advantage over netstandard2.)
  • Use of LPUTF8Str needs to be conditional and enabled only for netcore3.0/net472.
  • Add Span/ReadOnlySpan overrides, do not remove existing pointer based ones, and add newer ones only in the newer profiles.

The PR will be merged to master after these tasks done. I can work on that but it mat still take a while (I had postponed various tasks "to be done after 28th." ...).

PJB3005 commented

netstandard2.0 has to be added

See, the annoying problem is that there is no easy and way to do P/Invoke with .NET Standard 2.0. You can't just [DllImport] since that won't work on all platforms, you need something like Mono DllMap or NativeLibrary, neither of which you can rely upon in .NET Standard 2.0

You basically have to manually do dlopen/LoadLibrary with P/Invoke, then invoke the function pointers from dlsym/GetProcAddress. This either requires calli (requires an extra build step or something like Fody) OR you need to use Marshal.GetDelegateForFunctionPointer.

This is why I only added netcoreapp3.0 support, since P/Invoke can be easily handled.

atsushieno commented

You can't just [DllImport] since that won't work on all platforms, you need something like Mono DllMap or NativeLibrary, neither of which you can rely upon in .NET Standard 2.0

This is not true. We don't need anything special like dllmap to use netstandard2.0 based P/Invoke libs. If you need different [DllImport]s then your native library is most likely not appropriately named or implemented differently across platforms. And even if additional dllmap is required, that cannot be an excuse to skip netstandard2.0 and therefore excluding other platforms.

PJB3005 commented

Alright then.

atsushieno commented

I made the changes above, merged into master, and pushed a new package so far.

I still have to make sure that the new package works with the rest of my apps via the nuget package. Once it's verified, the changes are most likely set in stone.

atsushieno commented

Applied all the required changes (brought back array-based API, made Handle property public, etc.) and now it became almost alright. So I'm declaring it's safe to assume that the changes are set in stone and closing this issue. Thanks!