Bug 143299 – [ThreadedCompositor] Add support for PlatformLayer.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gwang Yoon Hwang]

[ThreadedCompositor] Add support for PlatformLayer.

[Gwang Yoon Hwang]

Created attachment 249906 [details]
Patch

[ChangSeok Oh]

Comment on attachment 249906 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/BitmapTextureGL.h:71
> +    GC3Dint internalFormat() const { return m_internalFormat; }

I think m_internalFormat is missing here.

[ChangSeok Oh]

Comment on attachment 249906 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:52
> +    return m_texture->canReuseWith(size) && (static_cast<BitmapTextureGL*>(m_texture.get())->internalFormat() == internalFormat || internalFormat == GraphicsContext3D::DONT_CARE);

BitmapTextureGL::canReuseWith has been removed after https://bugs.webkit.org/show_bug.cgi?id=143298. So it needs to be replaced with size() I think.

[Gwang Yoon Hwang]

(In reply to comment #3)
> Comment on attachment 249906 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249906&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:52
> > +    return m_texture->canReuseWith(size) && (static_cast<BitmapTextureGL*>(m_texture.get())->internalFormat() == internalFormat || internalFormat == GraphicsContext3D::DONT_CARE);
> 
> BitmapTextureGL::canReuseWith has been removed after
> https://bugs.webkit.org/show_bug.cgi?id=143298. So it needs to be replaced
> with size() I think.

Thanks for the comment, I'm cleaning up this patch to update it now.
Maybe it will be updated at tomorrow.

[Gwang Yoon Hwang]

Created attachment 264515 [details]
Patch

[Zan Dobersek]

Comment on attachment 264515 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:36
> +TextureMapperPlatformLayerBuffer::TextureMapperPlatformLayerBuffer(RefPtr<BitmapTexture>&& texture)
> +    : m_texture(WTF::move(texture))
> +    , m_extraFlags(0)
> +    , m_hasManagedTexture(true)
> +{
> +}

This should initialize m_textureID to 0.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:68
> +}; // namespace WebCore

No need for the semicolon.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:57
> +        UnmanagedBufferDataHolder() { }

The constructor definition shouldn't be required at all. If it is, you can default it, just like the destructor.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:76
> +};

No need for the semicolon. Add `// namespace WebCore` to note that this is the end of the namespace scope.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:43
> +    : m_compositor(0)
> +    , m_targetLayer(0)

nullptr

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:45
> +    , m_runLoop(RunLoop::current())
> +    , m_releaseUnusedBuffersTimer(m_runLoop, this, &TextureMapperPlatformLayerProxy::releaseUnusedBuffersTimerFired)

m_runLoop isn't used anywhere else -- no need to keep a reference to it in a member variable.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:154
> +};

No need for the semicolon. Add `// namespace WebCore` to note that this is the end of the namespace scope.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.h:95
> +};

No need for the semicolon. Add `// namespace WebCore` to note that this is the end of the namespace scope.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:88
> +    for (PlatformLayerProxyMap::iterator it = m_platformLayerProxies.begin(); it != m_platformLayerProxies.end(); ++it)
> +        it->value->swapBuffer();

for (auto& proxy : m_platformLayerProxies.values())
    proxy->swapBuffer();

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:194
> +        state.platformLayerProxy->activateOnCompositingThread(this, layer);

There shouldn't be a need to call this every time the proxy changes. Perhaps only when the layer key is added for the first time, or when the proxy for the layer changes?

[Gwang Yoon Hwang]

Created attachment 264679 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #6)
> Comment on attachment 264515 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264515&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:36
> > +TextureMapperPlatformLayerBuffer::TextureMapperPlatformLayerBuffer(RefPtr<BitmapTexture>&& texture)
> > +    : m_texture(WTF::move(texture))
> > +    , m_extraFlags(0)
> > +    , m_hasManagedTexture(true)
> > +{
> > +}
> 
> This should initialize m_textureID to 0.
> 

Nice catch, fixed!

> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:68
> > +}; // namespace WebCore
> 
> No need for the semicolon.
> 
Again, fixed.

> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:57
> > +        UnmanagedBufferDataHolder() { }
> 
> The constructor definition shouldn't be required at all. If it is, you can
> default it, just like the destructor.
> 

Because it is non-copyable, we should provide at least one constructor.
I fixed it to use default keyword.

> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:76
> > +};
> 
> No need for the semicolon. Add `// namespace WebCore` to note that this is
> the end of the namespace scope.
> 

Ditto.

> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:43
> > +    : m_compositor(0)
> > +    , m_targetLayer(0)
> 
> nullptr
> 

Nice catch!

> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:45
> > +    , m_runLoop(RunLoop::current())
> > +    , m_releaseUnusedBuffersTimer(m_runLoop, this, &TextureMapperPlatformLayerProxy::releaseUnusedBuffersTimerFired)
> 
> m_runLoop isn't used anywhere else -- no need to keep a reference to it in a
> member variable.
>

Removed.
 
> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:154
> > +};
> 
> No need for the semicolon. Add `// namespace WebCore` to note that this is
> the end of the namespace scope.
> 

Ditto
> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.h:95
> > +};
> 
> No need for the semicolon. Add `// namespace WebCore` to note that this is
> the end of the namespace scope.
>

Ditto
 
> > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:88
> > +    for (PlatformLayerProxyMap::iterator it = m_platformLayerProxies.begin(); it != m_platformLayerProxies.end(); ++it)
> > +        it->value->swapBuffer();
> 
> for (auto& proxy : m_platformLayerProxies.values())
>     proxy->swapBuffer();
>

A nice tidy statement, replaced.

> > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:194
> > +        state.platformLayerProxy->activateOnCompositingThread(this, layer);
> 
> There shouldn't be a need to call this every time the proxy changes. Perhaps
> only when the layer key is added for the first time, or when the proxy for
> the layer changes?

In the threaded compositor, platformLayerChanged means the proxy has been created or deleted. And activating the proxy only happens at its creation time.
AFAIK, there are no use cases which changes the layer's proxy except removing.
So I think this statement satisfies your worry.

[Zan Dobersek]

Comment on attachment 264679 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext3DPrivate.cpp:87
> +#if USE(COORDINATED_GRAPHICS_THREADED)
> +RefPtr<TextureMapperPlatformLayerProxy> GraphicsContext3DPrivate::proxy() const
> +{
> +    notImplemented();
> +    return RefPtr<TextureMapperPlatformLayerProxy>();
> +}

This can just return nullptr.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:420
> +RefPtr<TextureMapperPlatformLayerProxy> ImageBufferData::proxy() const
> +{
> +    notImplemented();
> +    return RefPtr<TextureMapperPlatformLayerProxy>();
> +}

Same here.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:49
> +TextureMapperPlatformLayerBuffer::~TextureMapperPlatformLayerBuffer()
> +{
> +}

This too can be defaulted. Either here or in the constructor.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:118
> +    if (m_releaseUnusedBuffersTimer.isActive())
> +        return;
> +
> +    m_releaseUnusedBuffersTimer.startOneShot(s_releaseUnusedBuffersTimerInterval);

This can just be a two-liner, by starting the timer if it's not yet active.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.h:66
> +    std::unique_ptr<TextureMapperPlatformLayerBuffer> getAvailableBuffer(const IntSize&, GC3Dint internalFormat);
> +    void pushNextBuffer(std::unique_ptr<TextureMapperPlatformLayerBuffer>);

What do you think about exposing the m_lock object, and accepting a LockHolder& as the first parameter of getAvailableBuffer() and pushNextBuffer(), like done in [1]? That way there's no possibility of the composition thread swapping the buffers while the provider thread is still working on pushing the next buffer.

[1] https://github.com/WebKitForWayland/webkit/blob/master/Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.h#L64

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:387
> +    if (auto* platformLayerProxy = m_platformLayerProxies.get(layer.get()))
> +        platformLayerProxy->invalidate();
> +    m_platformLayerProxies.remove(layer.get());

