Hotfix metal for ios 15 and lower

[taublast]

Description of Change

It has been reported that on lower iOS current metal itialiazation settings suggested by Apple are resulting in a crash, this hotfix solves this for iOS 15 and lower.

Bugs Fixed

• Fixes

[BUG] SKMetalView issues on older hardware (iOS 15)

API Changes

None.

Behavioral Changes

Should act as before .commit https://github.com/mono/SkiaSharp/commit/c2a1fde1bd3986d907f92445047b8fbc130c122b for devices with iOS 15 and lower.

Required skia PR

None.

PR Checklist

• [ ] Has tests (if omitted, state reason in description)
• [ ] Rebased on top of main at time of PR
• [ ] Merged related skia PRs
• [ ] Changes adhere to coding standard
• [ ] Updated documentation

[mattleibow]

/azp run

[azure-pipelines[bot]]

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

[mattleibow]

/rebase

[mattleibow]

/azp run

[azure-pipelines[bot]]

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

[mattleibow]

/Users/runner/work/1/s/source/SkiaSharp.Views.Maui/SkiaSharp.Views.Maui.Core/Platform/Android/SKTouchHandler.cs(120,37): warning CA1416: This call site is reachable on: 'Android' 21.0 and later. 'MotionEventButtonState.StylusSecondary' is only supported on: 'android' 23.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/Users/runner/work/1/s/source/SkiaSharp.Views.Maui/SkiaSharp.Views.Maui.Core/SkiaSharp.Views.Maui.Core.csproj::TargetFramework=net8.0-android34.0] /Users/runner/work/1/s/source/SkiaSharp.Views/SkiaSharp.Views/Platform/Apple/SKMetalView.cs(96,8): error CS0103: The name 'UIKit' does not exist in the current context [/Users/runner/work/1/s/source/SkiaSharp.Views/SkiaSharp.Views/SkiaSharp.Views.csproj::TargetFramework=net8.0-macos14.0] 98 Warning(s) 1 Error(s)

[mattleibow]

/azp run

[azure-pipelines[bot]]

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

[taublast]

