Bug 235946

Created attachment 450512 [details] Patch

Comment on attachment 450512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450512&action=review > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:369 > + m_eglCreateImage = reinterpret_cast<PFNEGLCREATEIMAGEPROC>(eglGetProcAddress("eglCreateImage")); > + m_eglDestroyImage = reinterpret_cast<PFNEGLDESTROYIMAGEPROC>(eglGetProcAddress("eglDestroyImage")); libepoxy does this for you. Also, the extension should be used, EGL 1.5 should not be presumed. > Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLEPipe.cpp:65 > + auto proxyOperation = [this, size, format, stride, fd = m_context.m_compositorTextureBacking->fd()] (TextureMapperPlatformLayerProxy& proxy) mutable { You can't just throw file descriptors into an asynchronous dispatch without guaranteeing they won't be closed by the time the dispatch occurs. > Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLEPipe.cpp:66 > + return proxy.scheduleUpdateOnCompositorThread([this, size, format, stride, fd] () mutable { swapBuffersIfNeeded() is called during layer flush that sets everything up for the next composition update. This scheduleUpdateOnCompositionThread() sets up a dispatch that might be in contention with the wholesale composition update, meaning it can be executed after the composition update that corresponds to this layer flush. pushNextBuffer() should be called from the main/worker thread, i.e. directly from GCGLANGLEPipeSource::swapBuffersIfNeeded(). But that requires a more delicate spawning of EGLImages. > Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:63 > + m_device = gbm_create_device(fd); It's not reasonable to spawn a gbm_device for each GC3D context. > Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:308 > GL_BindTexture(textureTarget, m_compositorTexture); > +#if USE(NICOSIA) > + if (m_compositorTextureBacking && m_compositorTextureBacking->image()) > + GL_EGLImageTargetTexture2DOES(GL_TEXTURE_2D, m_compositorTextureBacking->image()); You're not necessarily matching texture targets here. > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:37 > +#include "gbm.h" Whose gbm.h is this? > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:74 > + auto texmapFormat = (format == GBM_BO_FORMAT_ARGB8888) ? GL_RGBA : GL_RGB; Is this proper? ARGB8888 and XRGB8888 are both 32-bit formats, how does GL_RGB as a 24-bit format come into play here? > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:85 > + glGenTextures(1, &id); Leaked.

Created attachment 450760 [details] Patch

Created attachment 450767 [details] Patch

Created attachment 450768 [details] Patch

Comment on attachment 450768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450768&action=review > Source/WebCore/platform/ThreadGlobalData.h:109 > + GBMDevice& gbmDevice() const GBMDevice& > Source/WebCore/platform/ThreadGlobalData.h:137 > +#if USE(ANGLE) && USE(NICOSIA) > + std::unique_ptr<GBMDevice> m_gbmDevice; > +#endif Rather than latching it onto ThreadGlobalData, this could be implemented via ThreadSpecific<>. See TextureMapperContextAttributes. > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:412 > + Vector<EGLint> intAttribList; > + for (int i = 0; attribList[i] != EGL_NONE; i += 2) { > + intAttribList.append(attribList[i]); > + intAttribList.append(attribList[i+1]); > + } > + intAttribList.append(EGL_NONE); Feels like WTF::map(). > Source/WebCore/platform/graphics/gbm/GBMDevice.h:35 > +class ThreadGlobalData; Forward declaration not necessary. > Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:41 > +#include "gbm.h" It's a system header, it's supposed to be included in brackets-form. > Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:45 > + > +#include <fcntl.h> This ends up included only for the !USE(NICOSIA) block.

Comment on attachment 450768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450768&action=review > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:547 > #if USE(COORDINATED_GRAPHICS) > std::swap(m_texture, m_compositorTexture); > std::swap(m_texture, m_intermediateTexture); > +#if USE(NICOSIA) > + std::swap(m_textureBacking, m_compositorTextureBacking); > + std::swap(m_textureBacking, m_intermediateTextureBacking); > +#endif I think COORDINATED_GRAPHICS and NICOSIA mean basically the same, there are more situations like this in the patch, maybe we want to check if there is a m_textureBacking like it is done in swapBuffers funtion? > Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:278 > +#if USE(NICOSIA) Ditto. > Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:291 > +#if USE(NICOSIA) Ditto. > Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:298 > +#if USE(NICOSIA) Ditto. > Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:307 > +#if USE(NICOSIA) Ditto.

Created attachment 450780 [details] Patch

Committed r289106 (246803@main): <https://commits.webkit.org/246803@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 450780 [details].

<rdar://problem/88475916>