72333 2011-11-14 17:06:57 -0800 [Chromium] Clear tile update rectangle properly before each update. 2011-11-17 16:31:08 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED DUPLICATE 71388 P2 Normal --- 1 reveman reveman cc-bugs enne jamesr nduca webkit.review.bot oldest_to_newest 502497 0 reveman 2011-11-14 17:06:57 -0800 UpdatableTile::m_updateRect needs to be cleared for each tile before starting a new update. TiledLayerChromium::updateCompositorResources should only use m_updateRect to determine the tiles that need updating. 502509 1 115068 reveman 2011-11-14 17:13:33 -0800 Created attachment 115068 Patch 502518 2 115068 jamesr 2011-11-14 17:23:42 -0800 Comment on attachment 115068 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115068&action=review > Source/WebCore/ChangeLog:13 > + No new tests. Covered by the existing tests. which tests cover this change? 502529 3 reveman 2011-11-14 17:28:17 -0800 (In reply to comment #2) > (From update of attachment 115068 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115068&action=review > > > Source/WebCore/ChangeLog:13 > > + No new tests. Covered by the existing tests. > > which tests cover this change? I'm working on layout test that will expose this problem. This shouldn't have been marked for review yet. Sorry. 502533 4 jamesr 2011-11-14 17:32:33 -0800 OK that sounds great, just checking :) 502542 5 115068 enne 2011-11-14 17:38:20 -0800 Comment on attachment 115068 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115068&action=review Sorry for all the questions, but can you explain the changes to clearDirty() and createTile() as well? I don't quite follow why those need to get moved around. I think if you expect tiles to be there, then there should be asserts about it. > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:325 > + for (CCLayerTilingData::TileMap::const_iterator iter = m_tiler->tiles().begin(); iter != m_tiler->tiles().end(); ++iter) { > + UpdatableTile* tile = static_cast<UpdatableTile*>(iter->second.get()); > + tile->m_updateRect = IntRect(); > + } > + Why not just clear the update rect in updateCompositorResources as you iterate through the tiles with non-zero update rects, rather than every tile on the layer? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:382 > - if (!tile) > - tile = createTile(i, j); > - else if (!tile->dirty()) > + if (!tile || !tile->dirty()) How does this not break the default texture updater? If you don't create new tiles and consider them dirty, then how does m_paintRect get constructed to include new tiles? 502545 6 nduca 2011-11-14 17:43:02 -0800 (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 115068 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=115068&action=review > > > > > Source/WebCore/ChangeLog:13 > > > + No new tests. Covered by the existing tests. > > > > which tests cover this change? > > I'm working on layout test that will expose this problem. Is this unit testable? 502584 7 reveman 2011-11-14 18:55:04 -0800 (In reply to comment #5) > (From update of attachment 115068 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115068&action=review > > Sorry for all the questions, but can you explain the changes to clearDirty() and createTile() as well? I don't quite follow why those need to get moved around. I think if you expect tiles to be there, then there should be asserts about it. The idea is that we compute the paint area and the tile areas to update in prepareToUpdate. The result ends up in m_paintRect and UpdatableTile::m_updateRect. Once this is done, tiles can be invalidated without it affecting the behavior of the next call to updateCompositorResource. This is the reason for the move of clearDiry(). I intentionally didn't change the expectation of a tile's existence in this patch, though it's probably safe to do so. However, creating a tile when it doesn't exist is wrong in these cases which is why I included the changes related to that in this patch. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:325 > > + for (CCLayerTilingData::TileMap::const_iterator iter = m_tiler->tiles().begin(); iter != m_tiler->tiles().end(); ++iter) { > > + UpdatableTile* tile = static_cast<UpdatableTile*>(iter->second.get()); > > + tile->m_updateRect = IntRect(); > > + } > > + > > Why not just clear the update rect in updateCompositorResources as you iterate through the tiles with non-zero update rects, rather than every tile on the layer? That works too. I was a bit forward looking here and changed it in a way that will work with my patch that allows initialization of compositor resources prior to painting. That patch turns updateCompositorResources into forEachCompositorResource and it's not necessarily called just once during an updated. I can move the clear into updateCompositorResources for now if that's preferred? > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:382 > > - if (!tile) > > - tile = createTile(i, j); > > - else if (!tile->dirty()) > > + if (!tile || !tile->dirty()) > > How does this not break the default texture updater? If you don't create new tiles and consider them dirty, then how does m_paintRect get constructed to include new tiles? This is the second pass iterating over all tiles that might need updating. The first pass that create tiles if necessary is before the textureUpdater()->prepareToUpdate() call. Is there a chance that more tiles are dirty after the call to textureUpdater()->prepareToUpdate()? In that case we could allow the creation of them in the second pass but we can't call clearDirty() on them as m_paintRect might not include all of the dirty tile rect. In that case we need to subtract the m_paintRect from the tile's dirty rect. The patch is not correct here. We need to either have an ASSERT(tile) or create tiles and subtract m_paintRect from the tile's dirty rect. 502634 8 115068 enne 2011-11-14 22:14:15 -0800 Comment on attachment 115068 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115068&action=review (In reply to comment #7) > (In reply to comment #5) > > (From update of attachment 115068 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=115068&action=review > > > > Sorry for all the questions, but can you explain the changes to clearDirty() and createTile() as well? I don't quite follow why those need to get moved around. I think if you expect tiles to be there, then there should be asserts about it. > > The idea is that we compute the paint area and the tile areas to update in prepareToUpdate. The result ends up in m_paintRect and UpdatableTile::m_updateRect. Once this is done, tiles can be invalidated without it affecting the behavior of the next call to updateCompositorResource. This is the reason for the move of clearDiry(). It's my assumption that there'd be no invalidations once prepareToUpdate was called until after updateCompositorResources, so was wondering if there was a correctness issue you were addressing that I didn't quite understand. > I intentionally didn't change the expectation of a tile's existence in this patch, though it's probably safe to do so. However, creating a tile when it doesn't exist is wrong in these cases which is why I included the changes related to that in this patch. Oh, I see it now. Because you're setting more state on these tiles, lazy creation will cause problems. >>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:325 >>> + >> >> Why not just clear the update rect in updateCompositorResources as you iterate through the tiles with non-zero update rects, rather than every tile on the layer? > > That works too. I was a bit forward looking here and changed it in a way that will work with my patch that allows initialization of compositor resources prior to painting. That patch turns updateCompositorResources into forEachCompositorResource and it's not necessarily called just once during an updated. I can move the clear into updateCompositorResources for now if that's preferred? No, not at all. "Why" was just an honest question here. If this meshes better with where you're going, then by all means. :) >>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:382 >>> + if (!tile || !tile->dirty()) >> >> How does this not break the default texture updater? If you don't create new tiles and consider them dirty, then how does m_paintRect get constructed to include new tiles? > > This is the second pass iterating over all tiles that might need updating. The first pass that create tiles if necessary is before the textureUpdater()->prepareToUpdate() call. Is there a chance that more tiles are dirty after the call to textureUpdater()->prepareToUpdate()? In that case we could allow the creation of them in the second pass but we can't call clearDirty() on them as m_paintRect might not include all of the dirty tile rect. In that case we need to subtract the m_paintRect from the tile's dirty rect. > > The patch is not correct here. We need to either have an ASSERT(tile) or create tiles and subtract m_paintRect from the tile's dirty rect. Oops. I entirely misread this diff, then. Ignore my entirely incorrect worry about this breaking things. Yeah, an assert in both of these places where it's not ok to lazily create tiles would be excellent. For what it's worth, perhaps this should be better documented, but there should only be one case in which m_paintRect does not include all of the dirty tile rect: edge tiles on image layers. It's a hack to just consider those tiles clean once they've been partially uploaded, but the layer is entirely invalidated when its bounds change. That's why clearDirty() is called anyway in the old code, even though m_updateRect != dirtyRect. The alternative is to have more complicated per-tile dirty regions or tiles of different sizes, but I just didn't want to go there. 505343 9 reveman 2011-11-17 16:31:08 -0800 Fixed in the original patch that was reverted. *** This bug has been marked as a duplicate of bug 71388 *** 115068 2011-11-14 17:13:33 -0800 2011-11-14 22:14:15 -0800 Patch bug-72333-20111114171332.patch text/plain 3524 reveman U3VidmVyc2lvbiBSZXZpc2lvbjogMTAwMTk5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZTdiOWRhZTM4MDYwY2Yx YmJhN2ZhMTI1YzU4MDIxNTdiZDM1MDdjMS4uMTY0OWUxOTc4NjcxZTczMTQ1NWZkZWY1YzlhZTVk NWJiNTJiMTdmOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDExLTExLTE0ICBEYXZp ZCBSZXZlbWFuICA8cmV2ZW1hbkBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgW0Nocm9taXVtXSBD bGVhciB0aWxlIHVwZGF0ZSByZWN0YW5nbGUgcHJvcGVybHkgYmVmb3JlIGVhY2ggdXBkYXRlLgor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzIzMzMKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBVcGRhdGFibGVU aWxlOjptX3VwZGF0ZVJlY3QgbmVlZHMgdG8gYmUgY2xlYXJlZCBmb3IgZWFjaCB0aWxlCisgICAg ICAgIGJlZm9yZSBzdGFydGluZyBhIG5ldyB1cGRhdGUuCisgICAgICAgIFRpbGVkTGF5ZXJDaHJv bWl1bTo6dXBkYXRlQ29tcG9zaXRvclJlc291cmNlcyBzaG91bGQgb25seSB1c2UKKyAgICAgICAg bV91cGRhdGVSZWN0IHRvIGRldGVybWluZSB0aGUgdGlsZXMgdGhhdCBuZWVkIHVwZGF0aW5nLgor CisgICAgICAgIE5vIG5ldyB0ZXN0cy4gQ292ZXJlZCBieSB0aGUgZXhpc3RpbmcgdGVzdHMuCisK KyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9UaWxlZExheWVyQ2hyb21pdW0u Y3BwOgorICAgICAgICAoV2ViQ29yZTo6VGlsZWRMYXllckNocm9taXVtOjp1cGRhdGVDb21wb3Np dG9yUmVzb3VyY2VzKToKKyAgICAgICAgKFdlYkNvcmU6OlRpbGVkTGF5ZXJDaHJvbWl1bTo6cHJl cGFyZVRvVXBkYXRlKToKKwogMjAxMS0xMS0xNCAgQWRyaWVubmUgV2Fsa2VyICA8ZW5uZUBnb29n bGUuY29tPgogCiAgICAgICAgIFtjaHJvbWl1bV0gUGlwZSBjb21wb3NpdG9yIGNvbW1pdC9zd2Fw IHVwIHRvIFdlYldpZGdldENsaWVudApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZv cm0vZ3JhcGhpY3MvY2hyb21pdW0vVGlsZWRMYXllckNocm9taXVtLmNwcCBiL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL1RpbGVkTGF5ZXJDaHJvbWl1bS5jcHAKaW5k ZXggMjYwYTE3NWM0MjhjMTU5YTYzOGUwMjQ3NWE0YjVkZTQyMWQyZWE0YS4uOWM4ODRkNzQ2ZTY3 OTAxZmQzYzhiN2MxMmY2MThmMTZhMjE3ZWZiZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vVGlsZWRMYXllckNocm9taXVtLmNwcAorKysgYi9T b3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9UaWxlZExheWVyQ2hyb21p dW0uY3BwCkBAIC0xNjYsOCArMTY2LDYgQEAgdm9pZCBUaWxlZExheWVyQ2hyb21pdW06OnVwZGF0 ZUNvbXBvc2l0b3JSZXNvdXJjZXMoR3JhcGhpY3NDb250ZXh0M0QqIGNvbnRleHQsIEMKICAgICAg ICAgZm9yIChpbnQgaSA9IGxlZnQ7IGkgPD0gcmlnaHQ7ICsraSkgewogICAgICAgICAgICAgVXBk YXRhYmxlVGlsZSogdGlsZSA9IHRpbGVBdChpLCBqKTsKICAgICAgICAgICAgIGlmICghdGlsZSkK LSAgICAgICAgICAgICAgICB0aWxlID0gY3JlYXRlVGlsZShpLCBqKTsKLSAgICAgICAgICAgIGVs c2UgaWYgKCF0aWxlLT5kaXJ0eSgpKQogICAgICAgICAgICAgICAgIGNvbnRpbnVlOwogCiAgICAg ICAgICAgICBJbnRSZWN0IHNvdXJjZVJlY3QgPSB0aWxlLT5tX3VwZGF0ZVJlY3Q7CkBAIC0yMDEs NyArMTk5LDYgQEAgdm9pZCBUaWxlZExheWVyQ2hyb21pdW06OnVwZGF0ZUNvbXBvc2l0b3JSZXNv dXJjZXMoR3JhcGhpY3NDb250ZXh0M0QqIGNvbnRleHQsIEMKICAgICAgICAgICAgIEdMQyhjb250 ZXh0LCBjb250ZXh0LT50ZXhQYXJhbWV0ZXJpKEdyYXBoaWNzQ29udGV4dDNEOjpURVhUVVJFXzJE LCBHcmFwaGljc0NvbnRleHQzRDo6VEVYVFVSRV9NQUdfRklMVEVSLCBmaWx0ZXIpKTsKIAogICAg ICAgICAgICAgdXBkYXRlci5hcHBlbmQodGlsZS0+dGV4dHVyZSgpLCBzb3VyY2VSZWN0LCBkZXN0 UmVjdCk7Ci0gICAgICAgICAgICB0aWxlLT5jbGVhckRpcnR5KCk7CiAgICAgICAgIH0KICAgICB9 CiAKQEAgLTMyMSw2ICszMTgsMTEgQEAgdm9pZCBUaWxlZExheWVyQ2hyb21pdW06OnByZXBhcmVU b1VwZGF0ZShjb25zdCBJbnRSZWN0JiBjb250ZW50UmVjdCkKIAogICAgIG1fc2tpcHNEcmF3ID0g ZmFsc2U7CiAKKyAgICBmb3IgKENDTGF5ZXJUaWxpbmdEYXRhOjpUaWxlTWFwOjpjb25zdF9pdGVy YXRvciBpdGVyID0gbV90aWxlci0+dGlsZXMoKS5iZWdpbigpOyBpdGVyICE9IG1fdGlsZXItPnRp bGVzKCkuZW5kKCk7ICsraXRlcikgeworICAgICAgICBVcGRhdGFibGVUaWxlKiB0aWxlID0gc3Rh dGljX2Nhc3Q8VXBkYXRhYmxlVGlsZSo+KGl0ZXItPnNlY29uZC5nZXQoKSk7CisgICAgICAgIHRp bGUtPm1fdXBkYXRlUmVjdCA9IEludFJlY3QoKTsKKyAgICB9CisKICAgICBpZiAoY29udGVudFJl Y3QuaXNFbXB0eSgpKSB7CiAgICAgICAgIG1fcmVxdWVzdGVkVXBkYXRlUmVjdCA9IEludFJlY3Qo KTsKICAgICAgICAgcmV0dXJuOwpAQCAtMzc3LDkgKzM3OSw3IEBAIHZvaWQgVGlsZWRMYXllckNo cm9taXVtOjpwcmVwYXJlVG9VcGRhdGUoY29uc3QgSW50UmVjdCYgY29udGVudFJlY3QpCiAgICAg Zm9yIChpbnQgaiA9IHRvcDsgaiA8PSBib3R0b207ICsraikgewogICAgICAgICBmb3IgKGludCBp ID0gbGVmdDsgaSA8PSByaWdodDsgKytpKSB7CiAgICAgICAgICAgICBVcGRhdGFibGVUaWxlKiB0 aWxlID0gdGlsZUF0KGksIGopOwotICAgICAgICAgICAgaWYgKCF0aWxlKQotICAgICAgICAgICAg ICAgIHRpbGUgPSBjcmVhdGVUaWxlKGksIGopOwotICAgICAgICAgICAgZWxzZSBpZiAoIXRpbGUt PmRpcnR5KCkpCisgICAgICAgICAgICBpZiAoIXRpbGUgfHwgIXRpbGUtPmRpcnR5KCkpCiAgICAg ICAgICAgICAgICAgY29udGludWU7CiAKICAgICAgICAgICAgIC8vIENhbGN1bGF0ZSBjb250ZW50 LXNwYWNlIHJlY3RhbmdsZSB0byBjb3B5IGZyb20uCkBAIC0zODksNiArMzg5LDggQEAgdm9pZCBU aWxlZExheWVyQ2hyb21pdW06OnByZXBhcmVUb1VwZGF0ZShjb25zdCBJbnRSZWN0JiBjb250ZW50 UmVjdCkKICAgICAgICAgICAgIC8vIG1ha2Ugc3VyZSB0aGF0IHNvdXJjZVJlY3QgZG9lc24ndCBl eHRlbmQgb3V0c2lkZSBvZiBpdC4KICAgICAgICAgICAgIHNvdXJjZVJlY3QuaW50ZXJzZWN0KG1f cGFpbnRSZWN0KTsKIAorICAgICAgICAgICAgdGlsZS0+Y2xlYXJEaXJ0eSgpOworCiAgICAgICAg ICAgICB0aWxlLT5tX3VwZGF0ZVJlY3QgPSBzb3VyY2VSZWN0OwogICAgICAgICAgICAgaWYgKHNv dXJjZVJlY3QuaXNFbXB0eSgpKQogICAgICAgICAgICAgICAgIGNvbnRpbnVlOwo=