[Webkit-unassigned] [Bug 108899] Coordinated Graphics : Refactor GraphicsSurface and ShareableBitmap to be responsible for their GraphicsContext

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=108899





--- Comment #18 from Jae Hyun Park <jae.park at company100.net>  2013-02-18 17:53:02 PST ---
(In reply to comment #17)
> (From update of attachment 188836 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188836&action=review
> 
> > Source/WebCore/platform/graphics/surfaces/GraphicsSurface.cpp:52
> > +    m_graphicsContexts.append(createGraphicsContext(rect.size(), bits, stride));
> 
> It seems wrong to be able to create more than one GraphicsContext for a given surface. What happens when painting is interleaved from the two contexts?
> 
> > Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h:88
> > +    GraphicsContext* beginPaint(const IntRect&, LockOptions);
> > +    void endPaint();
> 
> Why not just create a GraphicsContext* when first asked, then keep it around? Forcing clients to call endPaint() is a bit burdensome.

Forcing clients to call endPaint() triggers platformUnlock() to be called. Since we platformLock() at beginPaint(), we should call platformUnlock(). So endPaint() must be called each time beginPaint() is called.

> 
> > Source/WebKit2/Shared/ShareableBitmap.h:173
> > +    Vector<OwnPtr<WebCore::GraphicsContext> > m_graphicsContexts;
> 
> Same comment here. Why allow more than one context?

I was also surprised that GraphicsContext is created more than once for a given surface. But in CoordinatedTile::updateBackBuffer, we first call m_client->beginContentUpdate(), which will create a GraphicsContext, and then m_tiledBackingStore->client()->tiledBackingStorePaint() will call CoordinatedTile::updateBackbuffer after a long series of function calls, which will then call another m_client->beginContentUpdate() that creates another GraphicsContext.

The below is the stack trace for above behavior.

ASSERTION FAILED: m_graphicsContexts.size() == 1
/home/jaepark/workspace/WebKitEfl/Source/WebKit2/Shared/ShareableBitmap.cpp(174) : WebCore::GraphicsContext* WebKit::ShareableBitmap::beginPaint()
1   0x7f6528304b0b WebKit::ShareableBitmap::beginPaint()
2   0x7f652834af92 WebKit::WebCoordinatedSurface::beginPaint(WebCore::IntRect const&)
3   0x7f652416c520 WebCore::UpdateAtlas::beginPaintingOnAvailableBuffer(unsigned int&, WebCore::IntSize const&, WebCore::IntPoint&)
4   0x7f6528527a66 WebKit::CoordinatedLayerTreeHost::beginContentUpdate(WebCore::IntSize const&, unsigned int, unsigned int&, WebCore::IntPoint&)
5   0x7f65241542f7 WebCore::CoordinatedGraphicsLayer::beginContentUpdate(WebCore::IntSize const&, unsigned int&, WebCore::IntPoint&)
6   0x7f652416b835 WebCore::CoordinatedTile::updateBackBuffer()
7   0x7f6524126521 WebCore::TiledBackingStore::updateTileBuffers()
8   0x7f6524127483 WebCore::TiledBackingStore::createTiles()
9   0x7f6524126170 WebCore::TiledBackingStore::coverWithTilesIfNeeded()
10  0x7f6524126aab WebCore::TiledBackingStore::commitScaleChange()
11  0x7f6524126a5a WebCore::TiledBackingStore::setContentsScale(float)
12  0x7f6524153c23 WebCore::CoordinatedGraphicsLayer::createBackingStore()
13  0x7f65241547d6 WebCore::CoordinatedGraphicsLayer::updateContentBuffers()
14  0x7f652415370b WebCore::CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly()
15  0x7f6524152b1c WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&)
16  0x7f6524152b5d WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&)
17  0x7f6524152b5d WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&)
18  0x7f6524152b5d WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&)
19  0x7f6524152b5d WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&)
20  0x7f6524352e2f WebCore::RenderLayerCompositor::flushPendingLayerChanges(bool)
21  0x7f6523fee651 WebCore::FrameView::flushCompositingStateForThisFrame(WebCore::Frame*)
22  0x7f6523ff793a WebCore::FrameView::paintContents(WebCore::GraphicsContext*, WebCore::IntRect const&)
23  0x7f6524094e67 WebCore::ScrollView::paint(WebCore::GraphicsContext*, WebCore::IntRect const&)
24  0x7f652440ef52 WebCore::RenderWidget::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
25  0x7f65243322ad WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
26  0x7f652434fce6 WebCore::RenderLayerBacking::paintIntoLayer(WebCore::GraphicsLayer const*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, unsigned int)
27  0x7f652435005f WebCore::RenderLayerBacking::paintContents(WebCore::GraphicsLayer const*, WebCore::GraphicsContext&, unsigned int, WebCore::IntRect const&)
28  0x7f65241038bd WebCore::GraphicsLayer::paintGraphicsLayerContents(WebCore::GraphicsContext&, WebCore::IntRect const&)
29  0x7f6524153c71 WebCore::CoordinatedGraphicsLayer::tiledBackingStorePaint(WebCore::GraphicsContext*, WebCore::IntRect const&)
30  0x7f652416b973 WebCore::CoordinatedTile::updateBackBuffer()
31  0x7f6524126521 WebCore::TiledBackingStore::updateTileBuffers()

I'm still not sure if this is the right direction. IMHO, the second last patch (https://bugs.webkit.org/attachment.cgi?id=187989&action=review) would be more appropriate for this refactoring. How do you feel about that?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.