Use dispatch_async for -[NSOpenGLContext update]

[Learath2]

Description

I just moved the patch I provided on bugzilla to here, the issue explains why I fixed it like this.
Whether setWindow also should be using an async dispatch is up to discussion but I never got any feedback on that.

Existing Issue(s)

#3680

[slouken]

@icculus, is this safe to do?

[icculus]

I think it mostly is, but it also serves as an ongoing cautionary tale to stop using OpenGL on Apple devices, as it continues to get more fragile.

There are places where we will now make two async calls in a row with this patch, though, so don't merge this; I'm going to clean those instances up to make a single async call, just in case.

[icculus]

Okay, looking at this closer: setWindow can't be async, even if this would deadlock the app, because this is the core of SDL_GL_MakeCurrent(), and it would be disastrous to return from that function before the setWindow call is definitely complete, as future GL calls from the app would race on what context they apply to, or maybe not apply to anything if the main thread is completely blocked. In such a case, the app will need to redesign to deal with this case (either make the context current earlier in the process, or at least call SDL_PumpEvents() while waiting on the main thread).

The docs for -[NSOpenGLContext update] say this, though:

A multithreaded application must synchronize all threads that access the same drawable object and call update for each thread’s context serially.

...and I'm wondering if the original problem wasn't that it has to be on the main thread, but that we had a case where someone was calling it from two threads at once, and we just needed a mutex instead of a dispatch.

But for now: this patch is fine as-is. But we might want to revisit this later and try to reproduce the original crash that led to the dispatch call being added.

[psi29a]

This is great news, resolved our OpenMW crashy-ness. When can we expect a new release? 😄

[icculus]

When can we expect a new release? 😄

https://github.com/libsdl-org/SDL/milestone/1

[Learath2]

...and I'm wondering if the original problem wasn't that it has to be on the main thread, but that we had a case where someone was calling it from two threads at once, and we just needed a mutex instead of a dispatch.

For posterity, I think I was the one that introduced the dispatch with a performSelectorOnMain in the main thread (#3600) and I can confirm it has to happen on the main thread atleast on AppKit shipped with 10.15. There is literally a call to pthread_main_np guarding it.

[icculus]

Heads up everyone, we just changed the default behavior back to dispatch_sync, because async upsets modern macOS, so if you still need async behavior, you should call this once, somewhere near SDL_Init().

SDL_SetHint("SDL_MAC_OPENGL_ASYNC_DISPATCH", "1");

And it will continue to do async updates as before. This is safe to do even if you don't update your SDL to 2.24.0, where this change is being made.

ALSO NOTE that the async behavior can cause crashes in the latest macOS, so you might want to consider reworking your code to deal with this, as there is nothing we can do to mitigate that problem on your behalf if you run into that (but it's possible that you won't, it's a bit of a black box).