107359 2013-01-18 21:46:56 -0800 [Mac] Use pageScaleFactor * deviceScaleFactor in requiresTiledLayer() and computePixelAlignment() of GraphicsLayerCA. 2013-05-15 12:12:25 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED INVALID P2 Normal --- 1 dongseong.hwang dongseong.hwang cmarrin commit-queue rniwa simon.fraser webkit.review.bot oldest_to_newest 811299 0 dongseong.hwang 2013-01-18 21:46:56 -0800 Currently GraphicsLayerCA uses pageScaleFactor in requiresTiledLayer() and computePixelAlignment(), but we must use pageScaleFactor * deviceScaleFactor in them. It is because: 1. requiresTiledLayer() uses the scale to compute an actual layer size in the device pixel unit. 2. computePixelAlignment() uses the scale to compute an aligned layer position in the device pixel unit. 811302 1 183603 dongseong.hwang 2013-01-18 21:53:03 -0800 Created attachment 183603 Patch 888645 2 183603 commit-queue 2013-05-13 19:10:39 -0700 Comment on attachment 183603 Patch Clearing flags on attachment: 183603 Committed r150047: <http://trac.webkit.org/changeset/150047> 888646 3 commit-queue 2013-05-13 19:10:41 -0700 All reviewed patches have been landed. Closing bug. 888707 4 183603 simon.fraser 2013-05-13 23:16:19 -0700 Comment on attachment 183603 Patch This sis wrong. We used to do this, and it causes pages to have different behavior on Retina and non-retina devices, which is undesirable. We’re not required to fall into tiled layers for larger surfaces; it’s just a memory optimization. Please revert the change. 888724 5 dongseong.hwang 2013-05-14 01:16:32 -0700 (In reply to comment #4) > (From update of attachment 183603 [details]) > This sis wrong. We used to do this, and it causes pages to have different behavior on Retina and non-retina devices, which is undesirable. We’re not required to fall into tiled layers for larger surfaces; it’s just a memory optimization. > > Please revert the change. Thank you for explanation. Can I ask a question?, because I don't fully catch up yet. In my understand of your explanation, it means that On Ratina, WebKit draws content on the same sized backing store and scales 2 times on the device. is it right? Let me say that the device scale factor on Ratina is 2. My curiosity is as follow: First of all, below code also causes different behavior on Retina and non-retina devices. afaik, layer's backing store on Ratina will be 2 times bigger than it on non-ratina, because contentsScale is 2 times bigger on Ratina. void GraphicsLayerCA::updateContentsScale(float pageScaleFactor) { float contentsScale = clampedContentsScaleForScale(pageScaleFactor * deviceScaleFactor()); m_layer->setContentsScale(contentsScale); if (drawsContent()) m_layer->setNeedsDisplay(); } In addition, backing store on Ratina should be two times bigger to draw text clearly. Even after rethinking, imo, my patch seems to be correct.. 888919 6 simon.fraser 2013-05-14 10:41:23 -0700 (In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 183603 [details] [details]) > > This sis wrong. We used to do this, and it causes pages to have different behavior on Retina and non-retina devices, which is undesirable. We’re not required to fall into tiled layers for larger surfaces; it’s just a memory optimization. > > > > Please revert the change. > > Thank you for explanation. Can I ask a question?, because I don't fully catch up yet. > > In my understand of your explanation, it means that On Ratina, WebKit draws content on the same sized backing store and scales 2 times on the device. is it right? Correct. > Let me say that the device scale factor on Ratina is 2. > > My curiosity is as follow: > > First of all, below code also causes different behavior on Retina and non-retina devices. afaik, layer's backing store on Ratina will be 2 times bigger than it on non-ratina, because contentsScale is 2 times bigger on Ratina. Actually 4x in memory use. > void GraphicsLayerCA::updateContentsScale(float pageScaleFactor) > { > float contentsScale = clampedContentsScaleForScale(pageScaleFactor * deviceScaleFactor()); Right, this is clamping based on the estimated memory use for the layer. Actually that could be pageScaleFactor * deviceScaleFactor() * deviceScaleFactor(). > m_layer->setContentsScale(contentsScale); > if (drawsContent()) > m_layer->setNeedsDisplay(); > } > > In addition, backing store on Ratina should be two times bigger to draw text clearly. > > Even after rethinking, imo, my patch seems to be correct.. It is theoretically correct, but introduces unwanted behavior. I want the layer size threshold at which requiresTiledLayer() returns true to be the same for retina and non-retina devices. That does mean that we allow layer memory use on retina to be larger before we fall into tiled layers, but we're willing to live with that. I'm curious about what device you care about that's using GraphicsLayerCA? 888934 7 dongseong.hwang 2013-05-14 10:58:12 -0700 (In reply to comment #6) > It is theoretically correct, but introduces unwanted behavior. I want the layer size threshold at which requiresTiledLayer() returns true to be the same for retina and non-retina devices. That does mean that we allow layer memory use on retina to be larger before we fall into tiled layers, but we're willing to live with that. I see. I'm ok to roll back this patch. I can not do it because I don't have committer permission. > I'm curious about what device you care about that's using GraphicsLayerCA? Nothing. On this January, when I read GraphicsLayerCA to understand original GraphicsLayer implementation, I just found the point to conflict with what I understood. I'm sorry for noise. 889436 8 simon.fraser 2013-05-15 12:12:25 -0700 Reverted in https://trac.webkit.org/r150135 183603 2013-01-18 21:53:03 -0800 2013-05-13 23:16:19 -0700 Patch bug-107359-20130119144952.patch text/plain 4035 dongseong.hwang U3VidmVyc2lvbiBSZXZpc2lvbjogMTQwMjMxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZWMxYzYxMzZiYmY0NzJl ZTMzYzY1YTcxZjA5ZTk1NDRlM2EyMTg1Yy4uNjRkOTI0ZjI4M2NkMWVkNGQ5YjM1Y2Y5ODkxMjU3 YWNkMDcwNmQzZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDI5IEBACiAyMDEzLTAxLTE4ICBIdWFu ZyBEb25nc3VuZyAgPGx1eHRlbGxhQGNvbXBhbnkxMDAubmV0PgogCisgICAgICAgIFtNYWNdIFVz ZSBwYWdlU2NhbGVGYWN0b3IgKiBkZXZpY2VTY2FsZUZhY3RvciBpbiByZXF1aXJlc1RpbGVkTGF5 ZXIoKSBhbmQgY29tcHV0ZVBpeGVsQWxpZ25tZW50KCkgb2YgR3JhcGhpY3NMYXllckNBLgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTA3MzU5CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQ3VycmVudGx5IEdy YXBoaWNzTGF5ZXJDQSB1c2VzIHBhZ2VTY2FsZUZhY3RvciBpbiByZXF1aXJlc1RpbGVkTGF5ZXIo KSBhbmQKKyAgICAgICAgY29tcHV0ZVBpeGVsQWxpZ25tZW50KCksIGJ1dCB3ZSBtdXN0IHVzZSBw YWdlU2NhbGVGYWN0b3IgKiBkZXZpY2VTY2FsZUZhY3RvciBpbgorICAgICAgICB0aGVtLgorICAg ICAgICBJdCBpcyBiZWNhdXNlOgorICAgICAgICAxLiByZXF1aXJlc1RpbGVkTGF5ZXIoKSB1c2Vz IHRoZSBzY2FsZSB0byBjb21wdXRlIGFuIGFjdHVhbCBsYXllciBzaXplIGluIHRoZSBkZXZpY2UK KyAgICAgICAgcGl4ZWwgdW5pdC4KKyAgICAgICAgMi4gY29tcHV0ZVBpeGVsQWxpZ25tZW50KCkg dXNlcyB0aGUgc2NhbGUgdG8gY29tcHV0ZSBhbiBhbGlnbmVkIGxheWVyIHBvc2l0aW9uCisgICAg ICAgIGluIHRoZSBkZXZpY2UgcGl4ZWwgdW5pdC4KKworICAgICAgICBObyBuZXcgdGVzdHMuIFdl IGNhbiBub3QgdGVzdCBhYm91dCByZXF1aXJlc1RpbGVkTGF5ZXIoKSBiZWNhdXNlIGl0IGRlcGVu ZHMgb24KKyAgICAgICAgZ3B1LiBjb21wdXRlUGl4ZWxBbGlnbm1lbnQoKSBpcyBjb3JyZWN0IG5v dyBiZWNhdXNlIGZvcnR1bmF0ZWx5IE1hYyB1c2VzCisgICAgICAgIG9ubHkgMiAoZm9yIHJldGlu YSBkaXNwbGF5KSBhcyBhIGRldmljZVNjYWxlRmFjdG9yLgorCisgICAgICAgICogcGxhdGZvcm0v Z3JhcGhpY3MvY2EvR3JhcGhpY3NMYXllckNBLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkdyYXBo aWNzTGF5ZXJDQTo6cmVxdWlyZXNUaWxlZExheWVyKToKKyAgICAgICAgKFdlYkNvcmU6OkdyYXBo aWNzTGF5ZXJDQTo6Y29tcHV0ZVBpeGVsQWxpZ25tZW50KToKKworMjAxMy0wMS0xOCAgSHVhbmcg RG9uZ3N1bmcgIDxsdXh0ZWxsYUBjb21wYW55MTAwLm5ldD4KKwogICAgICAgICBbTWFjXSBSZW1v dmUgdW51c2VkIHBhZ2VTY2FsZUZhY3RvciBhcmd1bWVudHMgaW4gR3JhcGhpY3NMYXllckNBLgog ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTA3MzU3CiAK ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2NhL0dyYXBoaWNz TGF5ZXJDQS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jYS9HcmFwaGlj c0xheWVyQ0EuY3BwCmluZGV4IGU1ZGQ4M2Q0YWY3N2Q5MjM1MTdiNDQzZTliNmJiM2Q2YmQyZmI5 N2QuLjZiZjJmMTQzOTE2NzJmYmEzYzljM2M4MWVlNjZjZDBmZTg2ZGFkYjQgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2NhL0dyYXBoaWNzTGF5ZXJDQS5jcHAK KysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2EvR3JhcGhpY3NMYXllckNB LmNwcApAQCAtMjU5OSw4ICsyNTk5LDEwIEBAIGJvb2wgR3JhcGhpY3NMYXllckNBOjpyZXF1aXJl c1RpbGVkTGF5ZXIoZmxvYXQgcGFnZVNjYWxlRmFjdG9yKSBjb25zdAogICAgIGlmICghbV9kcmF3 c0NvbnRlbnQgfHwgIW1fYWxsb3dUaWxlZExheWVyIHx8IG1faXNQYWdlVGlsZUNhY2hlTGF5ZXIp CiAgICAgICAgIHJldHVybiBmYWxzZTsKIAorICAgIGZsb2F0IGVmZmVjdGl2ZVNjYWxlID0gcGFn ZVNjYWxlRmFjdG9yICogZGV2aWNlU2NhbGVGYWN0b3IoKTsKKwogICAgIC8vIEZJWE1FOiBjYXRj aCB6ZXJvLXNpemUgaGVpZ2h0IG9yIHdpZHRoIGhlcmUgKG9yIGVhcmxpZXIpPwotICAgIHJldHVy biBtX3NpemUud2lkdGgoKSAqIHBhZ2VTY2FsZUZhY3RvciA+IGNNYXhQaXhlbERpbWVuc2lvbiB8 fCBtX3NpemUuaGVpZ2h0KCkgKiBwYWdlU2NhbGVGYWN0b3IgPiBjTWF4UGl4ZWxEaW1lbnNpb247 CisgICAgcmV0dXJuIG1fc2l6ZS53aWR0aCgpICogZWZmZWN0aXZlU2NhbGUgPiBjTWF4UGl4ZWxE aW1lbnNpb24gfHwgbV9zaXplLmhlaWdodCgpICogZWZmZWN0aXZlU2NhbGUgPiBjTWF4UGl4ZWxE aW1lbnNpb247CiB9CiAKIHZvaWQgR3JhcGhpY3NMYXllckNBOjpzd2FwRnJvbU9yVG9UaWxlZExh eWVyKGJvb2wgdXNlVGlsZWRMYXllcikKQEAgLTI5NDUsNyArMjk0Nyw4IEBAIHN0YXRpYyBpbmxp bmUgYm9vbCBpc0ludGVncmFsKGZsb2F0IHZhbHVlKQogdm9pZCBHcmFwaGljc0xheWVyQ0E6OmNv bXB1dGVQaXhlbEFsaWdubWVudChmbG9hdCBwYWdlU2NhbGVGYWN0b3IsIGNvbnN0IEZsb2F0UG9p bnQmIHBvc2l0aW9uUmVsYXRpdmVUb0Jhc2UsCiAgICAgRmxvYXRQb2ludCYgcG9zaXRpb24sIEZs b2F0U2l6ZSYgc2l6ZSwgRmxvYXRQb2ludDNEJiBhbmNob3JQb2ludCwgRmxvYXRTaXplJiBhbGln bm1lbnRPZmZzZXQpIGNvbnN0CiB7Ci0gICAgaWYgKCFtX21haW50YWluc1BpeGVsQWxpZ25tZW50 IHx8IGlzSW50ZWdyYWwocGFnZVNjYWxlRmFjdG9yKSB8fCAhbV9kcmF3c0NvbnRlbnQgfHwgbV9t YXNrc1RvQm91bmRzKSB7CisgICAgZmxvYXQgZWZmZWN0aXZlU2NhbGUgPSBwYWdlU2NhbGVGYWN0 b3IgKiBkZXZpY2VTY2FsZUZhY3RvcigpOworICAgIGlmICghbV9tYWludGFpbnNQaXhlbEFsaWdu bWVudCB8fCBpc0ludGVncmFsKGVmZmVjdGl2ZVNjYWxlKSB8fCAhbV9kcmF3c0NvbnRlbnQgfHwg bV9tYXNrc1RvQm91bmRzKSB7CiAgICAgICAgIHBvc2l0aW9uID0gbV9wb3NpdGlvbjsKICAgICAg ICAgc2l6ZSA9IG1fc2l6ZTsKICAgICAgICAgYW5jaG9yUG9pbnQgPSBtX2FuY2hvclBvaW50OwpA QCAtMjk1NiwxMiArMjk1OSwxMiBAQCB2b2lkIEdyYXBoaWNzTGF5ZXJDQTo6Y29tcHV0ZVBpeGVs QWxpZ25tZW50KGZsb2F0IHBhZ2VTY2FsZUZhY3RvciwgY29uc3QgRmxvYXRQbwogICAgIEZsb2F0 UmVjdCBiYXNlUmVsYXRpdmVCb3VuZHMocG9zaXRpb25SZWxhdGl2ZVRvQmFzZSwgbV9zaXplKTsK ICAgICBGbG9hdFJlY3Qgc2NhbGVkQm91bmRzID0gYmFzZVJlbGF0aXZlQm91bmRzOwogICAgIC8v IFNjYWxlIGJ5IHRoZSBwYWdlIHNjYWxlIGZhY3RvciB0byBjb21wdXRlIHRoZSBzY3JlZW4tcmVs YXRpdmUgYm91bmRzLgotICAgIHNjYWxlZEJvdW5kcy5zY2FsZShwYWdlU2NhbGVGYWN0b3IpOwor ICAgIHNjYWxlZEJvdW5kcy5zY2FsZShlZmZlY3RpdmVTY2FsZSk7CiAgICAgLy8gUm91bmQgdG8g aW50ZWdlciBib3VuZGFyaWVzLgogICAgIEZsb2F0UmVjdCBhbGlnbmVkQm91bmRzID0gZW5jbG9z aW5nSW50UmVjdChzY2FsZWRCb3VuZHMpOwogICAgIAogICAgIC8vIENvbnZlcnQgYmFjayB0byBs YXllciBjb29yZGluYXRlcy4KLSAgICBhbGlnbmVkQm91bmRzLnNjYWxlKDEgLyBwYWdlU2NhbGVG YWN0b3IpOworICAgIGFsaWduZWRCb3VuZHMuc2NhbGUoMSAvIGVmZmVjdGl2ZVNjYWxlKTsKICAg ICAKICAgICAvLyBFcHNpbG9uIGlzIG5lY2Vzc2FyeSB0byBlbnN1cmUgdGhhdCBiYWNraW5nIHN0 b3JlIHNpemUgY29tcHV0YXRpb24gaW4gQ0EsIHdoaWNoIGludm9sdmVzIGludGVnZXIgdHJ1bmNh dGlvbiwKICAgICAvLyB3aWxsIG1hdGNoIG91ciBhbGlnbmVkIGJvdW5kcy4K