Skip to content

[.NET/CoreGraphics] Use [UnmanagedCallersOnly] instead of [MonoPInvokeCallback] Partial Fix for #10470 #15906

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Sep 12, 2022
Merged

[.NET/CoreGraphics] Use [UnmanagedCallersOnly] instead of [MonoPInvokeCallback] Partial Fix for #10470 #15906

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
847c1e7
update to UnmanagedCallersOnly where possible
stephen-hawley Sep 8, 2022
3f8d0ae
Merge branch 'main' into pinvoke-coregraphics
mandel-macaque Sep 8, 2022
1909fa2
merge up unsafe struct
stephen-hawley Sep 9, 2022
26a001b
Merge branch 'pinvoke-coregraphics' of https://github.com/stephen-haw…
stephen-hawley Sep 9, 2022
587e14a
dead delegate declaration
stephen-hawley Sep 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/CoreGraphics/CGDataProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,29 +144,48 @@ public CGDataProvider (NSData data)
{
}

#if NET
[DllImport (Constants.CoreGraphicsLibrary)]
extern static unsafe IntPtr CGDataProviderCreateWithData (/* void* */ IntPtr info, /* const void* */ IntPtr data, /* size_t */ nint size, /* CGDataProviderReleaseDataCallback */ delegate* unmanaged<IntPtr, IntPtr, nint, void> releaseData);
#else
[DllImport (Constants.CoreGraphicsLibrary)]
extern static IntPtr CGDataProviderCreateWithData (/* void* */ IntPtr info, /* const void* */ IntPtr data, /* size_t */ nint size, /* CGDataProviderReleaseDataCallback */ CGDataProviderReleaseDataCallback releaseData);
#endif

delegate void CGDataProviderReleaseDataCallback (IntPtr info, IntPtr data, nint size);
Copy link
Member

@rolfbjarne rolfbjarne Sep 9, 2022

Choose a reason for hiding this comment

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

Is this delegate still needed in NET?

#if !NET
static CGDataProviderReleaseDataCallback release_gchandle_callback = ReleaseGCHandle;
static CGDataProviderReleaseDataCallback release_buffer_callback = ReleaseBuffer;
static CGDataProviderReleaseDataCallback release_func_callback = ReleaseFunc;
#endif

#if NET
[UnmanagedCallersOnly]
#else
[MonoPInvokeCallback (typeof (CGDataProviderReleaseDataCallback))]
#endif
private static void ReleaseGCHandle (IntPtr info, IntPtr data, nint size)
{
var gch = GCHandle.FromIntPtr (info);
gch.Free ();
}

#if NET
[UnmanagedCallersOnly]
#else
[MonoPInvokeCallback (typeof (CGDataProviderReleaseDataCallback))]
#endif
private static void ReleaseBuffer (IntPtr info, IntPtr data, nint size)
{
if (data != IntPtr.Zero)
Marshal.FreeHGlobal (data);
}

#if NET
[UnmanagedCallersOnly]
#else
[MonoPInvokeCallback (typeof (CGDataProviderReleaseDataCallback))]
#endif
private static void ReleaseFunc (IntPtr info, IntPtr data, nint size)
{
var gch = GCHandle.FromIntPtr (info);
Expand All @@ -188,7 +207,13 @@ static IntPtr Create (IntPtr memoryBlock, int size, bool ownBuffer)
{
if (!ownBuffer)
memoryBlock = Runtime.CloneMemory (memoryBlock, size);
#if NET
unsafe {
return CGDataProviderCreateWithData (IntPtr.Zero, memoryBlock, size, &ReleaseBuffer);
}
#else
return CGDataProviderCreateWithData (IntPtr.Zero, memoryBlock, size, release_buffer_callback);
#endif
}

public CGDataProvider (IntPtr memoryBlock, int size, bool ownBuffer)
Expand All @@ -202,7 +227,13 @@ static IntPtr Create (IntPtr memoryBlock, int size, Action<IntPtr> releaseMemory
ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (releaseMemoryBlockCallback));

var gch = GCHandle.Alloc (releaseMemoryBlockCallback);
#if NET
unsafe {
return CGDataProviderCreateWithData (GCHandle.ToIntPtr (gch), memoryBlock, size, &ReleaseFunc);
}
#else
return CGDataProviderCreateWithData (GCHandle.ToIntPtr (gch), memoryBlock, size, release_func_callback);
#endif
}

