Bug 101701

Summary:

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

Product:

WebKit

Reporter:

Dongseong Hwang <dongseong.hwang>

Component:

Layout and Rendering

Assignee:

Dongseong Hwang <dongseong.hwang>

Status:

REOPENED

Severity:

Normal

CC:

cdumez, noam, tmpsantos, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

103217

Bug Blocks:

101705, 101818

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Dongseong Hwang

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

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Dongseong Hwang

Comment 1 2012-11-08 21:07:03 PST

Created attachment 173193 [details] Patch

Dongseong Hwang

Comment 2 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.

Early Warning System Bot

Comment 3 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

EFL EWS Bot

Comment 4 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

Dongseong Hwang

Comment 5 2012-11-08 21:27:21 PST

Created attachment 173197 [details] Patch

Dongseong Hwang

Comment 6 2012-11-08 21:31:56 PST

Created attachment 173199 [details] Patch

Dongseong Hwang

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

Dongseong Hwang

Comment 8 2012-11-08 23:54:13 PST

I found a bug. I'll fix it.

Dongseong Hwang

Comment 9 2012-11-09 00:20:11 PST

Created attachment 173224 [details] Patch

Dongseong Hwang

Comment 10 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.

Noam Rosenthal

Comment 11 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?

Dongseong Hwang

Comment 12 2012-11-09 16:25:18 PST

Created attachment 173394 [details] Patch

Dongseong Hwang

Comment 13 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.

Dongseong Hwang

Comment 14 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.

WebKit Review Bot

Comment 15 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>

WebKit Review Bot

Comment 16 2012-11-09 18:32:56 PST

All reviewed patches have been landed. Closing bug.

Noam Rosenthal

Comment 17 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.

Chris Dumez

Comment 18 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

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