macOS Flicker Fix

[j-mie]

This moves the macOS implementation from a two buffer approach to a pool of buffers, this is unfortunately required because the macOS compositor seems to keep a reference to the IOSurface presented via CALayer's contents after the contents have changed - resulting in the compositor using a buffer that is partially rendered, this appears to become more noticeable past 60 FPS.

Fixes runelite/runelite#19369

[j-mie]

@abextm this is just an initial draft for you to take a look at, I've been running it for a few hours without any issues, the pool grew up to 5 IOSurface's.

I intend to drop the IOSurfacePool debug stats logging commit before merge :)

Not sure wherever or not this change warrants another file so I don't need to shove the whole pool impl in rlawt_mac.m ?

I also wasn't 100% sure if the dynamically growing pool was worth it or wherever or not a fixed size might be good enough, my very first test was with 3 buffers and when setting the back buffers to magenta I did still see some magenta - but it's possible that 3-4 buffers is good enough and removes the 2D flickering issue?

[Adam-]

This seems to think there are some times when pool->front is !IOSurfaceIsInUse but also it shouldn't be picked as the next buffer and it instead picks a different buffer, why is that?

Also I don't quite get the reasoning to pick appropriately-sized buffers in preference to non-appropriately sized buffers, what is that trying to do? It seems like it would maybe save a buffer recreation sometimes but those happen almost never, so it seems like it is not worth trying to do this?

I think a bunch of the code could be cleaned up by naming the pool entry structure and just passing that around instead of passing around the entire pool + index.

I'm probably fine with keeping the stats (or a subset of them) if it just #ifdefed out or so.

[j-mie]

This seems to think there are some times when pool->front is !IOSurfaceIsInUse but also it shouldn't be picked as the next buffer and it instead picks a different buffer, why is that?

We were double buffering before so I presumed to keep that, although I also wasn't sure if IOSurfaceIsInUse could return false & then later return true for the same CALayer's contents if something like the mouse moved over the area - I just tested this and it seemed safe to remove at first but I still had very occasional flickering at 250 FPS~ - which I guess makes sense since there's no way to guarantee the compositor won't start a new read of the surface between checking IOSurfaceIsInUse and finishing the next frame's render as it's still the current CALayer.contents.

Also I don't quite get the reasoning to pick appropriately-sized buffers in preference to non-appropriately sized buffers, what is that trying to do? It seems like it would maybe save a buffer recreation sometimes but those happen almost never, so it seems like it is not worth trying to do this?

Agreed - I've removed that

I think a bunch of the code could be cleaned up by naming the pool entry structure and just passing that around instead of passing around the entire pool + index.

Much cleaner, thanks for the suggestion.

I'm probably fine with keeping the stats (or a subset of them) if it just #ifdefed out or so.

Great, have done

---

I'll mark this as ready for review, I've been running it for about a day now without issue but given my lack of experience in rendering I'd be grateful for someone to check my findings

[Adam-]

poolNextBack allocating a pool and passing it to poolEntryMatchesSize() is relying on AWTContext being calloc'd to have surface == 0. But despite this you are still zero initing most of the rest of the structure for no reason. This is confusing.

[j-mie]

Sorry I have no idea why that closed...

poolNextBack allocating a pool and passing it to poolEntryMatchesSize() is relying on AWTContext being calloc'd to have surface == 0. But despite this you are still zero initing most of the rest of the structure for no reason. This is confusing.

Agreed, should be should be cleaner now - thanks!