64613 2011-07-15 11:56:12 -0700 Use supported framebuffer renderbuffer mode. 2011-08-05 19:26:14 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 65037 0 tomhudson webkit-unassigned alokp jamesr senorblanco tomhudson vangelis webkit.review.bot oldest_to_newest 437873 0 tomhudson 2011-07-15 11:56:12 -0700 Use supported framebuffer renderbuffer mode. 437874 1 101018 tomhudson 2011-07-15 11:58:49 -0700 Created attachment 101018 Patch 437878 2 tomhudson 2011-07-15 12:00:47 -0700 When force compositing & accelerated drawing are both on, the glFramebufferRenderbuffer call failed because the Chromium command buffer supports DEPTH and STENCIL but not DEPTH_STENCIL attachment modes. 437883 3 101018 alokp 2011-07-15 12:03:21 -0700 Comment on attachment 101018 Patch looks good 437891 4 101022 tomhudson 2011-07-15 12:11:45 -0700 Created attachment 101022 Patch 437897 5 101022 jamesr 2011-07-15 12:19:51 -0700 Comment on attachment 101022 Patch We already query for the GL_OES_packed_depth_stencil extension at lines 211-214, so how is this code failing? It makes me really sad that we aren't sharing code with DrawingBuffer. 437900 6 tomhudson 2011-07-15 12:25:21 -0700 Although that framebuffer configuration is supported, that enum for requesting the framebuffer configuration isn't. This is an EGL oddness that the Chromium command buffer mimics: you can get a DEPTH_STENCIL buffer, but only by requesting separately as DEPTH and STENCIL. So in practice: we succeed at the extensions->supports() call, then we get an error message from gles2_cmd_decoder.cc ("glFramebufferRenderbuffer: attachment GL_INVALID_ENUM) and cascading errors. 437917 7 alokp 2011-07-15 12:39:36 -0700 (In reply to comment #5) > (From update of attachment 101022 [details]) > We already query for the GL_OES_packed_depth_stencil extension at lines 211-214, so how is this code failing? > > It makes me really sad that we aren't sharing code with DrawingBuffer. Brian is working on moving most of the stencil buffer management code here to SKIA. 437922 8 jamesr 2011-07-15 12:45:08 -0700 If that fixes things, great. Otherwise (or until then) we should be using DrawingBuffer. 441048 9 vangelis 2011-07-22 11:29:48 -0700 (In reply to comment #8) > If that fixes things, great. Otherwise (or until then) we should be using DrawingBuffer. Not sure I understand the objection to this patch though. If appears to fix a path that's already there but is not coded the right way. If a later change simplified things, that's even better, but why not fix now something we know is broken? 441101 10 jamesr 2011-07-22 12:38:48 -0700 (In reply to comment #9) > (In reply to comment #8) > > If that fixes things, great. Otherwise (or until then) we should be using DrawingBuffer. > > Not sure I understand the objection to this patch though. If appears to fix a path that's already there but is not coded the right way. If a later change simplified things, that's even better, but why not fix now something we know is broken? The patch isn't wrong, but it's a symptom of a greater issue that this code is using a completely untested codepath that breaks all the time. We typically require tests for patches because patching untested code is a large waste of effort. If this code was using the same code as the test canvas 2d path, then it wouldn't have to deal with this issue and it wouldn't suffer from the other breakages. 441718 11 vangelis 2011-07-25 10:10:57 -0700 (In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > If that fixes things, great. Otherwise (or until then) we should be using DrawingBuffer. > > > > Not sure I understand the objection to this patch though. If appears to fix a path that's already there but is not coded the right way. If a later change simplified things, that's even better, but why not fix now something we know is broken? > > The patch isn't wrong, but it's a symptom of a greater issue that this code is using a completely untested codepath that breaks all the time. We typically require tests for patches because patching untested code is a large waste of effort. If this code was using the same code as the test canvas 2d path, then it wouldn't have to deal with this issue and it wouldn't suffer from the other breakages. The accelerated drawing path doesn't necessarily require its own tests. We just need to start running DRT with the --accelerated-drawing flag to make sure there are no regressions. There is some refactoring work going on both to make the DrawingBuffer less canvas2d-specific and to move the management of the buffers into Ganesh. Neither of these changes is going to make M14 but it would be a shame if the accelerated drawing option (which otherwise works modulo a couple more regression fixes that are in review) doesn't make it and has to be removed. I would consider this as a short term fix to meet our ship deadline. It is after an improvement over code that's already been checked in and is known broken. 447550 12 101022 webkit.review.bot 2011-08-05 19:26:10 -0700 Comment on attachment 101022 Patch Clearing flags on attachment: 101022 Committed r92541: <http://trac.webkit.org/changeset/92541> 447551 13 webkit.review.bot 2011-08-05 19:26:14 -0700 All reviewed patches have been landed. Closing bug. 101018 2011-07-15 11:58:49 -0700 2011-07-15 12:11:39 -0700 Patch bug-64613-20110715145847.patch text/plain 2108 tomhudson SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDkxMDg0KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDctMTUgIFRvbSBIdWRz b24gIDx0b21odWRzb25AZ29vZ2xlLmNvbT4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjQ2MTMKKyAgICAgICAgVXNlIHN1cHBvcnRlZCBmcmFtZWJ1 ZmZlciByZW5kZXJidWZmZXIgbW9kZTsgY2hyb21pdW0gY29tbWFuZCBidWZmZXIKKyAgICAgICAg YWxsb3dzIERFUFRIIGFuZCBTVEVOQ0lMIGJ1dCBub3QgREVQVEhfU1RFTkNJTC4KKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBuZXcgdGVzdHMgYmVj YXVzZSB3YXMgY2F1Z2h0IGJ5IGV4dGFudCB0ZXN0cywgYWxiZWl0IG9ubHkgd2hlbgorICAgICAg ICBhY2NlbGVyYXRlZCBkcmF3aW5nIGFuZCBmb3JjZWQgY29tcG9zaXRpbmcgd2VyZSBib3RoIHR1 cm5lZCBvbi4KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL0xheWVyVGV4 dHVyZVVwZGF0ZXJDYW52YXMuY3BwOgorICAgICAgICAoV2ViQ29yZTo6TGF5ZXJUZXh0dXJlVXBk YXRlclNrUGljdHVyZTo6Y3JlYXRlRnJhbWVCdWZmZXIpOgorCiAyMDExLTA3LTE1ICBQcmF0aWsg U29sYW5raSAgPHBzb2xhbmtpQGFwcGxlLmNvbT4KIAogICAgICAgICBQYXJ0IG9mIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02MzY3NApJbmRleDogU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vTGF5ZXJUZXh0dXJlVXBkYXRlckNhbnZhcy5j cHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21p dW0vTGF5ZXJUZXh0dXJlVXBkYXRlckNhbnZhcy5jcHAJKHJldmlzaW9uIDkxMDcwKQorKysgU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vTGF5ZXJUZXh0dXJlVXBkYXRl ckNhbnZhcy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTIyOSw3ICsyMjksOCBAQCBib29sIExheWVy VGV4dHVyZVVwZGF0ZXJTa1BpY3R1cmU6OmNyZWF0CiAgICAgfQogICAgIGNvbnRleHQoKS0+Ymlu ZFJlbmRlcmJ1ZmZlcihHcmFwaGljc0NvbnRleHQzRDo6UkVOREVSQlVGRkVSLCBtX2RlcHRoU3Rl bmNpbEJ1ZmZlcik7CiAgICAgY29udGV4dCgpLT5yZW5kZXJidWZmZXJTdG9yYWdlKEdyYXBoaWNz Q29udGV4dDNEOjpSRU5ERVJCVUZGRVIsIEV4dGVuc2lvbnMzRDo6REVQVEgyNF9TVEVOQ0lMOCwg bV9idWZmZXJTaXplLndpZHRoKCksIG1fYnVmZmVyU2l6ZS5oZWlnaHQoKSk7Ci0gICAgY29udGV4 dCgpLT5mcmFtZWJ1ZmZlclJlbmRlcmJ1ZmZlcihHcmFwaGljc0NvbnRleHQzRDo6RlJBTUVCVUZG RVIsIEdyYXBoaWNzQ29udGV4dDNEOjpERVBUSF9TVEVOQ0lMX0FUVEFDSE1FTlQsIEdyYXBoaWNz Q29udGV4dDNEOjpSRU5ERVJCVUZGRVIsIG1fZGVwdGhTdGVuY2lsQnVmZmVyKTsKKyAgICBjb250 ZXh0KCktPmZyYW1lYnVmZmVyUmVuZGVyYnVmZmVyKEdyYXBoaWNzQ29udGV4dDNEOjpGUkFNRUJV RkZFUiwgR3JhcGhpY3NDb250ZXh0M0Q6OkRFUFRIX0FUVEFDSE1FTlQsIEdyYXBoaWNzQ29udGV4 dDNEOjpSRU5ERVJCVUZGRVIsIG1fZGVwdGhTdGVuY2lsQnVmZmVyKTsKKyAgICBjb250ZXh0KCkt PmZyYW1lYnVmZmVyUmVuZGVyYnVmZmVyKEdyYXBoaWNzQ29udGV4dDNEOjpGUkFNRUJVRkZFUiwg R3JhcGhpY3NDb250ZXh0M0Q6OlNURU5DSUxfQVRUQUNITUVOVCwgR3JhcGhpY3NDb250ZXh0M0Q6 OlJFTkRFUkJVRkZFUiwgbV9kZXB0aFN0ZW5jaWxCdWZmZXIpOwogCiAgICAgLy8gQ3JlYXRlIGEg c2tpYSBncHUgY2FudmFzLgogICAgIEdyUGxhdGZvcm1TdXJmYWNlRGVzYyB0YXJnZXREZXNjOwo= 101022 2011-07-15 12:11:45 -0700 2011-08-05 19:26:10 -0700 Patch bug-64613-20110715151144.patch text/plain 2108 tomhudson SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDkxMDg0KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDctMTUgIFRvbSBIdWRz b24gIDx0b21odWRzb25AZ29vZ2xlLmNvbT4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjQ2MTMKKyAgICAgICAgVXNlIHN1cHBvcnRlZCBmcmFtZWJ1 ZmZlciByZW5kZXJidWZmZXIgbW9kZTsgY2hyb21pdW0gY29tbWFuZCBidWZmZXIKKyAgICAgICAg YWxsb3dzIERFUFRIIGFuZCBTVEVOQ0lMIGJ1dCBub3QgREVQVEhfU1RFTkNJTC4KKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBuZXcgdGVzdHMgYmVj YXVzZSB3YXMgY2F1Z2h0IGJ5IGV4dGFudCB0ZXN0cywgYWxiZWl0IG9ubHkgd2hlbgorICAgICAg ICBhY2NlbGVyYXRlZCBkcmF3aW5nIGFuZCBmb3JjZWQgY29tcG9zaXRpbmcgd2VyZSBib3RoIHR1 cm5lZCBvbi4KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL0xheWVyVGV4 dHVyZVVwZGF0ZXJDYW52YXMuY3BwOgorICAgICAgICAoV2ViQ29yZTo6TGF5ZXJUZXh0dXJlVXBk YXRlclNrUGljdHVyZTo6Y3JlYXRlRnJhbWVCdWZmZXIpOgorCiAyMDExLTA3LTE1ICBQcmF0aWsg U29sYW5raSAgPHBzb2xhbmtpQGFwcGxlLmNvbT4KIAogICAgICAgICBQYXJ0IG9mIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02MzY3NApJbmRleDogU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vTGF5ZXJUZXh0dXJlVXBkYXRlckNhbnZhcy5j cHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21p dW0vTGF5ZXJUZXh0dXJlVXBkYXRlckNhbnZhcy5jcHAJKHJldmlzaW9uIDkxMDcwKQorKysgU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vTGF5ZXJUZXh0dXJlVXBkYXRl ckNhbnZhcy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTIyOSw3ICsyMjksOCBAQCBib29sIExheWVy VGV4dHVyZVVwZGF0ZXJTa1BpY3R1cmU6OmNyZWF0CiAgICAgfQogICAgIGNvbnRleHQoKS0+Ymlu ZFJlbmRlcmJ1ZmZlcihHcmFwaGljc0NvbnRleHQzRDo6UkVOREVSQlVGRkVSLCBtX2RlcHRoU3Rl bmNpbEJ1ZmZlcik7CiAgICAgY29udGV4dCgpLT5yZW5kZXJidWZmZXJTdG9yYWdlKEdyYXBoaWNz Q29udGV4dDNEOjpSRU5ERVJCVUZGRVIsIEV4dGVuc2lvbnMzRDo6REVQVEgyNF9TVEVOQ0lMOCwg bV9idWZmZXJTaXplLndpZHRoKCksIG1fYnVmZmVyU2l6ZS5oZWlnaHQoKSk7Ci0gICAgY29udGV4 dCgpLT5mcmFtZWJ1ZmZlclJlbmRlcmJ1ZmZlcihHcmFwaGljc0NvbnRleHQzRDo6RlJBTUVCVUZG RVIsIEdyYXBoaWNzQ29udGV4dDNEOjpERVBUSF9TVEVOQ0lMX0FUVEFDSE1FTlQsIEdyYXBoaWNz Q29udGV4dDNEOjpSRU5ERVJCVUZGRVIsIG1fZGVwdGhTdGVuY2lsQnVmZmVyKTsKKyAgICBjb250 ZXh0KCktPmZyYW1lYnVmZmVyUmVuZGVyYnVmZmVyKEdyYXBoaWNzQ29udGV4dDNEOjpGUkFNRUJV RkZFUiwgR3JhcGhpY3NDb250ZXh0M0Q6OkRFUFRIX0FUVEFDSE1FTlQsIEdyYXBoaWNzQ29udGV4 dDNEOjpSRU5ERVJCVUZGRVIsIG1fZGVwdGhTdGVuY2lsQnVmZmVyKTsKKyAgICBjb250ZXh0KCkt PmZyYW1lYnVmZmVyUmVuZGVyYnVmZmVyKEdyYXBoaWNzQ29udGV4dDNEOjpGUkFNRUJVRkZFUiwg R3JhcGhpY3NDb250ZXh0M0Q6OlNURU5DSUxfQVRUQUNITUVOVCwgR3JhcGhpY3NDb250ZXh0M0Q6 OlJFTkRFUkJVRkZFUiwgbV9kZXB0aFN0ZW5jaWxCdWZmZXIpOwogCiAgICAgLy8gQ3JlYXRlIGEg c2tpYSBncHUgY2FudmFzLgogICAgIEdyUGxhdGZvcm1TdXJmYWNlRGVzYyB0YXJnZXREZXNjOwo=