We had SampleCount 1 before fixing metal for simulator, so in context of info from Jeremy0 (https://github.com/mono/SkiaSharp/pull/3156#issuecomment-2846719662) it could be safer to return to the state of SampleCount for phisical devices we had previously.
I did not notice any difference on real device (not simulator) with SampleCount 1 or 2, so 1 is fine with me personally to revert to the way it was.

[mattleibow]

@taublast are you saying to revert entirely, or detect if physican and then use 1, else use this version check?

[mattleibow]

If we got some testing to do here, I can merge the revert PR and we can iterate on this?

[taublast]

This pr "reverts for old phisical devices". The "revert pr" would make ios simulator unusable for skglview animated scenarios for people that use default skiasharp handlers.

[taublast]

I'm using this code several times a week on simulator/real mac/real iphone, if this info could help..

[jeremy-visionaid]

@taublast IIRC, the sample you provided for the performance measurements used a redundant intermediate blit (and also used a sample count of 1 on the intermediate surface). Have you fixed those yet? It would be good to rule them out. Might be helpful to get the actual render timings as opposed to the FPS too.

In my use case I did not notice any significant performance difference in the simulator with the defaults (i.e. 1), and I still think that the docs do not recommend this. If I'm reading it correctly, the docs and the sample just avoid using a sample count of 2 - they don't say not to use 1.

I'm happy to go and triple check my measurements, and I have an older MacBook I can test on too, but prob be a bit later in the day as I've got other more urgent bits I need to get to first.

[taublast]

@taublast IIRC, the sample you provided for the performance measurements used a redundant intermediate blit (and also used a sample count of 1 on the intermediate surface). Have you fixed those yet? It would be good to rule them out. Might be helpful to get the actual render timings as opposed to the FPS too.

@jeremy-visionaid The sample i provided in the first place was the current skiasharp handler, and my custom handlers appeared after the fix was already working, nothing to do with it. To answer direct about the handler, yes it is now better "timed" with more fluid rendering than what we had.

In my use case I did not notice any significant performance difference in the simulator with the defaults (i.e. 1), and I still think that the docs do not recommend this. If I'm reading it correctly, the docs and the sample just avoid using a sample count of 2 - they don't say not to use 1.

Exact quoted examples from apple were followed both for simulator and real device. Simulator settings worked perfectly, on real devices we had an issue with iOS lower than 16 (thanks apple).
This PR sets SampleCount to 1 for real devices as it was already the case previously, default is 1 and our code is not changing this.
The "1" variable is there for an explicit set, to be visible and customizable.
And I think we already had this interesting conversation between me "simulator went from unusable to awesome" and you "did not notice any significant performance difference", will not continue on this.

I'm happy to go and triple check my measurements, and I have an older MacBook I can test on too, but prob be a bit later in the day as I've got other more urgent bits I need to get to first.

Please suite yourself, meanwhile I would not choose to revert the good to avoid the bad, instead of fixing the bad to achieve a better state.

[jeremy-visionaid]

Hey, sorry, I didn't mean for my post to seem so terse - just got a lot of stuff on. I was only pointing out that there were other factors in the sample you provided me that might have been a contributing factor to the performance issues you experienced - I wasn't aware of the timeline of your own handlers to know whether that was important or not. I can only say that original PR didn't seem to affect the performance for my particular usage on my particular hardware - and I completely accept there might be something with either of those factors that make this PR the better one.

With the sample count set to 1 for real devices, I don't see that it's a particularly big deal either way about which PR gets accepted (just technical correctness and simulator will render slightly different results). I'd rather the attention go to higher priority stuff like #3258 since that's blocking for stuff like Avalonia than foresake the great for the good.

[jeremy-visionaid]

OK, I've got some time now to look at this. I recall now that my initial investigation was based off https://github.com/taublast/Maui.Game.SpaceShooter and https://github.com/taublast/DrawnUi in order to eliminate my use case being a factor.

I still had some of the modifications locally, so I've simplified them and pused those to a fork: https://github.com/jeremy-visionaid/Maui.Game.SpaceShooter/tree/samplecount-test https://github.com/jeremy-visionaid/DrawnUi/tree/samplecount-test The timings are obviously very quick and dirty - I have more sophisticated timings in my own app, but I hope they would be sufficient to show if there were a significant performance issue related to the sample count in the simulator.

At the time, there was a bug in dotnet and it wasn't possible to compile a release build for the simulator without setting UseInterpreter: https://github.com/dotnet/macios/issues/22205 This has now been resolved, so I've disabled UseInterpreter again.

Here are the timings with dotnet sdk 9.0.301:

M2 Pro - iPad A16 18.5 Simulator - Debug SampleCount = 1: ~5.5 ms SampleCount = 4: ~5.5 ms

M2 Pro - iPad A16 18.5 Simulator - Release SampleCount = 1: ~5.5 ms SampleCount = 4: ~5.5 ms

I also added a 5 ms sleep as a control, which increased the render timings to over 10 ms as expected. The shooter app maintained 60 fps for all the tests.

So, the test method is pretty blunt, please let me know if you disagree with it, but it is at least something that everyone here could look at and spin up as a test pretty quickly to inform a decision here.

[jeremy-visionaid]

Urgh. I forgot to hit reload when I switched to the release build 🤦 It's too late here for me to repeat the timings now since release builds are so very slow.

In the meantime I tested my x86 machine...

MacBook Pro 2019 - iPad A16 18.5 Simulator - Debug SampleCount = 1: ~15 ms SampleCount = 4: ~16 ms

So it seems sample count of 1 might be measurably faster there, which is probably what you'd expect, but one off timings are obviously pretty sketchy - it would really need an average to be more certain.

[mattleibow]

Sorry that I have not been on top of this. I am hoping to merge some fixes and get ready to release a new SkiaSharp.

Just want to confirm that this PR is beter than the crash :) And is there anything that would be worse than the revert PR? Effectively, is the PR beter or OK as before for common cases? Since these are public properties, devs can edit the properties after the ctor runs, so if we can't get 100% perfect, we should get better for 99.999% (or even 80%) of the cases.

[jeremy-visionaid]

I think there's a difference in opinion there :) It's definitely better than the crash, but from my perspective:

1. I was not able to repro the op's claim that MSAA improved simulator perfomance on arm64
2. The op's repro actually showed a perf degradation on x86
3. MSAA increases GPU memory usage
4. It possibly creates a difference in output between a real device and the simulator
5. The documentation doesn't endorse using MSAA for the simulator (i.e. the original change was made under a false pretense)
6. There were approach/implementation issues with the op's original repro that may have been contributing factors in the observations

So, yeah, IMHO, this is better than it crashing, but worse than the revert (though not catastrophically). I have provided evidence here to show that the ops claims appear to be invalid. That's open to rebutal, but, I would go with the revert pending demonstration/repro that there was ever an actual problem, since it should have evidence for it being an improvement before being accepted.

[jeremy-visionaid]

FWIW, I think that these being public properties also support applying the revert. If a user has a legitimate need to change the values then they can. But the original defaults are a better match for the upstream, so probably more in line with what people expect. Currently they're unexpectedly different without a shown need.