Bug 57248 – Occlusion issues with WebGL in Safari

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Simon Fraser (smfr)]

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).

[Simon Fraser (smfr)]

Created attachment 87162 [details]
Safari screenshot

[Simon Fraser (smfr)]

Created attachment 87163 [details]
Chrome screenshot

[Kenneth Russell]

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?

[Dean Jackson]

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.

[Dean Jackson]

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.

[Zhenyao Mo]

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.

[Kenneth Russell]

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");

[Dean Jackson]

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.

[Chris Marrin]

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.

[Dean Jackson]

(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.

[Kenneth Russell]

(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.

[Kenneth Russell]

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*.

[Dean Jackson]

http://trac.webkit.org/changeset/82335

[Chris Marrin]

(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.