[webkit-reviews] review granted: [Bug 237328] [GTK][WPE] Provide DMABuf-based composition layers, DMABufVideoSink integration : [Attachment 454329] Patch

[bugzilla-daemon at webkit.org]

Alejandro G. Castro <alex at igalia.com> has granted Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 237328: [GTK][WPE] Provide DMABuf-based composition layers, DMABufVideoSink
integration
https://bugs.webkit.org/show_bug.cgi?id=237328

Attachment 454329: Patch

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




--- Comment #10 from Alejandro G. Castro <alex at igalia.com> ---
Comment on attachment 454329
  --> https://bugs.webkit.org/attachment.cgi?id=454329
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.c
pp: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.