avoid issues with failure to create gl surface

[gmurray81]

Avoiding issues with failure to create GL surface in UWP

Note, I don't think this is complete, but I want to demonstrate what seems to be necessary to avoid the UWP issues with having multiple SKSwapChainPanels with interleaved creation/destruction.
NOTE for demonstration purposes I removed some cleanup of the GrContext from the panel. This is most likely not valid, but I'm demonstrating what was required to make the behavior correct so that someone with more Skia engine knowledge might use that to find the root cause.

Bugs Fixed

• Fixes [BUG] In UWP, navigating between two pages containing an SKSwapChainPanel raises exception: Failed to create GL surface #1573

API Changes

Behavioral Changes

There is now some ref-counting and locking around obtaining the display within the GL init. This appears to be required because a shared display handle will be returned, and this should not be cleaned up when one of the panels dies if other panels are still alive. I'm not sure if ref counting is the right approach or if the panels should detect that the display is invalid and re-initalize their graphics contexts.

PR Checklist

• Has tests (if omitted, state reason in description) (N/A couln't find any existing tests relating to Views?)
• Rebased on top of main at time of PR
• Changes adhere to coding standard
• Updated documentation (shouldn't require any new documentation, implementation detail)

[gmurray81]

Maybe this could be using some sort of ref counting already happening at the skia level?

[gmurray81]

BTW, the ref counting seems to completely avoid the error with not being able to create the surface. That appears to be driven by the fact that the shared display was terminated while a panel was still running. When it got a composition scale change it would try to reacquire a surface and its display would be cleaned up, I gather.

The remaining issue is that it seems like calling AbandonContext(true) (or Dispose without any context abandonment) results in some sort of resource being cleaned up that another context from the same display needs. I suspect it might be disconnection from the GPU? But I'm uncertain how to detect things have wound up in a bad state to recover.

Calling AbandonContext(false) avoids the issue, but I'm not sure what the difference is between:

abandonContext and releaseResourcesAndAbandonContext

[gmurray81]

I don't think its leaking without that call, because it seems like the memory gets reclaimed, but maybe there's more memory pressure. I didn't grab a graph before the change though.

[gmurray81]

I tried with AbandonContext(true) and the memory pressure didn't seem to perform any differently.

[mattleibow]

Thanks for keeping up with this. Since my GL knowledge is a tad bit low, I see where I went wrong based on what you did here. Looking at the docs:

Multiple calls made to eglGetPlatformDisplay with the same parameters will return the same EGLDisplay handle

https://khronos.org/registry/EGL/sdk/docs/man/html/eglGetPlatformDisplay.xhtml

So we do need to actually track these... Might just be able to put all this in some ConcurrentDictionary so we don't have to deal with locks and whatnot. I'll have a closer look and see if there is a better way.

With regards to abandonContext vs releaseResourcesAndAbandonContext, it looks like we are supposed to AbandonContext(true) the context before we destroy the surface so it can clean up things. If we are running afterwards, then we need to use AbandonContext(false) because we have no context anymore. Looking at my code it seems the OnDestroyingContext is being called before the GlesContext is disposed... But, since the OnUnloaded event may be firing after the real view surface has been cleaned up inside ANGLE so we cannot release anything.

/**
 * Abandons all GPU resources and assumes the underlying backend 3D API context is no longer
 * usable. Call this if you have lost the associated GPU context, and thus internal texture,
 * buffer, etc. references/IDs are now invalid. Calling this ensures that the destructors of the
 * GrContext and any of its created resource objects will not make backend 3D API calls. Content
 * rendered but not previously flushed may be lost. After this function is called all subsequent
 * calls on the GrContext will fail or be no-ops.
 *
 * The typical use case for this function is that the underlying 3D context was lost and further
 * API calls may crash.
 */
void abandonContext() override;

/**
 * This is similar to abandonContext() however the underlying 3D context is not yet lost and
 * the GrContext will cleanup all allocated resources before returning. After returning it will
 * assume that the underlying context may no longer be valid.
 *
 * The typical use case for this function is that the client is going to destroy the 3D context
 * but can't guarantee that GrContext will be destroyed first (perhaps because it may be ref'ed
 * elsewhere by either the client or Skia objects).
 */
virtual void releaseResourcesAndAbandonContext();

[mattleibow]

In fact, looking at the implementation of EAGLContext from WinObjC, it seems the display is a static, one-time thing:

https://github.com/microsoft/WinObjC/blob/develop/Frameworks/OpenGLES/EAGLContext.mm#L44

Since this represents a hardware thing and not windows of physical displays, we can probably do the same thing. Especially since if the smarter people on the WinObjC side used one display, who are we to doubt? Unless in your testing you got more than one display?

”In reality most environments have only one EGLDisplay even when multiple physical screens are supported. [...]"

https://www.khronos.org/registry/implementers_guide.html#negldisplay

[mattleibow]

Overall, good work and I am moving this to the next release so we can have a better UWP experience.

[gmurray81]

Hmm... from what I could tell though, is that if we had two GrContext alive, calling releaseResourcesAndAbandonContext on one of them (AbandonContext(true)) seemed to be doing something that made the other GrContext which was still supposed to be alive no longer valid (we continue trying to render to it, but it seems internally to be dead, and no longer produces content on the surface) but there didn't seem to be a way to detect that we had gotten into this state. So it seemed like some sort of shared resource between the two GrContext was being released and not re-acquired. Perhaps if we were to call abandoned() on the GrContext, we could detect if it had lost some shared resource?

[gmurray81]

For the display, I was just coding defensively since I wasn't sure if the handle would be the same in all scenarios. Sounds like it could be static or a singleton then.

[gmurray81]

Ok @mattleibow I changed display to static and I was able to revert to AbandonContext(true) it seems. I haven't been able to reproduce a scenario yet where an existing GrContext moves into a state where it can no longer render, it seems. So it may have had to do with the display being terminated and restarted?

Note, though, that with this change the display will never get terminated, although they didn't seem to be doing that in WinObjC

[gmurray81]

Hmm... I'll have to double check this later. Find it a bit odd that it worked with true... Maybe the dependency wasn't updating in my sample.

[gmurray81]

yeah, I was right, there is still an issue with AbandonContext(true), seems to be cleaning up something that kills other existing GrContext.

[mattleibow]

What happens if you don't call abandon?

Looking at the comment closer, it says this:

The typical use case for this function is that the client is going to destroy the 3D context but can't guarantee that GrContext will be destroyed first

This is not the case for our views. We are disposing GRContext before we dispose the GPU objects. If you remove that line, do we leak anything? Also, check to see if there is a difference between managed and native memory usage.

[gmurray81]

If I skip calling AbandonContext(true) in OnDestroyingContext then I get the same broken behavior where it seems like it can invalidate other GrContext which are still supposed to be alive. I have to skip both AbandonContext(true) and the Dispose of the context in order to avoid the problem. I'm guessing that may not be valid though, because if left up to the finalizer queue it may not be disposed before the other GPU stuff?

[gmurray81]

Interestingly, all initially seems well with the Abandon dropped, but the Dispose retained, until you change the composition scale by applying a render transform (see my repro code). I'm betting that is causing the interleaved timing again that I described in the bug #1573

[gmurray81]

@mattleibow is there a special trick to getting it to download the externals as of your change to add the IsAbandoned property? I wanted to see if at least that could be used to detect a GrContext that's gone sour and recover it. However, I'm a bit concerned that disposing the sour GrContext, and if the GlInterface perhaps needs to be discarded also, perhaps it will end up souring the other GrContexts that happen to be alive, so perhaps it wont end up being a good solution :)

[mattleibow]

@gmurray81 I was just tweaking the build script to support a better way of downloading the native bits. However, once CI is green again for main (in a few minutes) then you can run:

./bootstrapper.ps1 -t externals-download 

This will pull the latest native artifacts from the last green build of main.

There are also args to control which branch or sha to use:

./bootstrapper.ps1 -t externals-download  [--gitSha=<git-sha-to-pull>]  [--gitBranch=<git-branch-to-pull>]

[mattleibow]

I see what you mean with things looking like they still work when using AbandonContext(true). When the composition changes, then things break... I am going to stick a few breakpoints in the ANGLE code now and see what is happening. Could very well be the cases that destructors are being called in there so the Unloading even is actually far too late to do anything and everything needs to be abandoned - even the cleanup.

I am also building a Debug skia to see if there are any errors/asserts being thrown from that side.

[gmurray81]

Sounds good. I had tried to use IsAbandoned to see if anything was possible at that end, but I don't think the downloaded externals included it yet. Still, as I was saying, I would be somewhat concerned that you may have two swap chain panels warring over destroying the resources if we tried to just recover the resources if destroyed, so may be better to just make sure the cleanup happens in the right sequence.

[mattleibow]

The externals should have it now. There was a problem with CI a few days ago, but that is all fixed now.

But with regards to this issue, if these changes in this PR are working for you, then I think I will merge until there is a better fix. I am not quite sure what is going on yet since none of the other platforms need AbandonContext. As far as I can see, an AbandonContext(false) is required and IsAbandoned doesn't really do anything because it really is a new GRContext each time. It might be an issue with ANGLE. Maybe it is the swap chain setup. But, if this fixes things - and it looks like it does - then we can see if any other issues arise later.

[gmurray81]

Yeah, I'd say it may make sense to merge it until something better is found.