[webkit-reviews] review denied: [Bug 237328] [GTK][WPE] Add TextureMapperPlatformLayerProxyDMABuf : [Attachment 453709] Patch

[bugzilla-daemon at webkit.org]

Chris Lord <clord at igalia.com> has denied Zan Dobersek <zan at falconsigh.net>'s
request for review:
Bug 237328: [GTK][WPE] Add TextureMapperPlatformLayerProxyDMABuf
https://bugs.webkit.org/show_bug.cgi?id=237328

Attachment 453709: Patch

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




--- Comment #3 from Chris Lord <clord at igalia.com> ---
Comment on attachment 453709
  --> https://bugs.webkit.org/attachment.cgi?id=453709
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453709&action=review

Maybe I'm being overly cautious, but there's just too much unused and untested
code here and I have too many questions to grant a review. I think there are a
few options that make sense to me:
- Split this into multiple patches and introduce API tests to verify expected
behaviour.
- Reduce the size of this patch so there's less unused code added in one go and
be a bit more liberal with comments/asserts.
- Submit a more complete patch where the code is exercised and it's more
obvious the reasons for things because there's more surrounding context to
judge it.

For the record, I like where this patch is going, I just don't think this is
ready to land quite yet. The detailed changelog entry is great and appreciated.

> Source/WebCore/platform/graphics/gbm/DMABufFormat.h:98
> +    uint32_t planeWidth(unsigned planeIndex, uint32_t width) const
> +    {
> +	   return (width >> planes[planeIndex].horizontalSubsampling);
> +    }
> +
> +    uint32_t planeHeight(unsigned planeIndex, uint32_t height) const
> +    {
> +	   return (height >> planes[planeIndex].verticalSubsampling);
> +    }

I don't know why someone might ever do that, but maybe worth asserting that
planeIndex < numPlanes here.

> Source/WebCore/platform/graphics/gbm/DMABufFormat.h:156
> +inline DMABufFormat DMABufFormat::create<DMABufFormat::FourCC::XRGB8888>()

Given that none of these create functions are used in this patch, maybe it's
worth introducing them with the patch that uses them? I'm wary of adding this
much unused, untested code, even if it's as simple as this.

> Source/WebCore/platform/graphics/gbm/DMABufFormat.h:364
> +	   break;

Should we ASSERT_NOT_REACHED() here?

> Source/WebCore/platform/graphics/gbm/DMABufReleaseFlag.h:65
> +	   this->~DMABufReleaseFlag();
> +	   new (this) DMABufReleaseFlag(WTFMove(o));

This is quite clever, but it seems fragile to future possible
constructor/destructor changes. It's also not a form that exists anywhere else
in WebCore. I can't think of an alternative that isn't more verbose, however,
so maybe my dislike is irrational.

>
Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.c
pp:68
> +static PFNEGLCREATEIMAGEKHRPROC createImageKHR()
> +{
> +    static PFNEGLCREATEIMAGEKHRPROC s_createImageKHR;
> +    static std::once_flag s_flag;
> +    std::call_once(s_flag,
> +	   [&] {
> +	       s_createImageKHR =
reinterpret_cast<PFNEGLCREATEIMAGEKHRPROC>(eglGetProcAddress("eglCreateImageKHR
"));
> +	   });
> +    return s_createImageKHR;
> +}
> +
> +static PFNEGLDESTROYIMAGEKHRPROC destroyImageKHR()
> +{
> +    static PFNEGLDESTROYIMAGEKHRPROC s_destroyImageKHR;
> +    static std::once_flag s_flag;
> +    std::call_once(s_flag,
> +	   [&] {
> +	       s_destroyImageKHR =
reinterpret_cast<PFNEGLDESTROYIMAGEKHRPROC>(eglGetProcAddress("eglDestroyImageK
HR"));
> +	   });
> +    return s_destroyImageKHR;
> +}

Is there a reason to add these instead of using the ones in GLContextEGL?

>
Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.c
pp:122
> +    // Avoid any funky situation where the two layers are the same.

Can this legitimately happen? Should we assert something here?

>
Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.c
pp:152
> +    if (hasStaleLayers) {
> +	   LayerMap updatedLayers;
> +	   for (auto it = m_layers.begin(); it != m_layers.end(); ++it) {
> +	       if (!isStaleLayer(it->value.get()))
> +		   updatedLayers.add(it->key, it->value.copyRef());
> +	   }
> +	   m_layers = WTFMove(updatedLayers);
> +    }

Is there any reason not to use HashMap::removeIf here instead of recreating
LayerMap?

>
Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.c
pp:159
> +    // Avoid any funky situation where the two layers are the same.

Same question about legitimacy/asserting.

>
Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.c
pp:175
> +    m_object = DMABufObject(0);

It doesn't look like this function calls out to anything, is there any reason
to explicitly destroy this here?

>
Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.c
pp:190
> +    m_imageData = nullptr;

If we're explicitly clearing this, may as well put it inside the above block -
but I'd suggest just omitting it unless there's a specific reason.

>
Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.c
pp:220
> +	   // TODO: YUV support, possible when different colorspaces are
supported.

I'd add an ASSERT_NOT_REACHED to go with this TODO.

>
Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.c
pp:250
> +	   attributes[attrIndex++] = EGL_WIDTH;
> +	   attributes[attrIndex++] = object.format.planeWidth(i, object.width);
> +	   attributes[attrIndex++] = EGL_HEIGHT;
> +	   attributes[attrIndex++] = object.format.planeHeight(i,
object.height);
> +	   attributes[attrIndex++] = EGL_LINUX_DRM_FOURCC_EXT;
> +	   attributes[attrIndex++] = EGLint(object.format.planes[i].fourcc);
> +	   attributes[attrIndex++] = EGL_DMA_BUF_PLANE0_FD_EXT;
> +	   attributes[attrIndex++] = object.fd[i];
> +	   attributes[attrIndex++] = EGL_DMA_BUF_PLANE0_OFFSET_EXT;
> +	   attributes[attrIndex++] = object.offset[i];
> +	   attributes[attrIndex++] = EGL_DMA_BUF_PLANE0_PITCH_EXT;
> +	   attributes[attrIndex++] = object.stride[i];
> +	   attributes[attrIndex++] = EGL_NONE;

Using the existing GLContextEGL::createImage would allow you to use the nicer
attributes construction that alexg added - see
platform/graphics/texmap/TextureMapperPlatformLayerDmabuf.cpp