This patch makes sync canvas code handle the lifecycle of the canvas GraphicsSurface in the similar style to a directly image compositing and an update atlas code. In conclusion, this patch moves the canvas lifecycle handling code from LayerTreeRenderer to CoordinatedGraphicsLayer, because CoordinatedGraphicsLayer knows best when to create and remove the canvas GraphicsSurface. After this patch, we can remove the canvas GraphicsSurface in UI Process as soon as the canvas platform layer is unset in Web Process.
Created attachment 174919 [details] Patch
Comment on attachment 174919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174919&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:-247 > - if (canvasSize.isEmpty() || !m_textureMapper) Could you answer does GraphicsSurface need m_textureMapper? I think GraphicsSurface has its own GL Context. I can make LayerTreeCoordinator early return if canvasSize.isEmpty(). If so, we must remove ASSERT(m_surfaceBackingStores.contains(id)) in LayerTreeRenderer::removeCanvas(), because we cannot know the canvasSize in LayerTreeCoordinator::removeCanvas. Which is better? > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:-250 > - ensureLayer(id); I treated it as ASSERT.
Comment on attachment 174919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174919&action=review >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:-247 >> - if (canvasSize.isEmpty() || !m_textureMapper) > > Could you answer does GraphicsSurface need m_textureMapper? I think GraphicsSurface has its own GL Context. > > I can make LayerTreeCoordinator early return if canvasSize.isEmpty(). If so, we must remove ASSERT(m_surfaceBackingStores.contains(id)) in LayerTreeRenderer::removeCanvas(), because we cannot know the canvasSize in LayerTreeCoordinator::removeCanvas. Which is better? The current implementations of GraphicsSurface all rely on TextureMapper being available for drawing the transferred texture on screen. This is why we check if we have a TextureMapper in this place. It does/did not make sense to create a GraphicsSurface without having a TextureMapper. Unless of course, you are going to use a different method for drawing.
(In reply to comment #3) > (From update of attachment 174919 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174919&action=review > > >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:-247 > >> - if (canvasSize.isEmpty() || !m_textureMapper) > > > > Could you answer does GraphicsSurface need m_textureMapper? I think GraphicsSurface has its own GL Context. > > > > I can make LayerTreeCoordinator early return if canvasSize.isEmpty(). If so, we must remove ASSERT(m_surfaceBackingStores.contains(id)) in LayerTreeRenderer::removeCanvas(), because we cannot know the canvasSize in LayerTreeCoordinator::removeCanvas. Which is better? > > The current implementations of GraphicsSurface all rely on TextureMapper being available for drawing the transferred texture on screen. > This is why we check if we have a TextureMapper in this place. It does/did not make sense to create a GraphicsSurface without having a TextureMapper. > Unless of course, you are going to use a different method for drawing. Thanks for answer! LayerTreeRenderer::syncRemoteContent() only calls three methods: createCanvas(), syncCanvas() and removeCanvas(), and syncRemoteContent() creates TextureMapper if it does not exist. So how about putting ASSERT instead of the if statement?
(In reply to comment #4) > LayerTreeRenderer::syncRemoteContent() only calls three methods: createCanvas(), syncCanvas() and removeCanvas(), and syncRemoteContent() creates TextureMapper if it does not exist. So how about putting ASSERT instead of the if statement? That sounds good to me, as it is not meant to be used without TextureMapper anyway. The patch overall looks very nice to me. Thanks for the great work! :)
Comment on attachment 174919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174919&action=review Please fix comments before cq? (You don't need to r? again) > Source/WebCore/ChangeLog:8 > + As refactoring Coordinated Graphics in WebKit2, code related to TexMap TexMap -> TextureMapper > Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h:53 > +class GraphicsSurface : public ThreadSafeRefCounted<GraphicsSurface> { This belongs in a different patch. > Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:42 > + if (!surface) { > + m_graphicsSurface.clear(); > + return; > } This is redundant. > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeCoordinatorProxy.messages.in:54 > + RemoveCanvas(uint32_t id) DestroyCanvas. (Remove pairs with Add, Destroy pairs with Create)
Created attachment 175042 [details] Patch
(In reply to comment #5) > That sounds good to me, as it is not meant to be used without TextureMapper anyway. > > The patch overall looks very nice to me. Thanks for the great work! :) Thanks for review! (In reply to comment #6) > (From update of attachment 174919 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174919&action=review > > Please fix comments before cq? > (You don't need to r? again) OK :) > > TexMap -> TextureMapper Done > > This belongs in a different patch. Yes. Do you think we should change ThreadSafeRefCounted? > This is redundant. Done > DestroyCanvas. > (Remove pairs with Add, Destroy pairs with Create) Done. Oops. I'm afraid that there are some create and remove pairs in coordinated graphics I made..
Comment on attachment 175042 [details] Patch Clearing flags on attachment: 175042 Committed r135200: <http://trac.webkit.org/changeset/135200>
All reviewed patches have been landed. Closing bug.