Bug 237328 - [GTK][WPE] Provide DMABuf-based composition layers, DMABufVideoSink integration
Summary: [GTK][WPE] Provide DMABuf-based composition layers, DMABufVideoSink integration
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: DMA-BUF
  Show dependency treegraph
 
Reported: 2022-03-01 09:23 PST by Zan Dobersek
Modified: 2022-03-16 06:13 PDT (History)
14 users (show)

See Also:


Attachments
WIP patch (38.95 KB, patch)
2022-03-01 09:25 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (39.85 KB, patch)
2022-03-03 01:23 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (39.58 KB, patch)
2022-03-03 06:46 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (78.46 KB, patch)
2022-03-04 07:09 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (78.48 KB, patch)
2022-03-10 03:02 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch for landing (78.63 KB, patch)
2022-03-15 00:33 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (78.53 KB, patch)
2022-03-16 04:06 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-03-01 09:23:37 PST
[GTK][WPE] Add TextureMapperPlatformLayerProxyDMABuf
Comment 1 Zan Dobersek 2022-03-01 09:25:31 PST
Created attachment 453504 [details]
WIP patch
Comment 2 Zan Dobersek 2022-03-03 01:23:13 PST
Created attachment 453709 [details]
Patch
Comment 3 Chris Lord 2022-03-03 03:01:18 PST
Comment on attachment 453709 [details]
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.cpp: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("eglDestroyImageKHR"));
> +        });
> +    return s_destroyImageKHR;
> +}

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

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp: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.cpp: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.cpp:159
> +    // Avoid any funky situation where the two layers are the same.

Same question about legitimacy/asserting.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp: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.cpp: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.cpp: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.cpp: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
Comment 4 Zan Dobersek 2022-03-03 05:37:39 PST
Comment on attachment 453709 [details]
Patch

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

>> Source/WebCore/platform/graphics/gbm/DMABufFormat.h:98
>> +    }
> 
> I don't know why someone might ever do that, but maybe worth asserting that planeIndex < numPlanes here.

OK.

>> 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.

I don't think it makes sense having to revisit plane definitions every time a new format is introduced. That's why they are all provided here, from the beginning.

>> Source/WebCore/platform/graphics/gbm/DMABufFormat.h:364
>> +        break;
> 
> Should we ASSERT_NOT_REACHED() here?

Can't guarantee every FourCC value (e.g. something from GStreamer) will be covered here. So an 'invalid' DMABufFormat is returned.

>> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:68
>> +}
> 
> Is there a reason to add these instead of using the ones in GLContextEGL?

Yes, there's no construction of GLContextEGL. And they would be eliminated once libepoxy is adopted by the GTK port.

>> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:122
>> +    // Avoid any funky situation where the two layers are the same.
> 
> Can this legitimately happen? Should we assert something here?

I don't know, but it can be prevented if it ever does. Debug-time assert can be used to signal it.

>> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:152
>> +    }
> 
> Is there any reason not to use HashMap::removeIf here instead of recreating LayerMap?

It should be possible to use it.

>> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp: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?

I'll remove this.

>> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp: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.

I'll remove this too, and try and push the texture/image destruction into the EGLImageData dtor.

>> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:220
>> +        // TODO: YUV support, possible when different colorspaces are supported.
> 
> I'd add an ASSERT_NOT_REACHED to go with this TODO.

OK.

>> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:250
>> +        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

Not going to construct it. But I'll switch to using Vector<>.
Comment 5 Zan Dobersek 2022-03-03 06:44:16 PST
(In reply to Chris Lord from comment #3)
> Comment on attachment 453709 [details]
> 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.
> 

I'll upload the updated patch. Review it if you have time, but I'd prefer to roll it into a bigger patch at this point, not smaller ones.
Comment 6 Zan Dobersek 2022-03-03 06:46:59 PST
Created attachment 453729 [details]
Patch
Comment 7 Chris Lord 2022-03-03 06:50:10 PST
(In reply to Zan Dobersek from comment #5)
> I'll upload the updated patch. Review it if you have time, but I'd prefer to
> roll it into a bigger patch at this point, not smaller ones.

That sounds good, I'll wait for that.
Comment 8 Zan Dobersek 2022-03-04 07:09:09 PST
Created attachment 453839 [details]
Patch
Comment 9 Zan Dobersek 2022-03-10 03:02:35 PST
Created attachment 454329 [details]
Patch
Comment 10 Alejandro G. Castro 2022-03-14 13:34:04 PDT
Comment on attachment 454329 [details]
Patch

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

Awesome patch! Thanks, let's do it! Just added a couple of small comments.

> Source/WebCore/platform/graphics/gbm/DMABufReleaseFlag.h:38
> +    enum InitializeTag { Initialize };
> +    DMABufReleaseFlag(InitializeTag)

InitializeTag is not used, is it a left-over?

> Source/WebCore/platform/graphics/gbm/GBMBufferSwapchain.cpp:47
> +    // If the description of the requested byffers has changed, update the description to the new one and wreck the existing buffers.

Typo byffers.

> Source/WebCore/platform/graphics/gbm/GBMBufferSwapchain.cpp:142
> +    m_state.releaseFlag = DMABufReleaseFlag(DMABufReleaseFlag::Initialize);

Ditto bout the InitializeTag.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyDMABuf.cpp:123
> +void TextureMapperPlatformLayerProxyDMABuf::invalidate()
> +{
> +    Locker locker { m_lock };
> +
> +    m_pendingLayer = nullptr;
> +    m_committedLayer = nullptr;
> +    m_layers = { };
> +
> +    m_compositor = nullptr;
> +    m_targetLayer = nullptr;
> +}

If we don't set m_compositorThread to nullptr we can be asserting if the layer tree is reused when the threaded compositor is destroyed, it can happen when using the history.
Comment 11 Zan Dobersek 2022-03-15 00:11:21 PDT
Comment on attachment 454329 [details]
Patch

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

>> Source/WebCore/platform/graphics/gbm/DMABufReleaseFlag.h:38
>> +    DMABufReleaseFlag(InitializeTag)
> 
> InitializeTag is not used, is it a left-over?

It's not unused, it's the enumeration type.
Comment 12 Zan Dobersek 2022-03-15 00:33:46 PDT
Created attachment 454674 [details]
Patch for landing
Comment 13 EWS 2022-03-15 00:34:45 PDT
zan@falconsigh.net does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json.

Rejecting attachment 454674 [details] from commit queue.
Comment 14 EWS 2022-03-15 01:56:38 PDT
Committed r291270 (248419@main): <https://commits.webkit.org/248419@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454674 [details].
Comment 15 Radar WebKit Bug Importer 2022-03-15 01:57:19 PDT
<rdar://problem/90295492>
Comment 16 Zan Dobersek 2022-03-16 01:25:48 PDT
Rolled out in #237908.
Comment 17 Zan Dobersek 2022-03-16 04:06:50 PDT
Created attachment 454821 [details]
Patch
Comment 18 Alejandro G. Castro 2022-03-16 05:13:29 PDT
Comment on attachment 454821 [details]
Patch

LGTM. Let's try it again. We need to link here the next patch with the gbm guards.
Comment 19 EWS 2022-03-16 06:13:41 PDT
Committed r291343 (248482@main): <https://commits.webkit.org/248482@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454821 [details].