Bug 240276 - [GTK][WPE] Respect and use the DMABuf modifier values
Summary: [GTK][WPE] Respect and use the DMABuf modifier values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: InRadar
Depends on:
Blocks: 237649
  Show dependency treegraph
 
Reported: 2022-05-10 05:55 PDT by Zan Dobersek
Modified: 2022-05-13 00:01 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.35 KB, patch)
2022-05-10 05:56 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2022-05-10 05:55:14 PDT
[GTK][WPE] Respect and use the DMABuf modifier values
Comment 1 Zan Dobersek 2022-05-10 05:56:23 PDT
Created attachment 459114 [details]
Patch
Comment 2 Chris Lord 2022-05-10 06:30:41 PDT
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 3 Zan Dobersek 2022-05-10 07:42:46 PDT
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 4 Zan Dobersek 2022-05-12 06:30:31 PDT
Comment on attachment 459114 [details]
Patch

Clearing flags on attachment: 459114

Committed r294100 (250485@trunk): <https://commits.webkit.org/250485@trunk>
Comment 5 Zan Dobersek 2022-05-12 06:30:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2022-05-12 06:31:12 PDT
<rdar://problem/93170769>