Created attachment 459114 [details] Patch

Comment on attachment 459114 [details] 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.cpp: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.

Comment on attachment 459114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459114&action=review >> Source/WebCore/platform/graphics/PlatformDisplay.cpp:283 >> + > > 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. The scope block limits the use of the queried string and the split-result Vector to just this block, avoiding accidental use in subsequent changes outside of this scope. Lambda is provided for reuse when detecting additional extensions in the future. >> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:271 >> + 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. I'll do it in a subsequent patch cause it will require a separate header that utilizes EGL.

Comment on attachment 459114 [details] Patch Clearing flags on attachment: 459114 Committed r294100 (250485@trunk): <https://commits.webkit.org/250485@trunk>

All reviewed patches have been landed. Closing bug.

<rdar://problem/93170769>