Bug 143299

Created attachment 249906 [details] Patch

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

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

Created attachment 264515 [details] Patch

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?

Created attachment 264679 [details] Patch

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

Created attachment 266754 [details] Patch

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!

Created attachment 266767 [details] Patch

Created attachment 266768 [details] Patch

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

Created attachment 266774 [details] Patch

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.

Created attachment 266776 [details] Patch

Comment on attachment 266776 [details] Patch Clearing flags on attachment: 266776 Committed r193630: <http://trac.webkit.org/changeset/193630>

All patches are landed. Closing the bug.