Bug 101701 - Coordinated Graphics: Amend CoordinatedBackingStore::paintToTextureMapper to fit its own semantic.
Summary: Coordinated Graphics: Amend CoordinatedBackingStore::paintToTextureMapper to ...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dongseong Hwang
URL:
Keywords:
Depends on: 103217
Blocks: 101705 101818
  Show dependency treegraph
 
Reported: 2012-11-08 21:01 PST by Dongseong Hwang
Modified: 2013-10-02 12:28 PDT (History)
4 users (show)

See Also:


Attachments
Patch (22.17 KB, patch)
2012-11-08 21:07 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (22.37 KB, patch)
2012-11-08 21:27 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (21.94 KB, patch)
2012-11-08 21:31 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (22.26 KB, patch)
2012-11-09 00:20 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (22.26 KB, patch)
2012-11-09 16:25 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 2012-11-08 21:01:41 PST
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.
Comment 1 Dongseong Hwang 2012-11-08 21:07:03 PST
Created attachment 173193 [details]
Patch
Comment 2 Dongseong Hwang 2012-11-08 21:15:57 PST
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.
Comment 3 Early Warning System Bot 2012-11-08 21:17:53 PST
Comment on attachment 173193 [details]
Patch

Attachment 173193 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14777280
Comment 4 EFL EWS Bot 2012-11-08 21:18:43 PST
Comment on attachment 173193 [details]
Patch

Attachment 173193 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14762978
Comment 5 Dongseong Hwang 2012-11-08 21:27:21 PST
Created attachment 173197 [details]
Patch
Comment 6 Dongseong Hwang 2012-11-08 21:31:56 PST
Created attachment 173199 [details]
Patch
Comment 7 Dongseong Hwang 2012-11-08 21:42:07 PST
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.
Comment 8 Dongseong Hwang 2012-11-08 23:54:13 PST
I found a bug. I'll fix it.
Comment 9 Dongseong Hwang 2012-11-09 00:20:11 PST
Created attachment 173224 [details]
Patch
Comment 10 Dongseong Hwang 2012-11-09 00:21:55 PST
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.
Comment 11 Noam Rosenthal 2012-11-09 07:31:35 PST
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?
Comment 12 Dongseong Hwang 2012-11-09 16:25:18 PST
Created attachment 173394 [details]
Patch
Comment 13 Dongseong Hwang 2012-11-09 16:28:27 PST
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.
Comment 14 Dongseong Hwang 2012-11-09 17:12:21 PST
> > 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.
Comment 15 WebKit Review Bot 2012-11-09 18:32:52 PST
Comment on attachment 173394 [details]
Patch

Clearing flags on attachment: 173394

Committed r134142: <http://trac.webkit.org/changeset/134142>
Comment 16 WebKit Review Bot 2012-11-09 18:32:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Noam Rosenthal 2012-11-12 14:16:49 PST
This is creating issues with pixel tests; I think it breaks tiling implicit behavior. We should attempt to fix or roll out.
Comment 18 Chris Dumez 2012-11-24 08:54:55 PST
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