[.NET/AudioUnit] Use [UnmanagedCallersOnly] instead of [MonoPInvokeCallback] Partial Fix for #10470 by stephen-hawley · Pull Request #15808 · dotnet/macios

stephen-hawley · 2022-08-30T14:32:05Z

OK - this works, but you're not going to like it.
Why? Well, it comes down to this struct here:

#if NET
        [StructLayout (LayoutKind.Sequential)]
        public unsafe struct AURenderCallbackStruct
        {
                public delegate* unmanaged<IntPtr, AudioRenderActionFlags*, AudioTimeStamp*, uint, uint, IntPtr, AudioUnitStatus> Proc;
//              public delegate* unmanaged<IntPtr, int*, AudioTimeStamp*, uint, uint, IntPtr, int> Proc;
                public IntPtr ProcRefCon;
        }
#else
        [StructLayout (LayoutKind.Sequential)]
        struct AURenderCallbackStruct
        {
                public Delegate Proc;
                public IntPtr ProcRefCon;
        }
#endif

What you're seeing in the NET section is what I think the struct should be for the field Proc which represents the correct signature for the callback function. However, if I use this, I get the following errors:

AudioUnit/AudioUnit.cs(142,100): error CS0246: The type or namespace name 'AudioUnitStatus' could not be found (are you missing a using directive or an assembly reference?)
AudioUnit/AudioUnit.cs(142,38): error CS0246: The type or namespace name 'AudioRenderActionFlags' could not be found (are you missing a using directive or an assembly reference?)

Which is particularly interesting for 3 reasons:
1 - both types are in scope - the actual declaration of a function that meets the contract works with no issue.
2 - making the types fully qualified makes no difference
3 - I AUGraph.cs, which I did first, I initially made a local version of the callback which was declared as an inner type but otherwise declared identically and it worked (!!)

So yes, this is a greasy, greasy, hack.
Why does it work? The types we're smudging are really just 32 bit integers, so no harm no foul. Well, a little foul.

…llback] Partial Fix for dotnet#10470

rolfbjarne

Welcome to the joy of porting broken code...

rolfbjarne · 2022-08-31T16:07:09Z

+					foreach (RenderDelegate renderer in renderers) {
+						var tempActionFlags = *_ioActionFlags;
+						var tempTimeStamp = *_inTimeStamp;
+						renderer (tempActionFlags, tempTimeStamp, _inBusNumber, _inNumberFrames, buffers);


The temporary values here are unnecessary, because due to an incorrect binding, the ioActionFlags and inTimeStamp values aren't updated by the renderer callback:

Suggested change

renderer (tempActionFlags, tempTimeStamp, _inBusNumber, _inNumberFrames, buffers);

renderer (*_ioActionFlags, *_inTimeStamp, _inBusNumber, _inNumberFrames, buffers);

These arguments should be ref arguments in the RenderDelegate: https://github.com/xamarin/xamarin-macios/blob/4c36652927ceb4098472a913b0e579000294d4c2/src/AudioUnit/AudioUnit.cs#L132

But that would be a breaking change, so we can't do that (we'd have to add (and correct) API instead of changing the existing one).

In any case, that's completely out of scope for this PR. I'd just make the .NET code just as broken as the non-.NET code, and maybe we'll get around to fixing it properly one day.

Huh. I looked at this through a peephole and just saw ref args and figured that I should honor the ref-ness.

rolfbjarne · 2022-08-31T16:07:40Z

+#if NET
+				AudioUnitRenderActionFlags tempActionFlags = *actionFlags;
+				var tempTimeStamp = *timeStamp;
+				var returnValue = callback (tempActionFlags, tempTimeStamp, busNumber, numberFrames, buffers);


Just one left now!

vs-mobiletools-engineering-service2 · 2022-09-01T10:08:18Z

🔥 [CI Build] Test results 🔥

Test results

❌ Tests failed on VSTS: simulator tests

0 tests crashed, 206 tests failed, 4126 tests passed.

Failures

❌ bcl tests

3 tests failed, 225 tests passed.

Details

trimmode link/Mac Catalyst [dotnet]/Debug [dotnet]: Failed (Test run failed.
Tests run: 129 Passed: 121 Inconclusive: 0 Failed: 1 Ignored: 7)
trimmode link/Mac Catalyst [dotnet]/Release [dotnet]: Failed (Test run failed.
Tests run: 129 Passed: 120 Inconclusive: 0 Failed: 1 Ignored: 8)
introspection/watchOS 32-bits - simulator/Debug (watchOS 6.0): Crashed Known issue: HE0038)

	renderer (tempActionFlags, tempTimeStamp, _inBusNumber, _inNumberFrames, buffers);
	renderer (_ioActionFlags, _inTimeStamp, _inBusNumber, _inNumberFrames, buffers);

Conversation

stephen-hawley commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

rolfbjarne left a comment • 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!

rolfbjarne Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

stephen-hawley Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

rolfbjarne Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

rolfbjarne Sep 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vs-mobiletools-engineering-service2 commented Sep 1, 2022

🔥 [CI Build] Test results 🔥

Test results

Failures

❌ bcl tests

❌ cecil tests

❌ dotnettests tests

❌ fsharp tests

❌ framework tests

❌ generator tests

❌ interdependent_binding_projects tests

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vs-mobiletools-engineering-service2 commented Sep 2, 2022

🔥 [CI Build] Test results 🔥

Test results

Failures

❌ bcl tests

❌ cecil tests

❌ dotnettests tests

❌ fsharp tests

❌ framework tests

❌ generator tests

❌ interdependent_binding_projects tests

❌ install_source tests

❌ introspection tests

❌ linker tests

Uh oh!

rolfbjarne commented Sep 2, 2022

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rolfbjarne commented Sep 5, 2022

Uh oh!

azure-pipelines Bot commented Sep 5, 2022

Uh oh!

vs-mobiletools-engineering-service2 commented Sep 5, 2022

Uh oh!

vs-mobiletools-engineering-service2 commented Sep 5, 2022

stephen-hawley commented Aug 30, 2022 •

edited

Loading

rolfbjarne left a comment •

edited

Loading