54561 2011-02-16 08:48:25 -0800 [chromium] Layout Test fast/canvas/setWidthResetAfterForcedRender.html fails on accelerated 2D canvas 2011-02-28 12:45:19 -0800 1 1 1 Unclassified WebKit Canvas 528+ (Nightly build) PC Linux RESOLVED FIXED P2 Normal --- 1 senorblanco webkit-unassigned commit-queue eric honten jamesr kbr mdelaney7 vangelis oldest_to_newest 352105 0 senorblanco 2011-02-16 08:48:25 -0800 This layout test should produce a blank rectangle below the text, but produces a green one instead. It seems that the accelerated path is not clearing the canvas as it should.instead 352187 1 senorblanco 2011-02-16 10:46:11 -0800 Notes for posterity: This seems to be due to an interaction between the compositor and the accelerated 2D canvas. If I run DumpRenderTree on this test with only --enable-accelerated-2d-canvas, it gives the correct result. It's only when both that flag and --enable-accelerated-compositing are enabled that it fails (green square instead of white). It seems as if the compositor is not updating the canvas after the DrawingBuffer is reset. My first attempt was to try to update the compositor via a call to setNeedsStyleRecalc() in HTMLCanvasElement.cpp: 251c251,256 < if (hadImageBuffer) --- > if (hadImageBuffer) { > #if USE(IOSURFACE_CANVAS_BACKING_STORE) || (ENABLE(ACCELERATED_2D_CANVAS) && USE(ACCELERATED_COMPOSITING)) > // Force a compositor update as well. > if (m_context) > setNeedsStyleRecalc(SyntheticStyleChange); > #endif 252a258,259 > } > But this seems to cause the compositor to redraw the entire page. So the canvas rectangle is white (good), but the rest of the page is full intensity, i.e., not covered by the half-alpha mask which indicates non-repainted areas (bad). 352234 2 82670 senorblanco 2011-02-16 11:53:50 -0800 Created attachment 82670 Patch 352236 3 82670 jamesr 2011-02-16 11:56:33 -0800 Comment on attachment 82670 Patch Odd - the repaint() alone should be enough to force a recomposite. What happens on normal draw calls? 352239 4 82670 senorblanco 2011-02-16 12:00:08 -0800 Comment on attachment 82670 Patch Taking r? off this, since I'm not sure it's the correct solution yet. Just wanted to put it in context. 352399 5 82707 senorblanco 2011-02-16 15:22:50 -0800 Created attachment 82707 Patch 352402 6 senorblanco 2011-02-16 15:26:10 -0800 (In reply to comment #5) > Created an attachment (id=82707) [details] > Patch I'm more confident in this fix. This basically does the same thing that CanvasRenderingContext2D::didDraw() does, by calling contentChanged() on the RenderLayer. This still leaves the pixel results wrong, since the compositor doesn't seem to be compatible with repaint tests (it redraws the whole image), but at least the canvas reset is correctly showing up now (white box instead of green). 353034 7 82707 jamesr 2011-02-17 11:09:58 -0800 Comment on attachment 82707 Patch R=me. Thought: could we call didDraw(FloatRect(canvas()->width(), canvas->height()) ? 353043 8 senorblanco 2011-02-17 11:19:58 -0800 (In reply to comment #7) > (From update of attachment 82707 [details]) > R=me. > > Thought: could we call didDraw(FloatRect(canvas()->width(), canvas->height()) ? We probably could, but this might also affect the software path, which I don't believe needs this change. 353228 9 senorblanco 2011-02-17 15:57:08 -0800 The commit queue kept failing on unrelated tests, so I landed this manually as http://trac.webkit.org/changeset/78922. 353248 10 eric 2011-02-17 16:55:45 -0800 The cq was waiting for the tree to be green. It runs "hot" in that it keeps trying until success. It didn't ever "fail" this patch or it would have noted so in the bug. 358282 11 honten 2011-02-25 16:55:40 -0800 Still the tree info fails but the bug itself looks correct. So we have to (In reply to comment #10) > The cq was waiting for the tree to be green. It runs "hot" in that it keeps trying until success. It didn't ever "fail" this patch or it would have noted so in the bug. 358284 12 honten 2011-02-25 16:58:14 -0800 Oops, sorry, I miss-operated. Still the tree info fails but the bug itself looks gone. So we have to correct "expected" tree files. I guess, not only setWidthResetAfterForcedRender.html, but also other layout test fail because the tree result is unexpected. (In reply to comment #11) > > (In reply to comment #10) > > The cq was waiting for the tree to be green. It runs "hot" in that it keeps trying until success. It didn't ever "fail" this patch or it would have noted so in the bug. 359086 13 senorblanco 2011-02-28 08:44:54 -0800 (In reply to comment #12) > Oops, sorry, I miss-operated. > > Still the tree info fails but the bug itself looks gone. > > So we have to correct "expected" tree files. > > I guess, not only setWidthResetAfterForcedRender.html, but also other layout test fail because the tree result is unexpected. Although the results are correct in Chrome (same result in software or hardware), the test is still failing since LayoutTestController does not seem to do the right thing for composited layers. I think we need a way to make LayoutTestController.display() (or more precisely, WebViewHost::displayRepaintMask) write its results into the composited layer, rather than on top of the framebuffer. So in this case, we shouldn't rebaseline (commit a new expected result), since the result is still wrong until LayoutTestController is fixed. > (In reply to comment #11) > > > > (In reply to comment #10) > > > The cq was waiting for the tree to be green. It runs "hot" in that it keeps trying until success. It didn't ever "fail" this patch or it would have noted so in the bug. 359251 14 kbr 2011-02-28 12:45:19 -0800 (In reply to comment #13) > Although the results are correct in Chrome (same result in software or hardware), the test is still failing since LayoutTestController does not seem to do the right thing for composited layers. I think we need a way to make LayoutTestController.display() (or more precisely, WebViewHost::displayRepaintMask) write its results into the composited layer, rather than on top of the framebuffer. > > So in this case, we shouldn't rebaseline (commit a new expected result), since the result is still wrong until LayoutTestController is fixed. Is there a bug filed about this issue with the LayoutTestController and the compositor? If not, could you file one? 82670 2011-02-16 11:53:50 -0800 2011-02-16 15:22:47 -0800 Patch bug-54561-20110216145348.patch text/plain 1700 senorblanco SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDc4NzExKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTggQEAKKzIwMTEtMDItMTYgIFN0ZXBoZW4g V2hpdGUgIDxzZW5vcmJsYW5jb0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTGF5b3V0IFRlc3QgZmFzdC9jYW52YXMvc2V0V2lk dGhSZXNldEFmdGVyRm9yY2VkUmVuZGVyLmh0bWwgZmFpbHMgb24gYWNjZWxlcmF0ZWQgMkQgY2Fu dmFzLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTQ1 NjEKKworICAgICAgICBXaGVuIHJlc2V0dGluZyB0aGUgY2FudmFzJ3MgaW1hZ2UgYnVmZmVyLCB3 ZSBhbHNvIG5lZWQgdG8gc2VuZCBhCisgICAgICAgIHN5bnRoZXRpYyBzdHlsZSBjaGFuZ2UsIGlu IG9yZGVyIHRvIHRyaWdnZXIgdGhlIGNvbXBvc2l0b3IgdG8gcmVkcmF3LgorCisgICAgICAgIENv dmVyZWQgYnkgZmFzdC9jYW52YXMvc2V0V2lkdGhSZXNldEFmdGVyRm9yY2VkUmVuZGVyLmh0bWwK KworICAgICAgICAqIGh0bWwvSFRNTENhbnZhc0VsZW1lbnQuY3BwOgorICAgICAgICAoV2ViQ29y ZTo6SFRNTENhbnZhc0VsZW1lbnQ6OnJlc2V0KToKKwogMjAxMS0wMi0xNiAgQW5kcmVhcyBLbGlu ZyAgPGtsaW5nQHdlYmtpdC5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgRGlyayBTY2h1bHpl LgpJbmRleDogU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MQ2FudmFzRWxlbWVudC5jcHAKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MQ2FudmFzRWxlbWVudC5jcHAJKHJldmlz aW9uIDc4NzExKQorKysgU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MQ2FudmFzRWxlbWVudC5jcHAJ KHdvcmtpbmcgY29weSkKQEAgLTI0OCw4ICsyNDgsMTUgQEAgdm9pZCBIVE1MQ2FudmFzRWxlbWVu dDo6cmVzZXQoKQogICAgICAgICBpZiAobV9yZW5kZXJlcklzQ2FudmFzKSB7CiAgICAgICAgICAg ICBpZiAob2xkU2l6ZSAhPSBzaXplKCkpCiAgICAgICAgICAgICAgICAgdG9SZW5kZXJIVE1MQ2Fu dmFzKHJlbmRlcmVyKS0+Y2FudmFzU2l6ZUNoYW5nZWQoKTsKLSAgICAgICAgICAgIGlmIChoYWRJ bWFnZUJ1ZmZlcikKKyAgICAgICAgICAgIGlmIChoYWRJbWFnZUJ1ZmZlcikgeworI2lmIFVTRShJ T1NVUkZBQ0VfQ0FOVkFTX0JBQ0tJTkdfU1RPUkUpIHx8IChFTkFCTEUoQUNDRUxFUkFURURfMkRf Q0FOVkFTKSAmJiBVU0UoQUNDRUxFUkFURURfQ09NUE9TSVRJTkcpKQorICAgICAgICAgICAgICAg IC8vIEZvcmNlIGEgY29tcG9zaXRvciB1cGRhdGUgYXMgd2VsbC4KKyAgICAgICAgICAgICAgICBp ZiAobV9jb250ZXh0KQorICAgICAgICAgICAgICAgICAgICBzZXROZWVkc1N0eWxlUmVjYWxjKFN5 bnRoZXRpY1N0eWxlQ2hhbmdlKTsKKyNlbmRpZgogICAgICAgICAgICAgICAgIHJlbmRlcmVyLT5y ZXBhaW50KCk7CisgICAgICAgICAgICB9CisKICAgICAgICAgfQogICAgIH0KIAo= 82707 2011-02-16 15:22:50 -0800 2011-02-17 15:36:18 -0800 Patch bug-54561-20110216182249.patch text/plain 2098 senorblanco SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDc4NzM5KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjMgQEAKKzIwMTEtMDItMTYgIFN0ZXBoZW4g V2hpdGUgIDxzZW5vcmJsYW5jb0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTGF5b3V0IFRlc3QgZmFzdC9jYW52YXMvc2V0V2lk dGhSZXNldEFmdGVyRm9yY2VkUmVuZGVyLmh0bWwgZmFpbHMgb24KKyAgICAgICAgYWNjZWxlcmF0 ZWQgMkQgY2FudmFzIHcvY29tcG9zaXRvciBlbmFibGVkLgorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTQ1NjEKKworICAgICAgICBXaGVuIHJlc2V0dGlu ZyB0aGUgQ2FudmFzUmVuZGVyaW5nQ29udGV4dDJELCB3ZSBhbHNvIG5lZWQgdG8gc2VuZCBhCisg ICAgICAgIGNvbnRlbnRDaGFuZ2VkKCkgdG8gdGhlIHJlbGV2YW50IFJlbmRlckxheWVyLiAgVGhp cyBpcyBzaW1pbGFyIHRvIHdoYXQKKyAgICAgICAgaXMgZG9uZSBpbiBkaWREcmF3KCkuCisKKyAg ICAgICAgQ292ZXJlZCBieSBmYXN0L2NhbnZhcy9zZXRXaWR0aFJlc2V0QWZ0ZXJGb3JjZWRSZW5k ZXIuaHRtbCwgYnV0IG5vdGUgCisgICAgICAgIHRoYXQgdGhpcyB0ZXN0IHdpbGwgc3RpbGwgZmFp bCBwaXhlbCB0ZXN0cyBiZWNhdXNlIHRoZSBjb21wb3NpdG9yCisgICAgICAgIGlzIG5vdCBjb21w YXRpYmxlIHdpdGggcmVwYWludCB0ZXN0cyAodGhlIGdyZWVuIHNxdWFyZSBpcyBub3cgd2hpdGUs CisgICAgICAgIGJ1dCB0aGUgaGFsZi10cmFuc3BhcmVudCBncmV5IHJlcGFpbnQgcmVjdCBkb2Vz IG5vdCBhcHBlYXIpLgorCisgICAgICAgICogaHRtbC9jYW52YXMvQ2FudmFzUmVuZGVyaW5nQ29u dGV4dDJELmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkNhbnZhc1JlbmRlcmluZ0NvbnRleHQyRDo6 cmVzZXQpOgorCiAyMDExLTAyLTE2ICBKaWFuIExpICA8amlhbmxpQGNocm9taXVtLm9yZz4KIAog ICAgICAgICBSZXZpZXdlZCBieSBLZW5uZXRoIFJ1c3NlbGwuCkluZGV4OiBTb3VyY2UvV2ViQ29y ZS9odG1sL2NhbnZhcy9DYW52YXNSZW5kZXJpbmdDb250ZXh0MkQuY3BwCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IFNvdXJjZS9XZWJDb3JlL2h0bWwvY2FudmFzL0NhbnZhc1JlbmRlcmluZ0NvbnRleHQyRC5jcHAJ KHJldmlzaW9uIDc4NzM5KQorKysgU291cmNlL1dlYkNvcmUvaHRtbC9jYW52YXMvQ2FudmFzUmVu ZGVyaW5nQ29udGV4dDJELmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTc0LDYgKzE3NCwxMSBAQCB2 b2lkIENhbnZhc1JlbmRlcmluZ0NvbnRleHQyRDo6cmVzZXQoKQogICAgICAgICBpZiAobV9jb250 ZXh0M0QgJiYgbV9kcmF3aW5nQnVmZmVyKSB7CiAgICAgICAgICAgICBtX2RyYXdpbmdCdWZmZXIt PnJlc2V0KEludFNpemUoY2FudmFzKCktPndpZHRoKCksIGNhbnZhcygpLT5oZWlnaHQoKSkpOwog ICAgICAgICAgICAgYy0+c2V0U2hhcmVkR3JhcGhpY3NDb250ZXh0M0QobV9jb250ZXh0M0QuZ2V0 KCksIG1fZHJhd2luZ0J1ZmZlci5nZXQoKSwgSW50U2l6ZShjYW52YXMoKS0+d2lkdGgoKSwgY2Fu dmFzKCktPmhlaWdodCgpKSk7CisjaWYgVVNFKEFDQ0VMRVJBVEVEX0NPTVBPU0lUSU5HKQorICAg ICAgICAgICAgUmVuZGVyQm94KiByZW5kZXJCb3ggPSBjYW52YXMoKS0+cmVuZGVyQm94KCk7Cisg ICAgICAgICAgICBpZiAocmVuZGVyQm94ICYmIHJlbmRlckJveC0+aGFzTGF5ZXIoKSAmJiByZW5k ZXJCb3gtPmxheWVyKCktPmhhc0FjY2VsZXJhdGVkQ29tcG9zaXRpbmcoKSkKKyAgICAgICAgICAg ICAgICByZW5kZXJCb3gtPmxheWVyKCktPmNvbnRlbnRDaGFuZ2VkKFJlbmRlckxheWVyOjpDYW52 YXNDaGFuZ2VkKTsKKyNlbmRpZgogICAgICAgICB9CiAgICAgfQogI2VuZGlmCg==