66506 2011-08-18 15:49:04 -0700 [chromium] Root layer is not updated when only a portion of tile is updated 2011-08-24 13:18:02 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 alokp alokp jamesr vangelis webkit.review.bot oldest_to_newest 453443 0 alokp 2011-08-18 15:49:04 -0700 This only happens in accelerated drawing path. 453446 1 jamesr 2011-08-18 15:51:25 -0700 http://trac.webkit.org/changeset/93360 might fix this? It make the root and non-root layer paths much more similar. 453450 2 104417 alokp 2011-08-18 15:54:52 -0700 Created attachment 104417 proposed patch 453455 3 104417 jamesr 2011-08-18 15:59:05 -0700 Comment on attachment 104417 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=104417&action=review > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:664 > + GLC(context, context->bindTexture(GraphicsContext3D::TEXTURE_2D, 0)); I'm not sure I understand why this is here - is the problem here that skia is assuming a certain texture binding, but that isn't true here? 453558 4 104417 alokp 2011-08-18 19:46:33 -0700 Comment on attachment 104417 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=104417&action=review >> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:664 >> + GLC(context, context->bindTexture(GraphicsContext3D::TEXTURE_2D, 0)); > > I'm not sure I understand why this is here - is the problem here that skia is assuming a certain texture binding, but that isn't true here? It is not needed to fix this particular bug. This bug was yet another case of state management issue between skia and compositor, so I took a pass through the accelerated code path and fixed the functions that left GL state in a non-default state. I think in general it is a good practice to restore the GL state if it is not too expensive. 454645 5 alokp 2011-08-22 09:58:53 -0700 ping! 454690 6 104417 jamesr 2011-08-22 10:51:37 -0700 Comment on attachment 104417 proposed patch The flush seems fine but I'm not really a fan of making unrelated changes that don't do anything. We have not been trying to keep all GL state on the compositor in any particular state (nor do I think that we would want to), so restoring some bits of state in an ad-hoc fashion seems to just be adding complexity for no benefit. If we do need a given GL state to be maintained on the compositor context then we should design, test, and be rigorous about it. 454775 7 104417 vangelis 2011-08-22 11:58:43 -0700 Comment on attachment 104417 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=104417&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:791 > // FIXME: These calls can be made once, when the compositor context is initialized. I think this FIXME should be removed. Now that the compositor's context is shared with non-compositor code, it will be impossible to assume that any given state will be stick across frames. 456247 8 alokp 2011-08-24 11:27:14 -0700 (In reply to comment #6) > (From update of attachment 104417 [details]) > The flush seems fine but I'm not really a fan of making unrelated changes that don't do anything. We have not been trying to keep all GL state on the compositor in any particular state (nor do I think that we would want to), so restoring some bits of state in an ad-hoc fashion seems to just be adding complexity for no benefit. If we do need a given GL state to be maintained on the compositor context then we should design, test, and be rigorous about it. That particular change is not restoring the state in any ad-hoc manner. It is just restoring it the default state. We do that in multiple places. For example LayerRendererChromium::drawLayersInternal() restores STENCIL_TEST and BLEND state. LayerTextureUpdaterSkPicture::updateTextureRect() rebinds default frame-buffer. Anyways I have removed it from this patch because it did not have any effect. We need to start thinking about state management as the compositor context is increasingly used by skia for content rendering. 456248 9 104417 alokp 2011-08-24 11:27:41 -0700 Comment on attachment 104417 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=104417&action=review >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:791 >> // FIXME: These calls can be made once, when the compositor context is initialized. > > I think this FIXME should be removed. Now that the compositor's context is shared with non-compositor code, it will be impossible to assume that any given state will be stick across frames. DONE 456253 10 jamesr 2011-08-24 11:30:21 -0700 I meant ad-hoc in the sense that there are other places where the compositor sets GL state and does not restore it. I don't know if we have decided whether skia will continue to use the compositor context or not long-term. 456261 11 105030 alokp 2011-08-24 11:38:10 -0700 Created attachment 105030 proposed patch Removed the change to restore bound texture and removed FIXME. 456319 12 105030 jamesr 2011-08-24 12:44:54 -0700 Comment on attachment 105030 proposed patch Looks good. 456357 13 105030 webkit.review.bot 2011-08-24 13:17:57 -0700 Comment on attachment 105030 proposed patch Clearing flags on attachment: 105030 Committed r93727: <http://trac.webkit.org/changeset/93727> 456358 14 webkit.review.bot 2011-08-24 13:18:02 -0700 All reviewed patches have been landed. Closing bug. 104417 2011-08-18 15:54:52 -0700 2011-08-24 11:38:10 -0700 proposed patch stencil.patch text/plain 3270 alokp SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDkzMzYyKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjIgQEAKKzIwMTEtMDgtMTggIEFsb2sgUHJp eWFkYXJzaGkgIDxhbG9rcEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgW2Nocm9taXVtXSBSb290 IGxheWVyIGlzIG5vdCB1cGRhdGVkIHdoZW4gb25seSBhIHBvcnRpb24gb2YgdGlsZSBpcyB1cGRh dGVkCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02NjUw NgorICAgICAgICAKKyAgICAgICAgVGhpcyBvbmx5IGhhcHBlbmVkIGluIHRoZSBhY2NlbGVyYXRl ZCBwYXRoIHdoaWNoIGNoYW5nZXMgc3RlbmNpbCBzdGF0ZS4KKyAgICAgICAgSW5jb3JyZWN0IHN0 ZW5jaWwgc3RhdGUgaXMgdXNlZCB0byBkcmF3IHRoZSByb290IGxheWVyLgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRlc3Q6IGNvbXBvc2l0aW5nL292 ZXJmbG93L292ZXJmbG93LXNjcm9sbC5odG1sIChleGlzdGluZykKKworICAgICAgICAqIHBsYXRm b3JtL2dyYXBoaWNzL2Nocm9taXVtL0xheWVyUmVuZGVyZXJDaHJvbWl1bS5jcHA6CisgICAgICAg IChXZWJDb3JlOjpMYXllclJlbmRlcmVyQ2hyb21pdW06OmRyYXdMYXllcnNJbnRlcm5hbCk6Cisg ICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vTGF5ZXJUZXh0dXJlVXBkYXRlckNh bnZhcy5jcHA6CisgICAgICAgIChXZWJDb3JlOjpMYXllclRleHR1cmVVcGRhdGVyU2tQaWN0dXJl Ojp1cGRhdGVUZXh0dXJlUmVjdCk6CisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21p dW0vTGF5ZXJUaWxlckNocm9taXVtLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkxheWVyVGlsZXJD aHJvbWl1bTo6ZHJhd1RpbGVzKToKKwogMjAxMS0wOC0xOCAgU2FpbGVzaCBBZ3Jhd2FsICA8c2Fp bEBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgQ2hyb21pdW0gTWFjOiBTaG93IHNjcm9sbGJhciB3 aGVuIGRvaW5nIHNlYXJjaApJbmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mv Y2hyb21pdW0vTGF5ZXJSZW5kZXJlckNocm9taXVtLmNwcAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9MYXllclJlbmRlcmVyQ2hyb21pdW0u Y3BwCShyZXZpc2lvbiA5MzIwOSkKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNz L2Nocm9taXVtL0xheWVyUmVuZGVyZXJDaHJvbWl1bS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTc4 OSw2ICs3ODksNyBAQCB2b2lkIExheWVyUmVuZGVyZXJDaHJvbWl1bTo6ZHJhd0xheWVyc0luCiAg ICAgbV9zaGFyZWRHZW9tZXRyeS0+cHJlcGFyZUZvckRyYXcoKTsKIAogICAgIC8vIEZJWE1FOiBU aGVzZSBjYWxscyBjYW4gYmUgbWFkZSBvbmNlLCB3aGVuIHRoZSBjb21wb3NpdG9yIGNvbnRleHQg aXMgaW5pdGlhbGl6ZWQuCisgICAgR0xDKG1fY29udGV4dC5nZXQoKSwgbV9jb250ZXh0LT5kaXNh YmxlKEdyYXBoaWNzQ29udGV4dDNEOjpTQ0lTU09SX1RFU1QpKTsKICAgICBHTEMobV9jb250ZXh0 LmdldCgpLCBtX2NvbnRleHQtPmRpc2FibGUoR3JhcGhpY3NDb250ZXh0M0Q6OkRFUFRIX1RFU1Qp KTsKICAgICBHTEMobV9jb250ZXh0LmdldCgpLCBtX2NvbnRleHQtPmRpc2FibGUoR3JhcGhpY3ND b250ZXh0M0Q6OkNVTExfRkFDRSkpOwogCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9n cmFwaGljcy9jaHJvbWl1bS9MYXllclRleHR1cmVVcGRhdGVyQ2FudmFzLmNwcAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9MYXllclRleHR1 cmVVcGRhdGVyQ2FudmFzLmNwcAkocmV2aXNpb24gOTMyMDkpCisrKyBTb3VyY2UvV2ViQ29yZS9w bGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9MYXllclRleHR1cmVVcGRhdGVyQ2FudmFzLmNwcAko d29ya2luZyBjb3B5KQpAQCAtMTc0LDcgKzE3NCw3IEBAIHZvaWQgTGF5ZXJUZXh0dXJlVXBkYXRl clNrUGljdHVyZTo6dXBkYXQKICAgICBtX2NhbnZhcy0+ZHJhd1BpY3R1cmUobV9waWN0dXJlKTsK ICAgICBtX2NhbnZhcy0+cmVzdG9yZSgpOwogICAgIC8vIEZsdXNoIFNLSUEgY29udGV4dCBzbyB0 aGF0IGFsbCB0aGUgcmVuZGVyZWQgc3R1ZmYgYXBwZWFycyBvbiB0aGUgdGV4dHVyZS4KLSAgICBt X3NraWFDb250ZXh0LT5mbHVzaChHckNvbnRleHQ6OmtGb3JjZUN1cnJlbnRSZW5kZXJUYXJnZXRf Rmx1c2hCaXQpOworICAgIG1fc2tpYUNvbnRleHQtPmZsdXNoKCk7CiAKICAgICAvLyBVbmJpbmQg dGV4dHVyZS4KICAgICBjb250ZXh0KCktPmZyYW1lYnVmZmVyVGV4dHVyZTJEKEdyYXBoaWNzQ29u dGV4dDNEOjpGUkFNRUJVRkZFUiwgR3JhcGhpY3NDb250ZXh0M0Q6OkNPTE9SX0FUVEFDSE1FTlQw LCBHcmFwaGljc0NvbnRleHQzRDo6VEVYVFVSRV8yRCwgMCwgMCk7CkluZGV4OiBTb3VyY2UvV2Vi Q29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9MYXllclRpbGVyQ2hyb21pdW0uY3BwCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL0xh eWVyVGlsZXJDaHJvbWl1bS5jcHAJKHJldmlzaW9uIDkzMjA5KQorKysgU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vTGF5ZXJUaWxlckNocm9taXVtLmNwcAkod29ya2lu ZyBjb3B5KQpAQCAtNjYxLDYgKzY2MSw3IEBAIHZvaWQgTGF5ZXJUaWxlckNocm9taXVtOjpkcmF3 VGlsZXMoY29uc3QKICAgICAgICAgLy8gUmV2ZXJzZSBkaXJlY3Rpb24uCiAgICAgICAgIHByZXZF ZGdlWS5zY2FsZSgtMSwgLTEsIC0xKTsKICAgICB9CisgICAgR0xDKGNvbnRleHQsIGNvbnRleHQt PmJpbmRUZXh0dXJlKEdyYXBoaWNzQ29udGV4dDNEOjpURVhUVVJFXzJELCAwKSk7CiB9CiAKIH0g Ly8gbmFtZXNwYWNlIFdlYkNvcmUK 105030 2011-08-24 11:38:10 -0700 2011-08-24 13:17:57 -0700 proposed patch stencil.patch text/plain 2680 alokp SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDkzNzE0KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjAgQEAKKzIwMTEtMDgtMjQgIEFsb2sgUHJp eWFkYXJzaGkgIDxhbG9rcEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgW2Nocm9taXVtXSBSb290 IGxheWVyIGlzIG5vdCB1cGRhdGVkIHdoZW4gb25seSBhIHBvcnRpb24gb2YgdGlsZSBpcyB1cGRh dGVkCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02NjUw NgorCisgICAgICAgIFRoaXMgb25seSBoYXBwZW5lZCBpbiB0aGUgYWNjZWxlcmF0ZWQgcGF0aCB3 aGljaCBjaGFuZ2VzIHN0ZW5jaWwgc3RhdGUuCisgICAgICAgIEluY29ycmVjdCBzdGVuY2lsIHN0 YXRlIHdhcyB1c2VkIHRvIGRyYXcgdGhlIHJvb3QgbGF5ZXIuCisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGVzdDogY29tcG9zaXRpbmcvb3ZlcmZsb3cv b3ZlcmZsb3ctc2Nyb2xsLmh0bWwgKGV4aXN0aW5nKQorCisgICAgICAgICogcGxhdGZvcm0vZ3Jh cGhpY3MvY2hyb21pdW0vTGF5ZXJSZW5kZXJlckNocm9taXVtLmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OkxheWVyUmVuZGVyZXJDaHJvbWl1bTo6ZHJhd0xheWVyc0ludGVybmFsKToKKyAgICAgICAg KiBwbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9MYXllclRleHR1cmVVcGRhdGVyQ2FudmFzLmNw cDoKKyAgICAgICAgKFdlYkNvcmU6OkxheWVyVGV4dHVyZVVwZGF0ZXJTa1BpY3R1cmU6OnVwZGF0 ZVRleHR1cmVSZWN0KToKKwogMjAxMS0wOC0yNCAgVG9tbXkgV2lkZW5mbHljaHQgIDx0b21teXdA Z29vZ2xlLmNvbT4KIAogICAgICAgICBNZWRpYVN0cmVhbSBBUEk6IGFkZCBjcmVhdGVPYmplY3RV UkwgZnVuY3Rpb25hbGl0eQpJbmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mv Y2hyb21pdW0vTGF5ZXJSZW5kZXJlckNocm9taXVtLmNwcAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9MYXllclJlbmRlcmVyQ2hyb21pdW0u Y3BwCShyZXZpc2lvbiA5MzcwNykKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNz L2Nocm9taXVtL0xheWVyUmVuZGVyZXJDaHJvbWl1bS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTc0 Niw3ICs3NDYsNyBAQCB2b2lkIExheWVyUmVuZGVyZXJDaHJvbWl1bTo6ZHJhd0xheWVyc0luCiAg ICAgLy8gQmluZCB0aGUgY29tbW9uIHZlcnRleCBhdHRyaWJ1dGVzIHVzZWQgZm9yIGRyYXdpbmcg YWxsIHRoZSBsYXllcnMuCiAgICAgbV9zaGFyZWRHZW9tZXRyeS0+cHJlcGFyZUZvckRyYXcoKTsK IAotICAgIC8vIEZJWE1FOiBUaGVzZSBjYWxscyBjYW4gYmUgbWFkZSBvbmNlLCB3aGVuIHRoZSBj b21wb3NpdG9yIGNvbnRleHQgaXMgaW5pdGlhbGl6ZWQuCisgICAgR0xDKG1fY29udGV4dC5nZXQo KSwgbV9jb250ZXh0LT5kaXNhYmxlKEdyYXBoaWNzQ29udGV4dDNEOjpTQ0lTU09SX1RFU1QpKTsK ICAgICBHTEMobV9jb250ZXh0LmdldCgpLCBtX2NvbnRleHQtPmRpc2FibGUoR3JhcGhpY3NDb250 ZXh0M0Q6OkRFUFRIX1RFU1QpKTsKICAgICBHTEMobV9jb250ZXh0LmdldCgpLCBtX2NvbnRleHQt PmRpc2FibGUoR3JhcGhpY3NDb250ZXh0M0Q6OkNVTExfRkFDRSkpOwogCkluZGV4OiBTb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9MYXllclRleHR1cmVVcGRhdGVyQ2Fu dmFzLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9j aHJvbWl1bS9MYXllclRleHR1cmVVcGRhdGVyQ2FudmFzLmNwcAkocmV2aXNpb24gOTM3MDcpCisr KyBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9MYXllclRleHR1cmVV cGRhdGVyQ2FudmFzLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTc4LDcgKzE3OCw3IEBAIHZvaWQg TGF5ZXJUZXh0dXJlVXBkYXRlclNrUGljdHVyZTo6dXBkYXQKICAgICBtX2NhbnZhcy0+ZHJhd1Bp Y3R1cmUobV9waWN0dXJlKTsKICAgICBtX2NhbnZhcy0+cmVzdG9yZSgpOwogICAgIC8vIEZsdXNo IFNLSUEgY29udGV4dCBzbyB0aGF0IGFsbCB0aGUgcmVuZGVyZWQgc3R1ZmYgYXBwZWFycyBvbiB0 aGUgdGV4dHVyZS4KLSAgICBtX3NraWFDb250ZXh0LT5mbHVzaChHckNvbnRleHQ6OmtGb3JjZUN1 cnJlbnRSZW5kZXJUYXJnZXRfRmx1c2hCaXQpOworICAgIG1fc2tpYUNvbnRleHQtPmZsdXNoKCk7 CiAKICAgICAvLyBVbmJpbmQgdGV4dHVyZS4KICAgICBjb250ZXh0KCktPmZyYW1lYnVmZmVyVGV4 dHVyZTJEKEdyYXBoaWNzQ29udGV4dDNEOjpGUkFNRUJVRkZFUiwgR3JhcGhpY3NDb250ZXh0M0Q6 OkNPTE9SX0FUVEFDSE1FTlQwLCBHcmFwaGljc0NvbnRleHQzRDo6VEVYVFVSRV8yRCwgMCwgMCk7 Cg==