[webkit-reviews] review granted: [Bug 240276] [GTK][WPE] Respect and use the DMABuf modifier values : [Attachment 459114] Patch

[bugzilla-daemon at webkit.org]

Chris Lord <clord at igalia.com> has granted Zan Dobersek <zan at falconsigh.net>'s
request for review:
Bug 240276: [GTK][WPE] Respect and use the DMABuf modifier values
https://bugs.webkit.org/show_bug.cgi?id=240276

Attachment 459114: Patch

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




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

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

Looks good, some comments but nothing critical.

> Source/WebCore/platform/graphics/PlatformDisplay.cpp:283
> +    {
> +	   const char* extensionsString = eglQueryString(m_eglDisplay,
EGL_EXTENSIONS);
> +	   auto displayExtensions = StringView { extensionsString }.split(' ');
> +	   auto findExtension =
> +	       [&](auto extensionName) {
> +		   return std::any_of(displayExtensions.begin(),
displayExtensions.end(),
> +		       [&](auto extensionEntry) {
> +			   return extensionEntry == extensionName;
> +		       });
> +	       };
> +
> +	   m_eglExtensions.EXT_image_dma_buf_import_modifiers =
findExtension("EGL_EXT_image_dma_buf_import_modifiers"_s);
> +    }
> +

Is there a reason this is in braces? Also, though I like that the lambda
enables the final line to be very readable, it seems a bit wordy vs just

m_eglExtensions.EXT_image_dma_buf_import_modifiers =
std::find(displayExtensions.begin(), displayExtensions.end(),
"EGL_EXT_image_dma_buf_import_modifiers"_s) != displayExtensions.end();

It feels like this isn't really in keeping with the code surrounding it. Maybe
I've not fully understood something here though and it's not a dealbreaker or
anything.

>
Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.c
pp:271
> -	   std::initializer_list<EGLint> attributes {
> +	   Vector<EGLint> attributes;
> +	   attributes.reserveInitialCapacity(12 + 4 + 1);
> +	   attributes.uncheckedAppend(Span<const EGLint>({
>	       EGL_WIDTH, EGLint(object.format.planeWidth(i, object.width)),
>	       EGL_HEIGHT, EGLint(object.format.planeHeight(i, object.height)),
>	       EGL_LINUX_DRM_FOURCC_EXT,
EGLint(object.format.planes[i].fourcc),
>	       EGL_DMA_BUF_PLANE0_FD_EXT, object.fd[i].value(),
>	       EGL_DMA_BUF_PLANE0_OFFSET_EXT, EGLint(object.offset[i]),
>	       EGL_DMA_BUF_PLANE0_PITCH_EXT, EGLint(object.stride[i]),
> -	       EGL_NONE,
> -	   };
> -	   image[i] = createImageKHR()(eglDisplay, EGL_NO_CONTEXT,
EGL_LINUX_DMA_BUF_EXT, nullptr, std::data(attributes));
> +	   }));
> +	   if
(platformDisplay.eglExtensions().EXT_image_dma_buf_import_modifiers) {
> +	       attributes.uncheckedAppend(Span<const EGLint>({
> +		   EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT,
static_cast<EGLint>(object.modifier[i] >> 32),
> +		   EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT,
static_cast<EGLint>(object.modifier[i] & 0xffffffff),
> +	       }));
> +	   }
> +	   attributes.uncheckedAppend(EGL_NONE);

It's a shame to have this duplicated, maybe now's a good time to factor this
out into a utility function? Especially with the uncheckedAppend and manually
calculated capacity, it'd be nice to have that only done once.