Bug 240276 – [GTK][WPE] Respect and use the DMABuf modifier values

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[GTK][WPE] Respect and use the DMABuf modifier values

[Zan Dobersek]

Created attachment 459114 [details]
Patch

[Chris Lord]

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.

[Zan Dobersek]

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.

[Zan Dobersek]

Comment on attachment 459114 [details]
Patch

Clearing flags on attachment: 459114

Committed r294100 (250485@trunk): <https://commits.webkit.org/250485@trunk>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/93170769>