There are occlusion issues visible on http://helloracer.com/webgl/ in Safari, but not in Chrome. In Safari, as you drive the car around you can sometimes see the driver's legs, and there are obvious issues around the front wheel struts (see screenshots).
Created attachment 87162 [details] Safari screenshot
Created attachment 87163 [details] Chrome screenshot
That looks like a z-buffer precision issue. Chrome typically uses a packed depth/stencil renderbuffer, increasing the precision of the depth buffer to 24 bits; perhaps Safari's WebGL implementation is forcing a 16-bit depth buffer?
Created attachment 87385 [details] Patch Ken's right. Our depth buffer is 16 bits max, unless you explicitly ask for a stencil buffer. Ultimately I think we want to follow the approach Chrome does, where it tests for things via Extensions.
BTW - as far as I can tell, the other WebGL ports (Chrome and QT) use their own setup code, and not GraphicsContext3DOpenGL, despite the generic sounding name. If I'm wrong, then this patch is bad because it assumes the 24/8 depth/stencil extension is there. Hence my suggestion that we move to the same Extensions testing as Chrome.
Chromium does not go through GraphicsContext3DOpenGL, but there is an effort to share it between Mac and Qt. See https://bugs.webkit.org/show_bug.cgi?id=57261 So maybe you should use if defined PLATFORM(MAC) to guard your code? And go to the original path if it's other ports.
Comment on attachment 87385 [details] Patch There is a check for the availability of GL_EXT_packed_depth_stencil in GraphicsContext3D::validateAttributes higher up in the file, but it doesn't guarantee that the logic here will be covered. It would probably be best to rewrite validateAttributes in terms of the Extensions3D class. For example: Extensions3D* extensions = getExtensions(); if (m_attrs.stencil) { if (extensions->supports("GL_EXT_packed_depth_stencil")) { extensions->ensureEnabled("GL_EXT_packed_depth_stencil"); ... } ... } ... if (!isValidVendor || !extensions->supports("GL_ANGLE_framebuffer_multisample")) m_attrs.antialias = false; else extensions->ensureEnabled("GL_ANGLE_framebuffer_multisample");
Created attachment 87400 [details] Patch I've rewritten validateAttributes() as Ken suggested. I've also called it from within reshape(). This isn't needed for the Mac port because the GraphicsContext3D constructor calls it, but it seems the Qt port doesn't. Now reshape uses Extensions3D to test for the ability to use the 24bit depth buffer, which means it should be safe on any port that comes through here.
Comment on attachment 87400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87400&action=review Looks good except for one nit. > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:57 > + Extensions3D* extensions = getExtensions(); getExtensions() should really return a const pointer to reflect the ownership model. Maybe just make it a const here and open a new bug to change the call signature. > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:62 > + m_attrs.depth = true; Do you really need to set the depth attr to true? You can still use depth_stencil and just waste the 24 bits of depth if you leave this false. I'm thinking about the different behavior you might get if one implementation has this true and another has it false, since this is an author visible flag. But I suppose that's an issue for the spec, so this is fine for now.
(In reply to comment #9) > (From update of attachment 87400 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87400&action=review > > Looks good except for one nit. > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:57 > > + Extensions3D* extensions = getExtensions(); > > getExtensions() should really return a const pointer to reflect the ownership model. Maybe just make it a const here and open a new bug to change the call signature. OK > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:62 > > + m_attrs.depth = true; > > Do you really need to set the depth attr to true? You can still use depth_stencil and just waste the 24 bits of depth if you leave this false. I'm thinking about the different behavior you might get if one implementation has this true and another has it false, since this is an author visible flag. But I suppose that's an issue for the spec, so this is fine for now. I'm not sure where the requirement for stencil=true always implying depth=true comes from. Is it spec or implementation? There was a comment about it in the code. validateAttributes() did this before my change.
(In reply to comment #10) > I'm not sure where the requirement for stencil=true always implying depth=true comes from. Is it spec or implementation? There was a comment about it in the code. validateAttributes() did this before my change. It's an implementation issue. Desktop GL generally doesn't support separate depth and stencil buffers, only packed depth/stencil, and definitely doesn't support having a stencil buffer with no depth buffer. At the spec level, it's legal for the implementation to make changes to these attributes; they're requests, not requirements. Thanks for doing the cleanup.
Comment on attachment 87400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87400&action=review >>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:57 >>> + Extensions3D* extensions = getExtensions(); >> >> getExtensions() should really return a const pointer to reflect the ownership model. Maybe just make it a const here and open a new bug to change the call signature. > > OK There are methods on Extensions3D that are conceptually non-const, like ensureEnabled(), which modify the state of the GL context. It isn't obvious to me that we should change getExtensions() to return const Extensions*.
http://trac.webkit.org/changeset/82335
(In reply to comment #12) > (From update of attachment 87400 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87400&action=review > > >>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:57 > >>> + Extensions3D* extensions = getExtensions(); > >> > >> getExtensions() should really return a const pointer to reflect the ownership model. Maybe just make it a const here and open a new bug to change the call signature. > > > > OK > > There are methods on Extensions3D that are conceptually non-const, like ensureEnabled(), which modify the state of the GL context. It isn't obvious to me that we should change getExtensions() to return const Extensions*. Fair enough, although this class has several API issues. I've opened https://bugs.webkit.org/show_bug.cgi?id=57393 for this.