71057 2011-10-27 13:19:50 -0700 [chromium] webkit_gpu_tests are flaky with --accelerated-drawing 2011-10-31 12:11:44 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) All All RESOLVED WONTFIX P2 Normal --- 70822 1 alokp alokp jamesr oldest_to_newest 491861 0 alokp 2011-10-27 13:19:50 -0700 Accelerated drawing path does not reset the framebuffer state. It just deletes the temporary framebuffer. According to OpenGL spec, deleting a bound framebuffer should set the default frame buffer as current, but at least Mesa does not seem to do so. I think it is better to explicitly reset the framebuffer state. 492014 1 112767 alokp 2011-10-27 15:24:28 -0700 Created attachment 112767 proposed patch 492983 2 alokp 2011-10-29 15:10:00 -0700 ping! This needs to be committed before https://bugs.webkit.org/show_bug.cgi?id=70822. 492984 3 112767 jamesr 2011-10-29 15:15:28 -0700 Comment on attachment 112767 proposed patch I don't think this is right. If there is a bug with our implementation of GraphicsContext3D we should fix that, no? 492985 4 jamesr 2011-10-29 15:15:49 -0700 I would suspect that this sort of things would be handled by the command buffer code, not osmesa. 492987 5 alokp 2011-10-29 15:35:17 -0700 (In reply to comment #4) > I would suspect that this sort of things would be handled by the command buffer code, not osmesa. I should have been clearer. According to the OpenGL spec, if a currently-bound FBO is deleted, the default framebuffer (id = 0) is bound. So it is the job of the underlying GL implementation (osmesa) in this case to implement the spec. I do not this GraphicsContext3D or command-buffer should do anything extra. 492988 6 jamesr 2011-10-29 15:38:34 -0700 I'm not sure what point you are trying to make. I see code to handle deleting the currently bound framebuffer in GLES2DecoderImpl::DeleteFramebuffersHelper() and am pretty sure it's relevant here. Whatever the issue, we shouldn't be working around it in WebKit we should fix it wherever the bug actually lies. 492992 7 alokp 2011-10-29 15:59:58 -0700 (In reply to comment #6) > I'm not sure what point you are trying to make. I see code to handle deleting the currently bound framebuffer in GLES2DecoderImpl::DeleteFramebuffersHelper() and am pretty sure it's relevant here. > > Whatever the issue, we shouldn't be working around it in WebKit we should fix it wherever the bug actually lies. If you read the OpenGL spec for glDeleteFrameBuffers (http://www.opengl.org/sdk/docs/man3/xhtml/glDeleteFramebuffers.xml), it says: "If a framebuffer that is currently bound to one or more of the targets GL_DRAW_FRAMEBUFFER or GL_READ_FRAMEBUFFER is deleted, it is as though glBindFramebuffer had been executed with the corresponding target and framebuffer zero." Anyways my point is that the accelerated drawing path and Ganesh modify the currently bound framebuffer and do not restore it. We either restore it here or like other GL states do it in the LayerRendererChromium at the beginning of each frame. Doing it in LayerRendererChromium would be consistent with other GL states but unnecessary for the software path. 492993 8 jamesr 2011-10-29 16:12:17 -0700 I still don't quite get it. Is the scenario you are concerned with that ganesh will think FB #3 is bound, this code will delete FB #4, which binds FB #0 and confuses ganesh? If so then your patch doesn't make much sense since it binds FB #0 explicitly. Is there some other failure scenario? An example would help. 493417 9 alokp 2011-10-31 11:08:30 -0700 (In reply to comment #6) > I'm not sure what point you are trying to make. I see code to handle deleting the currently bound framebuffer in GLES2DecoderImpl::DeleteFramebuffersHelper() and am pretty sure it's relevant here. > > Whatever the issue, we shouldn't be working around it in WebKit we should fix it wherever the bug actually lies. I think you are right. Command buffer is internally using FBO=1 as the default render-surface for off-screen contexts, which it should bind when currently bound FBO is deleted. I have filed a bug here - crbug.com/102391 The reason this bug was only visible in layout tests and not chrome because layout-tests use an off-screen render surface. It was also not visible when render-to-texture was enabled because that code path explicitly binds FBO=0 when doing the final paint to the window. 493438 10 jamesr 2011-10-31 12:11:44 -0700 OK, then when that's fixed we should be good to go without this patch. Thanks for tracing this down to the root cause. 112767 2011-10-27 15:24:28 -0700 2011-10-29 15:15:28 -0700 proposed patch 71057.patch text/plain 1590 alokp SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDk4NjQ3KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTEtMTAtMjcgIEFsb2sgUHJp eWFkYXJzaGkgIDxhbG9rcEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgW2Nocm9taXVtXSB3ZWJr aXRfZ3B1X3Rlc3RzIGFyZSBmbGFreSB3aXRoIC0tYWNjZWxlcmF0ZWQtZHJhd2luZworICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzEwNTcKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBuZXcgdGVzdHMgbmVl ZGVkLiBJdCBhcHBsaWVzIHRvIGFsbCBncHUgdGVzdHMgd2l0aCAtLWFjY2VsZXJhdGVkLWRyYXdp bmcgZmxhZy4KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL0xheWVyVGV4 dHVyZVVwZGF0ZXJDYW52YXMuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RnJhbWVCdWZmZXI6OkZy YW1lQnVmZmVyOjp+RnJhbWVCdWZmZXIpOgorCiAyMDExLTEwLTI3ICBTaGVyaWZmIEJvdCAgPHdl YmtpdC5yZXZpZXcuYm90QGdtYWlsLmNvbT4KIAogICAgICAgICBVbnJldmlld2VkLCByb2xsaW5n IG91dCByOTg2MjYuCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJv bWl1bS9MYXllclRleHR1cmVVcGRhdGVyQ2FudmFzLmNwcAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9MYXllclRleHR1cmVVcGRhdGVyQ2Fu dmFzLmNwcAkocmV2aXNpb24gOTg2NDYpCisrKyBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFw aGljcy9jaHJvbWl1bS9MYXllclRleHR1cmVVcGRhdGVyQ2FudmFzLmNwcAkod29ya2luZyBjb3B5 KQpAQCAtNzIsOCArNzIsMTEgQEAgRnJhbWVCdWZmZXI6On5GcmFtZUJ1ZmZlcigpCiB7CiAgICAg bV9jYW52YXMuY2xlYXIoKTsKIAotICAgIGlmIChtX2ZibykKKyAgICBpZiAobV9mYm8pIHsKKyAg ICAgICAgbV9jb250ZXh0LT5mcmFtZWJ1ZmZlclRleHR1cmUyRChHcmFwaGljc0NvbnRleHQzRDo6 RlJBTUVCVUZGRVIsIEdyYXBoaWNzQ29udGV4dDNEOjpDT0xPUl9BVFRBQ0hNRU5UMCwgR3JhcGhp Y3NDb250ZXh0M0Q6OlRFWFRVUkVfMkQsIDAsIDApOworICAgICAgICBtX2NvbnRleHQtPmJpbmRG cmFtZWJ1ZmZlcihHcmFwaGljc0NvbnRleHQzRDo6RlJBTUVCVUZGRVIsIDApOwogICAgICAgICBt X2NvbnRleHQtPmRlbGV0ZUZyYW1lYnVmZmVyKG1fZmJvKTsKKyAgICB9CiB9CiAKIFNrQ2FudmFz KiBGcmFtZUJ1ZmZlcjo6aW5pdGlhbGl6ZShHcmFwaGljc0NvbnRleHQzRCogY29udGV4dCwgVGV4 dHVyZUFsbG9jYXRvciogYWxsb2NhdG9yLCBNYW5hZ2VkVGV4dHVyZSogdGV4dHVyZSkK