[webkit-reviews] review denied: [Bug 235946] [GTK][WPE] Use dmabuf when possible to transfer ANGLE rendering to the compositor : [Attachment 450512] Patch

[bugzilla-daemon at webkit.org]

Zan Dobersek <zan at falconsigh.net> has denied Chris Lord <clord at igalia.com>'s
request for review:
Bug 235946: [GTK][WPE] Use dmabuf when possible to transfer ANGLE rendering to
the compositor
https://bugs.webkit.org/show_bug.cgi?id=235946

Attachment 450512: Patch

https://bugs.webkit.org/attachment.cgi?id=450512&action=review




--- Comment #2 from Zan Dobersek <zan at falconsigh.net> ---
Comment on attachment 450512
  --> https://bugs.webkit.org/attachment.cgi?id=450512
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.