Bug 49460 – Refactor GL backend flags

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

[Zhenyao Mo]

As suggested by [e-mail], we should use extensions to set GL backend flags instead of virtual functions that go three layers deep.

Flags includes:

isGLES2NPOTStrict
isErrorGeneratedOnOutOfBoundsAccesses
isResourceSafe (newly added for texture/renderbuffer initialization)

[Zhenyao Mo]

Created attachment 73765 [details]
patch

Will remove chromium side flag query functions once this lands and webkit rolls over.

[Kenneth Russell]

Comment on attachment 73765 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73765&action=review

r- because of the need for more documentation of the Chromium-specific extensions.

> WebCore/html/canvas/WebGLRenderingContext.cpp:166
> +    m_isErrorGeneratedOnOutOfBoundsAccesses = m_context->getExtensions()->supports("GL_CHROMIUM_strict_attribs");
> +    m_isResourceSafe = m_context->getExtensions()->supports("GL_CHROMIUM_resource_safe");

These extension names minimally need to be added to WebCore/platform/graphics/chromium/Extensions3DChromium.h, ideally with at least a little documentation of their semantics since they aren't standard, registered extensions.

> WebCore/html/canvas/WebGLRenderingContext.cpp:172
> +        m_isGLES2NPOTStrict = !m_context->getExtensions()->supports("GL_ARB_texture_non_power_of_two");
> +        m_isDepthStencilSupported = m_context->getExtensions()->supports("GL_EXT_packed_depth_stencil");

Are these checks sufficient? These extensions were folded into core OpenGL at some point. I'm concerned that if the GraphicsContext3D implementation actually uses say OpenGL 4.0 that these extensions might not be advertised, so you should probably have a check of the GL version too. You could consider having the Extensions3D interface advertise extensions like GL_VERSION_2_1 if the OpenGL version is 2.1 or greater.

[Zhenyao Mo]

(In reply to comment #2)
> (From update of attachment 73765 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73765&action=review
> 
> r- because of the need for more documentation of the Chromium-specific extensions.
> 
> > WebCore/html/canvas/WebGLRenderingContext.cpp:166
> > +    m_isErrorGeneratedOnOutOfBoundsAccesses = m_context->getExtensions()->supports("GL_CHROMIUM_strict_attribs");
> > +    m_isResourceSafe = m_context->getExtensions()->supports("GL_CHROMIUM_resource_safe");
> 
> These extension names minimally need to be added to WebCore/platform/graphics/chromium/Extensions3DChromium.h, ideally with at least a little documentation of their semantics since they aren't standard, registered extensions.

Will do.

> 
> > WebCore/html/canvas/WebGLRenderingContext.cpp:172
> > +        m_isGLES2NPOTStrict = !m_context->getExtensions()->supports("GL_ARB_texture_non_power_of_two");
> > +        m_isDepthStencilSupported = m_context->getExtensions()->supports("GL_EXT_packed_depth_stencil");
> 
> Are these checks sufficient? These extensions were folded into core OpenGL at some point. I'm concerned that if the GraphicsContext3D implementation actually uses say OpenGL 4.0 that these extensions might not be advertised, so you should probably have a check of the GL version too. You could consider having the Extensions3D interface advertise extensions like GL_VERSION_2_1 if the OpenGL version is 2.1 or greater.

According to the GL spec (up to 4.0),  it is required that an GL implementation to keep exporting extensions in EXTENSIONS even after they are folded into the core.  Also, the functions names with EXT or ARB should still be valid.  Therefore, relying on the extensions should be enough.  Also, our code also assumes the EXT/ARB function names and enum names.  If a future GL implementation starts to remove them, we need to do code refactoring for quite a few places, including this.

Please correct me if my reading/understanding of the spec is incorrect.  Otherwise I'll fix the first issue and upload a new patch.

[Kenneth Russell]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 73765 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=73765&action=review
> > 
> > r- because of the need for more documentation of the Chromium-specific extensions.
> > 
> > > WebCore/html/canvas/WebGLRenderingContext.cpp:166
> > > +    m_isErrorGeneratedOnOutOfBoundsAccesses = m_context->getExtensions()->supports("GL_CHROMIUM_strict_attribs");
> > > +    m_isResourceSafe = m_context->getExtensions()->supports("GL_CHROMIUM_resource_safe");
> > 
> > These extension names minimally need to be added to WebCore/platform/graphics/chromium/Extensions3DChromium.h, ideally with at least a little documentation of their semantics since they aren't standard, registered extensions.
> 
> Will do.
> 
> > 
> > > WebCore/html/canvas/WebGLRenderingContext.cpp:172
> > > +        m_isGLES2NPOTStrict = !m_context->getExtensions()->supports("GL_ARB_texture_non_power_of_two");
> > > +        m_isDepthStencilSupported = m_context->getExtensions()->supports("GL_EXT_packed_depth_stencil");
> > 
> > Are these checks sufficient? These extensions were folded into core OpenGL at some point. I'm concerned that if the GraphicsContext3D implementation actually uses say OpenGL 4.0 that these extensions might not be advertised, so you should probably have a check of the GL version too. You could consider having the Extensions3D interface advertise extensions like GL_VERSION_2_1 if the OpenGL version is 2.1 or greater.
> 
> According to the GL spec (up to 4.0),  it is required that an GL implementation to keep exporting extensions in EXTENSIONS even after they are folded into the core.  Also, the functions names with EXT or ARB should still be valid.  Therefore, relying on the extensions should be enough.  Also, our code also assumes the EXT/ARB function names and enum names.  If a future GL implementation starts to remove them, we need to do code refactoring for quite a few places, including this.
> 
> Please correct me if my reading/understanding of the spec is incorrect.  Otherwise I'll fix the first issue and upload a new patch.

As long as you've double-checked this in the spec it sounds fine. Please go ahead and upload a revised patch.

[Zhenyao Mo]

Created attachment 74014 [details]
revised patch

register extensions in Extensions3D.h and Extensions3DChromium.h

[Zhenyao Mo]

Created attachment 74015 [details]
revised patch

Fix some comment issue and remove an extra empty line

[Kenneth Russell]

Comment on attachment 74015 [details]
revised patch

Looks good.

[Zhenyao Mo]

Committed r72124: <http://trac.webkit.org/changeset/72124>