Bug 102664

Summary:

Coordinated Graphics: refactor syncCanvas to handle the lifecycle clearly.

Product:

WebKit

Reporter:

Dongseong Hwang <dongseong.hwang>

Component:

Layout and Rendering

Assignee:

Dongseong Hwang <dongseong.hwang>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dino, levin+threading, noam, webkit.review.bot, zeno

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none

Dongseong Hwang

Reported 2012-11-19 01:50:07 PST

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.

Attachments
Patch (27.68 KB, patch) 2012-11-19 02:04 PST, Dongseong Hwang	no flags	Details Formatted Diff Diff
Patch (26.89 KB, patch) 2012-11-19 14:37 PST, Dongseong Hwang	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Dongseong Hwang

Comment 1 2012-11-19 02:04:56 PST

Created attachment 174919 [details] Patch

Dongseong Hwang

Comment 2 2012-11-19 02:40:22 PST

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.

Zeno Albisser

Comment 3 2012-11-19 02:51:27 PST

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.

Dongseong Hwang

Comment 4 2012-11-19 03:16:45 PST

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

Zeno Albisser

Comment 5 2012-11-19 03:20:17 PST

(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! :)

Noam Rosenthal

Comment 6 2012-11-19 07:33:45 PST

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)

Dongseong Hwang

Comment 7 2012-11-19 14:37:47 PST

Created attachment 175042 [details] Patch

Dongseong Hwang

Comment 8 2012-11-19 14:41:13 PST

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

WebKit Review Bot

Comment 9 2012-11-19 14:55:02 PST

Comment on attachment 175042 [details] Patch Clearing flags on attachment: 175042 Committed r135200: <http://trac.webkit.org/changeset/135200>

WebKit Review Bot

Comment 10 2012-11-19 14:55:06 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.