Bug 143299 - [ThreadedCompositor] Add support for PlatformLayer.
Summary: [ThreadedCompositor] Add support for PlatformLayer.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gwang Yoon Hwang
URL:
Keywords:
Depends on: 143298
Blocks: 100341 143300 143301
  Show dependency treegraph
 
Reported: 2015-04-01 01:46 PDT by Gwang Yoon Hwang
Modified: 2015-12-07 13:36 PST (History)
5 users (show)

See Also:


Attachments
Patch (42.62 KB, patch)
2015-04-01 01:47 PDT, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff
Patch (45.50 KB, patch)
2015-11-01 06:57 PST, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff
Patch (45.47 KB, patch)
2015-11-03 02:30 PST, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff
Patch (45.26 KB, patch)
2015-12-06 23:59 PST, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff
Patch (45.63 KB, patch)
2015-12-07 03:47 PST, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff
Patch (45.60 KB, patch)
2015-12-07 04:02 PST, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff
Patch (45.60 KB, patch)
2015-12-07 08:17 PST, Gwang Yoon Hwang
zan: review+
Details | Formatted Diff | Diff
Patch (45.64 KB, patch)
2015-12-07 08:50 PST, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gwang Yoon Hwang 2015-04-01 01:46:35 PDT
[ThreadedCompositor] Add support for PlatformLayer.
Comment 1 Gwang Yoon Hwang 2015-04-01 01:47:51 PDT
Created attachment 249906 [details]
Patch
Comment 2 ChangSeok Oh 2015-10-23 03:30:54 PDT
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.
Comment 3 ChangSeok Oh 2015-10-25 23:53:49 PDT
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.
Comment 4 Gwang Yoon Hwang 2015-10-25 23:56:53 PDT
(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.
Comment 5 Gwang Yoon Hwang 2015-11-01 06:57:12 PST
Created attachment 264515 [details]
Patch
Comment 6 Zan Dobersek 2015-11-02 23:06:13 PST
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?
Comment 7 Gwang Yoon Hwang 2015-11-03 02:30:12 PST
Created attachment 264679 [details]
Patch
Comment 8 Gwang Yoon Hwang 2015-11-03 02:47:33 PST
(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.
Comment 9 Zan Dobersek 2015-11-10 04:44:26 PST
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.
Comment 10 Gwang Yoon Hwang 2015-12-06 23:59:27 PST
Created attachment 266754 [details]
Patch
Comment 11 Gwang Yoon Hwang 2015-12-07 00:02:36 PST
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!
Comment 12 Gwang Yoon Hwang 2015-12-07 03:47:47 PST
Created attachment 266767 [details]
Patch
Comment 13 Gwang Yoon Hwang 2015-12-07 04:02:07 PST
Created attachment 266768 [details]
Patch
Comment 14 Zan Dobersek 2015-12-07 04:10:36 PST
(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).
Comment 15 Gwang Yoon Hwang 2015-12-07 08:17:24 PST
Created attachment 266774 [details]
Patch
Comment 16 Zan Dobersek 2015-12-07 08:30:17 PST
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.
Comment 17 Gwang Yoon Hwang 2015-12-07 08:50:22 PST
Created attachment 266776 [details]
Patch
Comment 18 WebKit Commit Bot 2015-12-07 09:16:14 PST
Comment on attachment 266776 [details]
Patch

Clearing flags on attachment: 266776

Committed r193630: <http://trac.webkit.org/changeset/193630>
Comment 19 Gwang Yoon Hwang 2015-12-07 13:36:31 PST
All patches are landed. Closing the bug.