Replace this with HashMap::take(). That way you only have to look up the key once.

[Gwang Yoon Hwang]

Created attachment 266754 [details]
Patch

[Gwang Yoon Hwang]

Thanks for review!

(In reply to comment #9)
> Comment on attachment 264679 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264679&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext3DPrivate.cpp:87
> > +#if USE(COORDINATED_GRAPHICS_THREADED)
> > +RefPtr<TextureMapperPlatformLayerProxy> GraphicsContext3DPrivate::proxy() const
> > +{
> > +    notImplemented();
> > +    return RefPtr<TextureMapperPlatformLayerProxy>();
> > +}
> 
> This can just return nullptr.

Nice, fixed it.

> 
> > Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:420
> > +RefPtr<TextureMapperPlatformLayerProxy> ImageBufferData::proxy() const
> > +{
> > +    notImplemented();
> > +    return RefPtr<TextureMapperPlatformLayerProxy>();
> > +}
> 
> Same here.
> 

Ditto

> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:49
> > +TextureMapperPlatformLayerBuffer::~TextureMapperPlatformLayerBuffer()
> > +{
> > +}
> 
> This too can be defaulted. Either here or in the constructor.
> 

Oh, I missed that.

> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:118
> > +    if (m_releaseUnusedBuffersTimer.isActive())
> > +        return;
> > +
> > +    m_releaseUnusedBuffersTimer.startOneShot(s_releaseUnusedBuffersTimerInterval);
> 
> This can just be a two-liner, by starting the timer if it's not yet active.
> 

Modified.

> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.h:66
> > +    std::unique_ptr<TextureMapperPlatformLayerBuffer> getAvailableBuffer(const IntSize&, GC3Dint internalFormat);
> > +    void pushNextBuffer(std::unique_ptr<TextureMapperPlatformLayerBuffer>);
> 
> What do you think about exposing the m_lock object, and accepting a
> LockHolder& as the first parameter of getAvailableBuffer() and
> pushNextBuffer(), like done in [1]? That way there's no possibility of the
> composition thread swapping the buffers while the provider thread is still
> working on pushing the next buffer.
> 
> [1]
> https://github.com/WebKitForWayland/webkit/blob/master/Source/WebCore/
> platform/graphics/texmap/TextureMapperPlatformLayerProxy.h#L64
> 

I think it is better to skip the frame instead of blocking the working thread (main thread or decoding thread).
Is there any other use-cases which enforcing not to skip a frame?

> > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:387
> > +    if (auto* platformLayerProxy = m_platformLayerProxies.get(layer.get()))
> > +        platformLayerProxy->invalidate();
> > +    m_platformLayerProxies.remove(layer.get());
> 
> Replace this with HashMap::take(). That way you only have to look up the key
> once.
I didn't know that there is a ::take() method. Thanks!

[Gwang Yoon Hwang]

Created attachment 266767 [details]
Patch

[Gwang Yoon Hwang]

Created attachment 266768 [details]
Patch

[Zan Dobersek]

(In reply to comment #10)
> Created attachment 266754 [details]
> Patch

(In reply to comment #12)
> Created attachment 266767 [details]
> Patch

(In reply to comment #13)
> Created attachment 266768 [details]
> Patch

All these patches failed to address feedback from comment #6 (which was addressed in attachment #264679 [details] from November 3rd).

[Gwang Yoon Hwang]

Created attachment 266774 [details]
Patch

[Zan Dobersek]

Comment on attachment 266774 [details]
Patch

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

r=me

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:51
> +    if (m_targetLayer)
> +        m_targetLayer->setContentsLayer(nullptr);

Should a lock be held here? Elsewhere m_targetLayer is operated on with a held lock.

[Gwang Yoon Hwang]

Created attachment 266776 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 266776 [details]
Patch

Clearing flags on attachment: 266776

Committed r193630: <http://trac.webkit.org/changeset/193630>

[Gwang Yoon Hwang]

All patches are landed. Closing the bug.