Re-work the managed-native types by mattleibow · Pull Request #900 · mono/SkiaSharp

mattleibow · 2019-07-01T05:36:14Z

Description of Change

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 [FEATURE] Add a SkiaApi and SKAbstractManagedStream native abstraction #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

Bugs Fixed

Related to issue [FEATURE] Add a SkiaApi and SKAbstractManagedStream native abstraction #876

Provide links to issues here. Ensure that a GitHub issue was created for your feature or bug fix before sending PR.

API Changes
None visible.

Behavioral Changes

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.

PR Checklist

Has tests (if omitted, state reason in description)
Rebased on top of master at time of PR
Changes adhere to coding standard
Updated documentation

- pass a context around - clean up the API - don't use dictionaries - don't use GetFunctionPointerForDelegate

- added several debug-only hooks to track objects and exceptions - added a field to keep the document stream alive - drawable picture is not referenced as it is a plain pointer - creating typefaces always copies the managed stream as multiple reads are needed - automatically dispose objects that already exist in the instances dictionary when the same handle is loaded - the object may already be deleted on the native side - a native crash will happen if the object is unref-ed later

mattleibow · 2019-07-07T01:50:37Z

 			using (var image = SKImage.FromPixels(pixmap, onRelease, "RELEASING!"))
 			{
 				Assert.False(image.IsTextureBacked);
-				using (var raster = image.ToRasterImage())


This is no longer supported as it is not valid C#/.NET code. You have to actually dispose of the same SKImage instance twice (or more depending).If the instance is the same, you cannot dispose it more time.

mattleibow · 2019-07-07T02:52:46Z

 		public const float DefaultRasterDpi = 72.0f;

+		// keep the stream alive for as long as the document exists
+		private SKWStream underlyingStream;


The streams were in danger of being disposed while the document was in progress. To fix this, we keep a reference to it to make sure the GC doesn't pick it up early. This is different to the internal stream ownership where the internal streams are DISPOSED after usage. This will allow the stream to stay open and alive.

This reverts commit 4d8836a.

mattleibow · 2019-07-30T01:20:55Z

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.

mattleibow added 5 commits July 1, 2019 07:34

Re-work the managed-native types

fe87634

- pass a context around - clean up the API - don't use dictionaries - don't use GetFunctionPointerForDelegate

Update native

34c1d6d

This is not supposed to be here

26e3e9c

Re-working the entire native-managed interop and reference counting

2ef57ae

Fixed several memory leaks and access violations

1259c31

mattleibow mentioned this pull request Jul 5, 2019

Improve managed interop mono/skia#61

Merged

mattleibow commented Jul 7, 2019

View reviewed changes

mattleibow added 2 commits July 7, 2019 04:02

Clean up some test code

2e38a78

More cleanup

e1d48c3

mattleibow mentioned this pull request Jul 7, 2019

UWP store version has crashes from libSkiaSharp.dll #905

Closed

mattleibow commented Jul 7, 2019

View reviewed changes

mattleibow added 10 commits July 7, 2019 16:47

Fix .NET Core tests

d1167ed

Fix for mac

e06d139

move the delegates into the separate files

4d8836a

Add the missing slash

17ae9b4

Revert "move the delegates into the separate files"

281d5c1

This reverts commit 4d8836a.

Try more parallel tests

f1707f9

Merge branch 'master' into dev/improve-managed

a75dd99

Using shared proxy delegates

077648d

Both mono and non-Windows platforms have different limitations

d6c91c2

Use weak user data

075f633

mattleibow mentioned this pull request Jul 26, 2019

Fix bug in SKObject.Dispose #489

Merged

mattleibow added 7 commits July 27, 2019 00:04

Merge branch 'master' into dev/improve-managed

9bead86

This canvas won't ever own the handle

419d0a4

Make sure to ALWAYS copy test outputs

c472565

This image might be collected too soon

8cbfd17

Trying this...

04ce9aa

Ok, so there are a few checks...

a2d4c98

Try 3

80e4167

mattleibow added 3 commits July 29, 2019 20:49

Trying to make sure the results get out

ad22bdf

Update to the newly-merged skia submodule

73e2c64

Remove the invalid usages of sk_nvrefcnt_t

22bbe8e

mattleibow merged commit d5aa34a into master Jul 30, 2019

mattleibow deleted the dev/improve-managed branch July 30, 2019 01:26

This was referenced Jul 30, 2019

Some of the colorspaces created are just references #922

Merged

Getting the message "Not replacing existing, living, managed instance with new object." #835

Closed

mattleibow added this to the v1.68.1 milestone Nov 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Re-work the managed-native types#900

Re-work the managed-native types#900
mattleibow merged 28 commits into
masterfrom
dev/improve-managed

mattleibow commented Jul 1, 2019 •

edited

Loading

Uh oh!

mattleibow Jul 7, 2019

Uh oh!

mattleibow Jul 7, 2019 •

edited

Loading

Uh oh!

mattleibow commented Jul 30, 2019 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

mattleibow commented Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattleibow Jul 7, 2019

Choose a reason for hiding this comment

Uh oh!

mattleibow Jul 7, 2019 • 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!

mattleibow commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mattleibow commented Jul 1, 2019 •

edited

Loading

mattleibow Jul 7, 2019 •

edited

Loading

mattleibow commented Jul 30, 2019 •

edited

Loading