-
Notifications
You must be signed in to change notification settings - Fork 549
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
[FEATURE] Add a SkiaApi and SKAbstractManagedStream native abstraction #876
Comments
@jeromelaban I had a very, very quick think and this looks to be something that may work. Just tested it on Windows and it runs. Will it work for WASM? https://gist.github.com/mattleibow/6f0cd3efd6152fb614aa0fd803867d25 That should be a fully working gist, at least on Windows x64, here is a snippet: public delegate IntPtr sk_surface_new_null(int width, int height);
public static class TheInterop {
public static sk_surface_new_null sk_surface_new_null = ThePInvokeInterop.sk_surface_new_null;
}
public unsafe class ThePInvokeInterop {
[DllImport("libSkiaSharp", CallingConvention = CallingConvention.Cdecl)]
public extern static IntPtr sk_surface_new_null(int width, int height);
}
public unsafe class TheWasmInterop {
public static void Init() {
TheInterop.sk_surface_new_null = CreateProxy;
}
public static IntPtr CreateProxy(int width, int height) {
var p = new sk_surface_new_null_params();
p.width = width;
p.height = height;
return sk_surface_new_null(ref p);
}
} |
One important thing to note is that we initialize all the delegates, and then WASM can just override the specific ones that need adjustments. Is this useful? Will WASM override all of the methods and this is now just extra work? |
@mattleibow most of the native functions in wasm don't change, so this could definitely work. I wondering about the performance impact of using an indirect call through a delegate, vs. a virtual invocation through an interface. I don't know what LLVM does in those cases, if there's some magic optimization that helps there :) The case of the delegate registration for the |
Is it likely to happen that we will get a full-featured Skia running via WASM? In my opinion, this is a special environment that needs a special build of Skia like CanvasKit. Cripleing other platforms just for one platform wouldn't be ideal. A user of SkiaSharp would expect to be able to use all features of SkiaSharp if there was a WASM build of it. |
@Gillibald this is already working, I've built a specific version of Skia from CanvasKit. This is actually more like a Skia DLL than CanvasKit, as I've stripped out everything JavaScript related. The discussion here is about providing a minimally intrusive way to override the P/Invoke layer, so it fits the needs of WebAssembly and mono-wasm. The outcome of the discussion could very well be that the cost is too high, and that mono-wasm needs to evolve more for this to be viable. |
Here's what it could look like, with your suggestion: unoplatform@a904780 The only particularity is the addition of I have not added the Would that make sense ? |
The delegate based approach can totally be done behind some conditional compilation magic. This way, only netstandard and/or net45 is impacted. (I have updated the gist to demo this) Interfaces will be a bit heavy and will require that ALL the methods be implemented. |
@jeromelaban we can always replace the _2 and |
And, yes. @jeromelaban what you have looks like what I am thinking. What we can do is ask some folks on the runtime team for some input on the performance and optimizations for this approach. You know of anyone we can pull in to this discussion since you have been working closely with them? |
Excellent. Without tagging them explicitly, Zoltan or Bernhard may know more more about this, but in all cases, because those are not generic delegates, they will likely perform very well. |
@jeromelaban will something like this be possible in WASM? https://gist.github.com/mattleibow/7f21dd8fdcd176f035ecdbb47af8f223 @Gillibald and I discovered that we can just create a delegate and pass the I was hoping to migrate the rest of SkiaSharp to this sort of pattern as not only do we avoid a dictionary which helps concurrency, but also we get a performance improvement. We have already moved most of the types to this, but we still have the streams to do. Let me know if this is better or worse. |
Changes: - Added `GCHandleProxy` to debug builds - this is used to track all `GCHandle` `Alloc` and `Free` calls to ensure that all allocations are freed. - added some unit tests to make sure this is actually enforced - as a result, several object are now freed correctly - Added `ISKReferenceCounted` and `ISKNonVirtualReferenceCounted` interfaces to represent the reference counting types used in the native library - this helps with automatically de-referencing objects - `SKAbstractManagedStream`, `SKAbstractManagedWStream` and `SKDrawable` have been re-written to use better delegates - instead of passing each of the delegates as parameters, they are now a struct that is passed as a single object - better for extensions (which there shouldn't be) and only a single static field on the type - removed the usage of `Marshal.GetFunctionPointerForDelegate`, which should help out with WASM (see #876) - the objects now only keep weak references, meaning that they can now be garbage collected - instead of trying to resolve the instances with a dictionary, a delegate is used and passed as "user context" - Moved some of the repetitive logic from the types into the base `SKObject` and `SKNativeObject` - some logic is automatically executed if the concrete type is `ISKReferenceCounted` or `ISKNonVirtualReferenceCounted` - with the more centralized logic and stricter patterns, better tests can be written to make sure all memory is freed correctly and timely - `SKData`, `SKFontManager` and `SKTypeface` now correctly prevent disposal of the "static" instances - `SKPaint` now references the `Shader`, `MaskFilter`, `ColorFilter`, `ImageFilter`, `Typeface` and `PathEffect` properties - this prevents accidental collection, or non-collection when the object goes out of scope - the `SKPath` iterators (`Iterator` and `RawIterator`) and op builder (`OpBuilder`) now correctly own and dispose their native objects - `SKRegion` objects are now disposed on the native side - `SKTypeface` construction from a `SKManagedStream` (via both `SKTypeface` and `SKFontManager`) now copy the contents of the .NET `Stream` into a native memory - typeface construction requires multiple seeks (previously, the stream was copied only if it was non-seekable) - it also requires "duplicating" the stream, which is not supported on .NET streams - duplicates or forks of a stream means that each of the streams need to be read concurrently from different locations - .NET streams can only have a single position - Updated the NuGets used for the tests - using the `Xunit.AssemblyFixture` and `Xunit.SkippableFact` NuGets instead of using the code directly - removed the `Xunit.Categories` NuGet as it was preventing tests from running This PR has a big set of changes that may be breaking due to bug fixes: - The `SKAbstractManagedStream`, `SKAbstractManagedWStream` and `SKDrawable` no longer prevent the GC from collecting them. This means that if code no longer references them, they will be disposed. - As far as I can tell, this should not be a problem for the streams as they are never kept around - they are just used for reading and writing and typically only need to live for as long as a single method, and then need to be disposed by the caller. The `SKTypeface` and `SKDocument` do keep it around for a bit, but then they also take ownership of the stream and keep a hard reference to the streams themselves. They will dispose the streams when they are disposed. - `SKDrawable` is never kept around and is entirely a user-controlled object. If it goes out of scope, skia doesn't have a reference anyway. - The `SKFontManager` and `SKTypeface` no longer use the managed streams (`SKManagedStream` or `Stream`) directly - they make a copy. - This is simply because skia streams can do things that are not possible for .NET - they can be read concurrently from different positions. If a `SKFileStream` or `SKMemoryStream` are passed, then the streams are not copied. - Further optimizations can be made in the case of a `MemoryStream` or `byte[]` to not actually copy but use GC pinning to get a handle to the managed data and work with pointers. But this can be done later so that this PR can be merged and tested.
Closing this as I think all the new changes and code fixes have created a place where WASM does not need this anymore. |
Is your feature request related to a problem? Please describe.
For the implementation of https://github.com/nventive/Uno.SkiaSharp, some internal changes have to be made to enable WebAssembly support. All of the changes need to be made specifically at the P/Invoke (parameter shuffling, types adjustments, custom delegates, etc...) while the rest of the code remains untouched.
This requires the creation of a wasm specific version of the SkiaSharp library, and therefore requires the creation of wasm-specific dependencies, such as SkiaSharp.Extended packages.
Describe the solution you'd like
SkiaSharp should provide a way to change the native backend. e.g. a
SkiaNativeBackend.Current
which provides anISkiaNativeBackend
implementation.This will enable the support for WebAssembly, while minimally impacting the structure of SkiaSharp, until the situation of the packaging of WebAssembly support settles down (NuGet/Home#8186).
The text was updated successfully, but these errors were encountered: