Closed Scony closed 9 months ago
magomez
commented
9 months ago @Scony I've pushed 9f13ddc7498c35c1c1629401ab853109826a4485 that should fix this. We need to ensure that there's a current glContext in order to get the GLES version that we're using, and use fences if they are available. Can you verify that the problem is fixed and close this if it is? Thanks!
Scony
commented
9 months ago Ok, that makes sense. Yes, I'll check in few hours.
Scony
commented
9 months ago I confirm that the crash is no longer occuring with https://github.com/WebPlatformForEmbedded/WPEWebKit/commit/9f13ddc7498c35c1c1629401ab853109826a4485
varumugam123
commented
9 months ago Hi @magomez I have a query related to that, could you help clarify?
I see that the shared context is made current from ImageBufferCairoGLSurfaceBackend::swapBuffersIfNeeded() only when the second texture is not created. Post texture[1] creation, swapBuffersIfNeeded() uses whatever context is set already (could be null as seen in this crash case or some other context). Is this intended?
Because on all other cases where context is required like create() / ~ImageBufferCairoGLSurfaceBackend() / cairoDevice() the shared context is used.
magomez
commented
9 months ago Hi @magomez I have a query related to that, could you help clarify?
I see that the shared context is made current from ImageBufferCairoGLSurfaceBackend::swapBuffersIfNeeded() only when the second texture is not created. Post texture[1] creation, swapBuffersIfNeeded() uses whatever context is set already (could be null as seen in this crash case or some other context). Is this intended?
Because on all other cases where context is required like create() / ~ImageBufferCairoGLSurfaceBackend() / cairoDevice() the shared context is used.
When the cairo gl surface is created, cairo will store the current context and use it whenever it performs any rendering operation on the surface. That's why when there are cairo operations, there's no need to set a context current. I don't get which weird corner case is this bug hitting, but there should always be a current context after the cairo operations in swapBuffersIfNeeded.
asurdej-comcast
commented
9 months ago Looking at the traces it suggests that current context was destroyed by webgl destruction that happended somewhere in between and should not impact our case really as those are/should be separate contexts.
But ImageBufferCairoGLSurfaceBackend::swapBuffersIfNeeded() doesn't call makeContextCurrent() so it don't change ThreadSpecific
Another thing is eglContext set by eglMakeCurrent(). Cairo indeed sets the context for every operation but it seems to be reset after exiting from cairo function in some cases, not sure if it is applicable for our use-cases. Like cairo_gl_flush() sources (cairo-1.16.0):
static inline cairo_status_t
__cairo_surface_flush (cairo_surface_t *surface, unsigned flags)
{
cairo_status_t status = CAIRO_STATUS_SUCCESS;
if (surface->backend->flush)
status = surface->backend->flush (surface, flags);
return status;
}static cairo_status_t
_cairo_gl_surface_flush (void *abstract_surface, unsigned flags)
{
...
status = _cairo_gl_context_acquire (surface->base.device, &ctx);
...
return _cairo_gl_context_release (ctx, status);
}_cairo_gl_context_acquire -> device->backend->lock (device); -> cairo-gl-device::_gl_lock() -> ctx->acquire (ctx);
That ends up here I believe:
static void
_egl_acquire (void *abstract_ctx)
{
cairo_egl_context_t *ctx = abstract_ctx;
EGLSurface current_surface = _egl_get_current_surface (ctx);
_egl_query_current_state (ctx);
if (!_context_acquisition_changed_egl_state (ctx, current_surface))
return;
eglMakeCurrent (ctx->display,
current_surface, current_surface, ctx->context);
}
static void
_egl_release (void *abstract_ctx)
{
cairo_egl_context_t *ctx = abstract_ctx;
if (!ctx->base.thread_aware ||
!_context_acquisition_changed_egl_state (ctx,
_egl_get_current_surface (ctx))) {
return;
}
eglMakeCurrent (ctx->display,
EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT);
}So at the end it may restore egl context to EGL_NO_CONTEXT in some cases and glFenceSync is context specific and may not behave as correclty.
magomez
commented
9 months ago Yeah, I was absent minded yesterday. Cairo resets the appropriate context but just at the EGL level, not the one stored in GLContext::current(), and that's why inside addFenceSyncIfNeeded() there may not be a current context (in this case it was removed by the destruction of the prior WebGL context). Making the sharingContext as current always inside swapBuffersIfNeeded() would have fixed this as well.
asurdej-comcast
commented
9 months ago Right, in fact I don't really care about GLContext::current() as it is used to get version only, until not crashing then it's fine. I'm mainly wondering what EGL context is set at the time of crating glFenceSync as it can be EGL_NO_CONTEXT or sharingContext (I need to confirm what is the result of "if" cond in _egl_release) and if it makes any difference for glFenceSync behavior (like if glFenceSync waits for commands from current context only or so).
All in all this is some corner-case probably so should not impact us much
There is a regression causing a following crash in the https://sky.play.works/service/apps/puppy_rescue/index.html game (after few seconds - few minutes of actually playing):
The crash originates from the fact that
GLContext::current()is null. It's being set to null via such flow:I haven't analyzed this isse further, but the regression looks to be introduced in https://github.com/WebPlatformForEmbedded/WPEWebKit/commit/2deb4161ca87e2b4cb77a7133b76243f947e1b5f
The above was reproduced on arm32 platform using WPE WebKit at https://github.com/WebPlatformForEmbedded/WPEWebKit/commit/51171f833a61a458777b7303462b4da9cf5152df