public CGDataProvider (IntPtr memoryBlock, int size, Action<IntPtr> releaseMemoryBlockCallback)
Expand All @@ -221,7 +252,13 @@ static IntPtr Create (byte [] buffer, int offset, int count)

var gch = GCHandle.Alloc (buffer, GCHandleType.Pinned); // This requires a pinned GCHandle, because unsafe code is scoped to the current block, and the address of the byte array will be used after this function returns.
var ptr = gch.AddrOfPinnedObject () + offset;
#if NET
unsafe {
return CGDataProviderCreateWithData (GCHandle.ToIntPtr (gch), ptr, count, &ReleaseGCHandle);
}
#else
return CGDataProviderCreateWithData (GCHandle.ToIntPtr (gch), ptr, count, release_gchandle_callback);
#endif
}

public CGDataProvider (byte [] buffer, int offset, int count)
Expand Down
25 changes: 23 additions & 2 deletions src/CoreGraphics/CGFunction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,13 @@ public class CGFunction : NativeObject {
unsafe static CGFunction ()
{
cbacks.version = 0;
#if NET
cbacks.evaluate = &EvaluateCallback;
cbacks.release = &ReleaseCallback;
#else
cbacks.evaluate = new CGFunctionEvaluateCallback (EvaluateCallback);
cbacks.release = new CGFunctionReleaseCallback (ReleaseCallback);
#endif
}

#if !NET
Expand Down Expand Up @@ -103,13 +108,22 @@ protected internal override void Release ()
// Apple's documentation says 'float', the header files say 'CGFloat'
unsafe delegate void CGFunctionEvaluateCallback (/* void* */ IntPtr info, /* CGFloat* */ nfloat *data, /* CGFloat* */ nfloat *outData);
delegate void CGFunctionReleaseCallback (IntPtr info);


#if NET
[StructLayout (LayoutKind.Sequential)]
unsafe struct CGFunctionCallbacks {
public /* unsigned int */ uint version;
public delegate* unmanaged<IntPtr, nfloat*, nfloat*, void> evaluate;
public delegate* unmanaged<IntPtr, void> release;
}
#else
[StructLayout (LayoutKind.Sequential)]
struct CGFunctionCallbacks {
public /* unsigned int */ uint version;
public CGFunctionEvaluateCallback? evaluate;
public CGFunctionReleaseCallback? release;
}
#endif
Copy link
Contributor

@mandel-macaque mandel-macaque Sep 8, 2022

Choose a reason for hiding this comment

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

Since the struct is the same and we might add more data in it, why not doing:

Suggested change
#if NET
[StructLayout (LayoutKind.Sequential)]
unsafe struct CGFunctionCallbacks {
public /* unsigned int */ uint version;
public delegate* unmanaged<IntPtr, nfloat*, nfloat*, void> evaluate;
public delegate* unmanaged<IntPtr, void> release;
}
#else
[StructLayout (LayoutKind.Sequential)]
struct CGFunctionCallbacks {
public /* unsigned int */ uint version;
public CGFunctionEvaluateCallback? evaluate;
public CGFunctionReleaseCallback? release;
}
#endif
[StructLayout (LayoutKind.Sequential)]
struct CGFunctionCallbacks {
public /* unsigned int */ uint version;
#if NET
public delegate* unmanaged<IntPtr, nfloat*, nfloat*, void> evaluate;
public delegate* unmanaged<IntPtr, void> release;
#else
public CGFunctionEvaluateCallback? evaluate;
public CGFunctionReleaseCallback? release;
#endif
}

That way if we need to add more stuff, we won't have to write it twice.

Copy link
Contributor Author

@stephen-hawley stephen-hawley Sep 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I considered that, but since the struct needs to be marked unsafe, this would start to get into the ridiculous range:

    [StructLayout (LayoutKind.Sequential)]
#if NET
    unsafe struct CGFunctionCallbacks {
#else
    struct CGFunctionCallbacks {
#endif
        public /* unsigned int */ uint version;
#if NET
        public delegate* unmanaged<IntPtr, nfloat*, nfloat*, void> evaluate;
        public delegate* unmanaged<IntPtr, void> release;
#else
        public CGFunctionEvaluateCallback? evaluate;
        public CGFunctionReleaseCallback? release;
#endif
    }

At this point every element is different except for the attribute and the uint.

Copy link
Member

@rolfbjarne rolfbjarne Sep 9, 2022

Choose a reason for hiding this comment

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

It doesn't work if you mark just the fields as unsafe?

    public unsafe delegate* unmanaged<IntPtr, nfloat*, nfloat*, void> evaluate;
    public unsafe delegate* unmanaged<IntPtr, void> release;


[DllImport (Constants.CoreGraphicsLibrary)]
extern static IntPtr CGFunctionCreate (/* void* */ IntPtr data, /* size_t */ nint domainDimension, /* CGFloat* */ nfloat []? domain, nint rangeDimension, /* CGFloat* */ nfloat []? range, ref CGFunctionCallbacks callbacks);
Expand All @@ -136,17 +150,24 @@ public unsafe CGFunction (nfloat []? domain, nfloat []? range, CGFunctionEvaluat
var handle = CGFunctionCreate (GCHandle.ToIntPtr (gch), domain is not null ? domain.Length / 2 : 0, domain, range is not null ? range.Length / 2 : 0, range, ref cbacks);
InitializeHandle (handle);
}

#if NET
Copy link
Contributor

@mandel-macaque mandel-macaque Sep 8, 2022

Choose a reason for hiding this comment

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

The [MonoPInvokeCallback (typeof (CGFunctionReleaseCallback))] was just being added in those platforms that are not macOS, now with the #If NET the UnmanagedCallersOnly is added in all platforms including macOS, is this intended?

Copy link
Contributor Author

@stephen-hawley stephen-hawley Sep 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. It's intentional. My understanding is that macOS doesn't need the [MonoPInvoke] attribute, but other platforms do. For .NET 7, everybody gets [UnmanagedCallersOnly]

[UnmanagedCallersOnly]
#else
#if !MONOMAC
[MonoPInvokeCallback (typeof (CGFunctionReleaseCallback))]
#endif
#endif
static void ReleaseCallback (IntPtr info)
{
GCHandle.FromIntPtr (info).Free ();
}

#if NET
[UnmanagedCallersOnly]
#else
Copy link
Contributor

@mandel-macaque mandel-macaque Sep 8, 2022

Choose a reason for hiding this comment

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

Same question as the previous one.

#if !MONOMAC
[MonoPInvokeCallback (typeof (CGFunctionEvaluateCallback))]
#endif
#endif
unsafe static void EvaluateCallback (IntPtr info, nfloat *input, nfloat *output)
{
Expand Down
15 changes: 14 additions & 1 deletion src/CoreGraphics/CGPath.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,9 +478,12 @@ public unsafe bool ContainsPoint (CGPoint point, bool eoFill)
public delegate void ApplierFunction (CGPathElement element);

delegate void CGPathApplierFunction (/* void* */ IntPtr info, /* const CGPathElement* */ IntPtr element);

#if NET
[UnmanagedCallersOnly]
#else
#if !MONOMAC
Copy link
Contributor

@mandel-macaque mandel-macaque Sep 8, 2022

Choose a reason for hiding this comment

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

Same.

[MonoPInvokeCallback (typeof (CGPathApplierFunction))]
#endif
#endif
static void ApplierCallback (IntPtr info, IntPtr element_ptr)
{
Expand Down Expand Up @@ -520,12 +523,22 @@ static void ApplierCallback (IntPtr info, IntPtr element_ptr)


[DllImport (Constants.CoreGraphicsLibrary)]
#if NET
extern unsafe static void CGPathApply (/* CGPathRef */ IntPtr path, /* void* */ IntPtr info, delegate* unmanaged<IntPtr, IntPtr, void> function);
#else
extern static void CGPathApply (/* CGPathRef */ IntPtr path, /* void* */ IntPtr info, CGPathApplierFunction function);
#endif

public void Apply (ApplierFunction func)
{
GCHandle gch = GCHandle.Alloc (func);
#if NET
unsafe {
CGPathApply (Handle, GCHandle.ToIntPtr (gch), &ApplierCallback);
}
#else
CGPathApply (Handle, GCHandle.ToIntPtr (gch), ApplierCallback);
#endif
gch.Free ();
}

Expand Down
31 changes: 31 additions & 0 deletions src/CoreGraphics/CGPattern.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,21 @@ public enum CGPatternTiling {
delegate void DrawPatternCallback (/* void* */ IntPtr info, /* CGContextRef */ IntPtr c);
delegate void ReleaseInfoCallback (/* void* */ IntPtr info);

#if NET
[StructLayout (LayoutKind.Sequential)]
unsafe struct CGPatternCallbacks {
internal /* unsigned int */ uint version;
internal delegate* unmanaged<IntPtr, IntPtr, void> draw;
internal delegate* unmanaged<IntPtr, void> release;
}
#else
[StructLayout (LayoutKind.Sequential)]
struct CGPatternCallbacks {
internal /* unsigned int */ uint version;
internal DrawPatternCallback draw;
internal ReleaseInfoCallback release;
}
#endif
Copy link
Contributor

@mandel-macaque mandel-macaque Sep 8, 2022

Choose a reason for hiding this comment

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

Same comment about the struct.

Suggested change
#if NET
[StructLayout (LayoutKind.Sequential)]
unsafe struct CGPatternCallbacks {
internal /* unsigned int */ uint version;
internal delegate* unmanaged<IntPtr, IntPtr, void> draw;
internal delegate* unmanaged<IntPtr, void> release;
}
#else
[StructLayout (LayoutKind.Sequential)]
struct CGPatternCallbacks {
internal /* unsigned int */ uint version;
internal DrawPatternCallback draw;
internal ReleaseInfoCallback release;
}
#endif
[StructLayout (LayoutKind.Sequential)]
struct CGPatternCallbacks {
internal /* unsigned int */ uint version;
#if NET
internal delegate* unmanaged<IntPtr, IntPtr, void> draw;
internal delegate* unmanaged<IntPtr, void> release;
#else
internal DrawPatternCallback draw;
internal ReleaseInfoCallback release;
}
#endif



#if NET
Expand Down Expand Up @@ -95,11 +104,25 @@ extern static IntPtr CGPatternCreate (/* void* */ IntPtr info, CGRect bounds, CG
/* CGFloat */ nfloat xStep, /* CGFloat */ nfloat yStep, CGPatternTiling tiling, [MarshalAs (UnmanagedType.I1)] bool isColored,
/* const CGPatternCallbacks* */ ref CGPatternCallbacks callbacks);

#if NET
static CGPatternCallbacks callbacks;

static CGPattern () {
unsafe {
callbacks = new CGPatternCallbacks () {
version = 0,
draw = &DrawCallback,
release = &ReleaseCallback,
};
}
}
#else
static CGPatternCallbacks callbacks = new CGPatternCallbacks () {
version = 0,
draw = DrawCallback,
release = ReleaseCallback,
};
#endif
GCHandle gch;

public CGPattern (CGRect bounds, CGAffineTransform matrix, nfloat xStep, nfloat yStep, CGPatternTiling tiling, bool isColored, DrawPattern drawPattern)
Expand All @@ -111,8 +134,12 @@ public CGPattern (CGRect bounds, CGAffineTransform matrix, nfloat xStep, nfloat
Handle = CGPatternCreate (GCHandle.ToIntPtr (gch), bounds, matrix, xStep, yStep, tiling, isColored, ref callbacks);
}

#if NET
[UnmanagedCallersOnly]
#else
Copy link
Contributor

@mandel-macaque mandel-macaque Sep 8, 2022

Choose a reason for hiding this comment

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

Same, it adds macOS when it did not in the past.

#if !MONOMAC
[MonoPInvokeCallback (typeof (DrawPatternCallback))]
#endif
#endif
static void DrawCallback (IntPtr voidptr, IntPtr cgcontextptr)
{
Expand All @@ -123,8 +150,12 @@ static void DrawCallback (IntPtr voidptr, IntPtr cgcontextptr)
}
}

#if NET
[UnmanagedCallersOnly]
#else
#if !MONOMAC
Copy link
Contributor

@mandel-macaque mandel-macaque Sep 8, 2022

Choose a reason for hiding this comment

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

Same.

[MonoPInvokeCallback (typeof (ReleaseInfoCallback))]
#endif
#endif
static void ReleaseCallback (IntPtr voidptr)
{
Expand Down