Bug 49460 - Refactor GL backend flags
Summary: Refactor GL backend flags
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks: 49355
  Show dependency treegraph
 
Reported: 2010-11-12 11:16 PST by Zhenyao Mo
Modified: 2010-11-16 11:31 PST (History)
4 users (show)

See Also:


Attachments
patch (12.49 KB, patch)
2010-11-12 11:30 PST, Zhenyao Mo
kbr: review-
zmo: commit-queue-
Details | Formatted Diff | Diff
revised patch (13.98 KB, patch)
2010-11-16 11:05 PST, Zhenyao Mo
zmo: commit-queue-
Details | Formatted Diff | Diff
revised patch (13.93 KB, patch)
2010-11-16 11:08 PST, Zhenyao Mo
kbr: review+
zmo: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhenyao Mo 2010-11-12 11:16:16 PST
As suggested by gman@google.com, 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)
Comment 1 Zhenyao Mo 2010-11-12 11:30:49 PST
Created attachment 73765 [details]
patch

Will remove chromium side flag query functions once this lands and webkit rolls over.
Comment 2 Kenneth Russell 2010-11-12 18:13:07 PST
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.
Comment 3 Zhenyao Mo 2010-11-16 10:33:33 PST
(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.
Comment 4 Kenneth Russell 2010-11-16 10:38:10 PST
(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.
Comment 5 Zhenyao Mo 2010-11-16 11:05:22 PST
Created attachment 74014 [details]
revised patch

register extensions in Extensions3D.h and Extensions3DChromium.h
Comment 6 Zhenyao Mo 2010-11-16 11:08:24 PST
Created attachment 74015 [details]
revised patch

Fix some comment issue and remove an extra empty line
Comment 7 Kenneth Russell 2010-11-16 11:24:33 PST
Comment on attachment 74015 [details]
revised patch

Looks good.
Comment 8 Zhenyao Mo 2010-11-16 11:31:07 PST
Committed r72124: <http://trac.webkit.org/changeset/72124>