Bug 101701 – Coordinated Graphics: Amend CoordinatedBackingStore::paintToTextureMapper to fit its own semantic.

• Component: Layout and Rendering
• Status: REOPENED

[Dongseong Hwang]

Currently, all subclasses of TextureMapperPlatformLayer implement
paintToTextureMapper to draw its own texture on the given targetRect. Subclasses
can scale the texture to fit the size of the targetRect. However, only
CoordinatedBackingStore::paintToTextureMapper draws its texture using its own
texture size. There is no bug yet, because TextureMapperLayer uses
CoordinatedBackingStore only as a backing store, not a content layer. So,
TextureMapperLayer always request CoordinatedBackingStore to draw using its own
texture size. However, we can use CoordinatedBackingStore as a content layer in
the future. So this patch fixes this potential bug.

[Dongseong Hwang]

Created attachment 173193 [details]
Patch

[Dongseong Hwang]

Comment on attachment 173193 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173193&action=review

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:384
> +    backingStore->setSize(backingSize);

We can know backingSize via following code.

GraphicsLayer* layer = layerByID(layerID);
backingStore->setSize(layer->size());

However, I change the CreateTile message containing a backingSize.
There are two reasons.
1. It is not strange. Currently the CreateTile message plays two roles: createTile and createTiledBackingStore.
We piggyback creating backing on the CreateTile message. If a createTiledBackingStore message exists, it is not strange containing a backingSize.

2. Currently, layer->size() equals the backingSize logically, but it is fragile. Moreover, it heavily depends on CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly() implementation detail.
 CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly() calls syncLayerState() before updateContentBuffers(). If the order is reversed, we can not get the latest layer->size() at this time.

[Early Warning System Bot]

Comment on attachment 173193 [details]
Patch

Attachment 173193 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14777280

[EFL EWS Bot]

Comment on attachment 173193 [details]
Patch

Attachment 173193 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14762978

[Dongseong Hwang]

Created attachment 173197 [details]
Patch

[Dongseong Hwang]

Created attachment 173199 [details]
Patch

[Dongseong Hwang]

Comment on attachment 173199 [details]
Patch

In this patch, I fixes build fail. And one more thing.

View in context: https://bugs.webkit.org/attachment.cgi?id=173199&action=review

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:170
> +    adjustedTransform.multiply(TransformationMatrix::rectToRect(rectOnContents, targetRect));

This patch change here.

Currently, understanding CoordinatedBackingStore is not easy because of m_scale. Each tile has m_rect on the contents coordinate system, so its texture size is not m_rect.size().
In summary, everything is fine if we always use the contents coordinate system with arguments and return values.

[Dongseong Hwang]

I found a bug. I'll fix it.

[Dongseong Hwang]

Created attachment 173224 [details]
Patch

[Dongseong Hwang]

Comment on attachment 173224 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173224&action=review

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedTile.cpp:91
> +        m_client->createTile(m_ID, updateInfo, m_rect, m_tiledBackingStore->rect().size());

The previous patch used tile's rect instead of tiledBackingStore's rect. I made a mistake.

[Noam Rosenthal]

Comment on attachment 173224 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173224&action=review

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:130
> +        tile->paint(textureMapper, transform, opacity, mask, edgesExposed);

Since we now have the entire surfaceRect, we should be able to antialias the edge-tiles. But we can do this at a different patch. Can you add a FIXME?

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:174
> +    IntRect rectOnContents = mapToContents(IntRect(IntPoint::zero(), m_size), m_scale);

Shouldn't this be FloatRects?

[Dongseong Hwang]

Created attachment 173394 [details]
Patch

[Dongseong Hwang]

Comment on attachment 173394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173394&action=review

Thank you for review!

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:167
> +    // FIXME: When the TextureMapper makes a distinction between some edges exposed and no edges

