-
Notifications
You must be signed in to change notification settings - Fork 514
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
[.NET/AudioUnit] Use [UnmanagedCallersOnly] instead of [MonoPInvokeCallback] Partial Fix for #10470 #15808
Conversation
…llback] Partial Fix for xamarin#10470
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to the joy of porting broken code...
src/AudioUnit/AUGraph.cs
Outdated
foreach (RenderDelegate renderer in renderers) { | ||
var tempActionFlags = *_ioActionFlags; | ||
var tempTimeStamp = *_inTimeStamp; | ||
renderer (tempActionFlags, tempTimeStamp, _inBusNumber, _inNumberFrames, buffers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The temporary values here are unnecessary, because due to an incorrect binding, the ioActionFlags and inTimeStamp values aren't updated by the renderer callback:
renderer (tempActionFlags, tempTimeStamp, _inBusNumber, _inNumberFrames, buffers); | |
renderer (*_ioActionFlags, *_inTimeStamp, _inBusNumber, _inNumberFrames, buffers); |
These arguments should be ref
arguments in the RenderDelegate
:
xamarin-macios/src/AudioUnit/AudioUnit.cs
Line 132 in 4c36652
public delegate AudioUnitStatus RenderDelegate (AudioUnitRenderActionFlags actionFlags, AudioTimeStamp timeStamp, uint busNumber, uint numberFrames, AudioBuffers data); |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. I looked at this through a peephole and just saw ref args and figured that I should honor the ref-ness.
src/AudioUnit/AUGraph.cs
Outdated
#if NET | ||
AudioUnitRenderActionFlags tempActionFlags = *actionFlags; | ||
var tempTimeStamp = *timeStamp; | ||
var returnValue = callback (tempActionFlags, tempTimeStamp, busNumber, numberFrames, buffers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one left now!
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.
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.
🔥 [CI Build] Test results 🔥Test results❌ Tests failed on VSTS: simulator tests 0 tests crashed, 206 tests failed, 4126 tests passed. Failures❌ bcl tests
Html Report (VSDrops) Download ❌ cecil tests
Html Report (VSDrops) Download ❌ dotnettests tests
Html Report (VSDrops) Download ❌ fsharp tests
Html Report (VSDrops) Download ❌ framework tests
Html Report (VSDrops) Download ❌ generator tests
Html Report (VSDrops) Download ❌ interdependent_binding_projects tests
|
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.
This comment has been minimized.
🔥 [CI Build] Test results 🔥Test results❌ Tests failed on VSTS: simulator tests 0 tests crashed, 37 tests failed, 4295 tests passed. Failures❌ bcl tests
Html Report (VSDrops) Download ❌ cecil tests
Html Report (VSDrops) Download ❌ dotnettests tests
Html Report (VSDrops) Download ❌ fsharp tests
Html Report (VSDrops) Download ❌ framework tests
Html Report (VSDrops) Download ❌ generator tests
Html Report (VSDrops) Download ❌ interdependent_binding_projects tests
Html Report (VSDrops) Download ❌ install_source tests
Html Report (VSDrops) Download ❌ introspection tests
Html Report (VSDrops) Download ❌ linker tests
[Html Report (VSDrops)](https://vsdrop.corp.microsoft.com/file/v1/xamarin-macios/device-tests/20220901.\n\nThe message from CI is too large for the GitHub comments. You can find the full results here. |
I merged main into this PR to see if that fixes a few of the test failures. |
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.
This comment has been minimized.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
🔥 Failed to compare API and create generator diff 🔥 Error: 'make' failed for the hash 7511281. Pipeline on Agent |
❌ [PR Build] Tests on macOS Mac Catalina (10.15) failed ❌Tests on macOS Mac Catalina (10.15) failed for unknown reasons. Pipeline on Agent |
📚 [PR Build] Artifacts 📚Artifacts were not provided. Pipeline on Agent XAMMINI-013.Monterey |
❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌Failed tests are:
Pipeline on Agent |
🔥 [CI Build] Test results 🔥Test results❌ Tests failed on VSTS: simulator tests 0 tests crashed, 1 tests failed, 222 tests passed. Failures❌ bcl tests
Html Report (VSDrops) Download Successes✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
OK - this works, but you're not going to like it.
Why? Well, it comes down to this struct here:
What you're seeing in the
NET
section is what I think the struct should be for the fieldProc
which represents the correct signature for the callback function. However, if I use this, I get the following errors: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.