3rd party bindings and CMSampleBuffer usage in trampolines

[dalexsoto]

Description

We have a report that using CMSampleBuffer in delegates on 3rd party bindings throws the following error:

error CS1729: 'CMSampleBuffer' does not contain a constructor that takes 2 arguments

Binding code:

[BaseType (typeof (NSObject))]
interface Foo {
	[Export ("enumerateSampleBuffers:")]
	void Enumerate (Action<CMSampleBuffer, NSError> handler);
}

This happens because we are special casing CMSampleBuffer in our generator and we are manually calling the .ctor(IntPtr, bool):

https://github.com/xamarin/xamarin-macios/blob/507d37eef68f13aec954121a2d441702bf87864e/src/generator.cs#L1617-L1624

But the .ctor(IntPtr, bool) of CMSampleBuffer is internal so it cannot be used by 3rd party bindings this way:

https://github.com/xamarin/xamarin-macios/blob/507d37eef68f13aec954121a2d441702bf87864e/src/CoreMedia/CMSampleBuffer.cs#L67-L74

Posible Solutions

Solution 1

Make the generator use Runtime.GetINativeObject<T> (IntPtr, bool) but this adds some overhead other than simply using the .ctor(IntPtr, bool) and most scenarios where CMSampleBuffer is used require speed which leads into the next solution.

Solution 2

Turn the .ctor(IntPtr, bool) of CMSampleBuffer visibility public but this can lead to potential incorrect usage.

Solution 3

Make the generator aware of the 3rd party binding context and special case the the usage of Runtime.GetINativeObject<T> (IntPtr, bool) only for 3rd party bindings and keep the current behavior for our bindings.

Workaround

Bind the CMSampleBuffer as IntPtr:

[BaseType (typeof (NSObject))]
interface Foo {
	[Export ("enumerateSampleBuffers:")]
	void Enumerate (Action<IntPtr, NSError> handler);
}

Then in actual code usage code do:

foo.Enumerate ((sb, error) => {
	using (var sampleBuffer = Runtime.GetINativeObject<CMSampleBuffer> (sb, false)) {
		// Do something...
	}
});

[dalexsoto]

Myself I lean towards Solution 3.

[spouliot]

I'd say solution #2 with Advanced (or maybe Never) for blocking IntelliSense from suggesting it when people write code. Maybe an [Advice] too ?

[dalexsoto]

Oh I did not think about Advice + Never or Advanced yeah I think Option 2 makes more sense now.

[rolfbjarne]

How does the generator handle all the other INativeObject subclasses? I think we should use the same pattern everywhere.

[dalexsoto]

@rolfbjarne we have some special cases but others generally are called with GetINativeObject<T> (IntPtr, false). I guess we have CMSampleBuffer special cased because performance(?), the special casing in the generator predates all of us 😄, goes beyond the initial import of maccore.

Other than the reflection performance impact to get the .ctor(IntPtr, bool) I do not see why it can't be called using GetINativeObject<T> (IntPtr, false).

[rolfbjarne]

I had a look, and this is annoying :/

The default case in the generator is to call the IntPtr constructor directly: https://github.com/xamarin/xamarin-macios/blob/de8276445642e8589382f9c2346036b1bd8f74f8/src/generator.cs#L1634-L1638

However, for CMSampleBuffer we started calling the owns overload here: https://github.com/xamarin/maccore/commit/1ac93b5ebceb4a8526ab7ddce9496b1c732e078f, so that the CMSampleBuffer is automatically retained.

Turns out that fix is wrong, the correct fix is to make the CMSampleBuffer IntPtr constructor follow our INativeObject pattern like this:

internal CMSampleBuffer (IntPtr handle)
	: this (handle, false)
{
}

and now the CMSampleBuffer special case in the generator can be removed (we'd also have to audit everybody that calls this constructor to make sure we don't start leaking CMSampleBuffers).

Now this doesn't solve the problem with third-party bindings, but for that case I think Solution 3 is the best, because I don't like making the IntPtr constructors public (there's a reason we made the IntPtr constructors protected for NSObject subclasses).

[spouliot]

I don't mind #3. Ideally we would avoid the following 3 situations - but it disqualifies all options ;-)

• minimize the difference between our and 3rd party bindings - make install does not work unless make all is already complete #1 increase that problem;
• measure performance impact, since it was a problem for CMSampleBuffer, which is what [linker] Fix debugging of watchOS apps when link all is used. Fixes #40641 #3 is a bit of an unknown;
• avoid public .ctor with IntPtr (they are too often misused) - which is a problem with [appkit] Add NSImage lazy initialization #2

[dalexsoto]

Since we now have #5693 I'll go for Option 3 for now.