Changed FIXME.
We can do it because we know the entire size of the backing now.
We will do it in another bug as you mentioned :)

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:174
> +    FloatRect rectOnContents = mapToContents(IntRect(IntPoint::zero(), m_size), m_scale);

Changed FloatRect from IntRect.

[Dongseong Hwang]

> > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:167
> > +    // FIXME: When the TextureMapper makes a distinction between some edges exposed and no edges
> 
> Changed FIXME.
> We can do it because we know the entire size of the backing now.
> We will do it in another bug as you mentioned :)

I posted the patch to fix in Bug 101818.

[WebKit Review Bot]

Comment on attachment 173394 [details]
Patch

Clearing flags on attachment: 173394

Committed r134142: <http://trac.webkit.org/changeset/134142>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Noam Rosenthal]

This is creating issues with pixel tests; I think it breaks tiling implicit behavior. We should attempt to fix or roll out.

[Chris Dumez]

If you open the following URL:
http://www.robohornet.org

8 times out of 10, you will hit an ASSERT introduced in this patch:
ASSERTION FAILED: !m_size.isZero()
/home/chris/unencrypted/WebKit/Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp(170) : virtual void WebKit::CoordinatedBackingStore::paintToTextureMapper(WebCore::TextureMapper*, const WebCore::FloatRect&, const WebCore::TransformationMatrix&, float, WebCore::BitmapTexture*)
1   0x7ff4b8d52473 WebKit::CoordinatedBackingStore::paintToTextureMapper(WebCore::TextureMapper*, WebCore::FloatRect const&, WebCore::TransformationMatrix const&, float, WebCore::BitmapTexture*)
2   0x7ff4b50accea WebCore::TextureMapperLayer::paintSelf(WebCore::TextureMapperPaintOptions const&)
3   0x7ff4b50acddb WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions const&)
4   0x7ff4b50ad71d WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica(WebCore::TextureMapperPaintOptions const&)
5   0x7ff4b50ad84f WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions const&)
6   0x7ff4b50acf47 WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions const&)
7   0x7ff4b50ad71d WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica(WebCore::TextureMapperPaintOptions const&)
8   0x7ff4b50ad84f WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions const&)
9   0x7ff4b50acf47 WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions const&)
10  0x7ff4b50ad71d WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica(WebCore::TextureMapperPaintOptions const&)
11  0x7ff4b50ad84f WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions const&)
12  0x7ff4b50acf47 WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions const&)
13  0x7ff4b50ad71d WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica(WebCore::TextureMapperPaintOptions const&)
14  0x7ff4b50ad84f WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions const&)
15  0x7ff4b50acf47 WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions const&)
16  0x7ff4b50ad71d WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica(WebCore::TextureMapperPaintOptions const&)
17  0x7ff4b50ad84f WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions const&)
18  0x7ff4b50acf47 WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions const&)
19  0x7ff4b50ad71d WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica(WebCore::TextureMapperPaintOptions const&)
20  0x7ff4b50ad84f WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions const&)
21  0x7ff4b50ac8b9 WebCore::TextureMapperLayer::paint()
22  0x7ff4b8d60455 WebKit::LayerTreeRenderer::paintToCurrentGLContext(WebCore::TransformationMatrix const&, float, WebCore::FloatRect const&, unsigned int)
23  0x7ff4b8e8e572 EwkViewImpl::displayTimerFired(WebCore::Timer<EwkViewImpl>*)
24  0x7ff4b8e95ae8 WebCore::Timer<EwkViewImpl>::fired()
25  0x7ff4b5021fea WebCore::ThreadTimers::sharedTimerFiredInternal()
26  0x7ff4b5021f0b WebCore::ThreadTimers::sharedTimerFired()
27  0x7ff4b5a32175
28  0x7ff4b94fb46e _ecore_timer_expired_call
29  0x7ff4b94fb63b _ecore_timer_expired_timers_call
30  0x7ff4b94f8551
31  0x7ff4b94f8be7 ecore_main_loop_begin