Bug 42908 – WebGL must enforce restrictions even if running on OpenGL ES 2.0

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

[Gregg Tavares]

The current Webkit implementation of WebGL checks if the underlying platform is OpenGL ES 2.0 and if so stops checking certain WebGL requirements.

The issue is nearly all implementations of OpenGL ES 2.0 support various extensions, for example

GL_OES_texture_npot         makes npot textures work in all situations
GL_OES_element_index_uint   makes it possible to pass GL_INT to drawElement
GL_OES_texture_float        makes it possible to pass GL_FLOAT to texImage2D

WebGL 1.0 is supposed to enforce the restriction these features do not work unless some specific WebGL extension enables the feature.

[Zhenyao Mo]

Created attachment 65012 [details]
patch

Compiled and tested in Safari and Chromium.

[Kenneth Russell]

Comment on attachment 65012 [details]
patch

After looking at these changes I do not think that this is the right direction to go. Please see below and reply with your thoughts.


WebCore/html/canvas/WebGLRenderingContext.cpp:306
 +      if ((!isGLES2Compliant() || !isGLES2NPOTStrict()) && texture)
This looks like a disaster waiting to happen. If we have multiple guards for certain of the tracking and verification logic how are we going to keep it straight? I think it would be better to just always do the tracking logic regardless of whether we think we are running on OpenGL ES 2.0, because in the presence of extensions (including ones that are exposed via WebGL) some of this logic will be enabled once the user enables an extension.


WebCore/html/canvas/WebGLRenderingContext.cpp:542
 +      if (!isGLES2Compliant() || !isGLES2NPOTStrict())
Same comment here.


WebCore/html/canvas/WebGLRenderingContext.cpp:975
 +      if (!isGLES2Compliant() || !isErrorGeneratedOnOutOfBoundsAccesses()) {
Same comment here.


WebCore/html/canvas/WebGLRenderingContext.cpp:1130
 +      if (!isGLES2NPOTStrict() || !isGLES2Compliant()) {
Same here.


WebCore/html/canvas/WebGLRenderingContext.cpp:1137
 +      if (!isGLES2Compliant() || !isGLES2Compliant())
This is a flat out bug.


WebCore/html/canvas/WebGLRenderingContext.cpp:2108
 +      if (!isGLES2Compliant() || !isGLES2NPOTStrict())
Here too.


WebCore/html/canvas/WebGLRenderingContext.cpp:2222
 +      if (!isGLES2ParameterStrict() || !isGLES2NPOTStrict()) {
Here too.

[Zhenyao Mo]

Seems like if we always track NPOT and texture complete-ness in WebGLRenderingContext and remove the isGLES2NPOTStrict() flag, it will clean up most things.  If we do that, command buffer should not turn this on to avoid double efforts.

For isGLES2ParameterStrict(), we might just remove the flag and always do the checking.  The overhead is trivia, and the code is much cleaner.

For OutOfBoundsAccesses, I think we should keep the flag.  If command buffer implements the stricter restriction, we can turn it off on the WebGLRenderingContext side because Gregg is uncomfortable about totally turning it off on the command buffer side.

Let me know what you think then I can finish this.

Mo

(In reply to comment #2)
> (From update of attachment 65012 [details])
> After looking at these changes I do not think that this is the right direction to go. Please see below and reply with your thoughts.
> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:306
>  +      if ((!isGLES2Compliant() || !isGLES2NPOTStrict()) && texture)
> This looks like a disaster waiting to happen. If we have multiple guards for certain of the tracking and verification logic how are we going to keep it straight? I think it would be better to just always do the tracking logic regardless of whether we think we are running on OpenGL ES 2.0, because in the presence of extensions (including ones that are exposed via WebGL) some of this logic will be enabled once the user enables an extension.
> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:542
>  +      if (!isGLES2Compliant() || !isGLES2NPOTStrict())
> Same comment here.
> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:975
>  +      if (!isGLES2Compliant() || !isErrorGeneratedOnOutOfBoundsAccesses()) {
> Same comment here.
> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:1130
>  +      if (!isGLES2NPOTStrict() || !isGLES2Compliant()) {
> Same here.
> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:1137
>  +      if (!isGLES2Compliant() || !isGLES2Compliant())
> This is a flat out bug.
> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:2108
>  +      if (!isGLES2Compliant() || !isGLES2NPOTStrict())
> Here too.
> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:2222
>  +      if (!isGLES2ParameterStrict() || !isGLES2NPOTStrict()) {
> Here too.

[Kenneth Russell]

(In reply to comment #3)
> Seems like if we always track NPOT and texture complete-ness in WebGLRenderingContext and remove the isGLES2NPOTStrict() flag, it will clean up most things.  If we do that, command buffer should not turn this on to avoid double efforts.

It is likely that WebGL will expose the OES_texture_npot extension at some point, which means that the NPOT checks in WebGLRenderingContext will need to be disabled if that extension is supported and was enabled at the WebGL level. The command buffer code will advertise the availability of OES_texture_npot if the underlying hardware supports it, and enforce the limited NPOT restrictions if not. Either way, I do not see an opportunity to disable the checks in the command buffer code. I agree that we should basically always track the NPOT and texture completeness state in WebGLRenderingContext. With suitable optimization these checks should not be that expensive. (The common case going forward is that WebGLRenderingContext will have to do the checks, since desktop GL and most OpenGL ES 2.0 implementations will support unrestricted NPOT textures.)

> For isGLES2ParameterStrict(), we might just remove the flag and always do the checking.  The overhead is trivia, and the code is much cleaner.

Agree.

> For OutOfBoundsAccesses, I think we should keep the flag.  If command buffer implements the stricter restriction, we can turn it off on the WebGLRenderingContext side because Gregg is uncomfortable about totally turning it off on the command buffer side.

Agree that we must not turn off the validation on the command buffer side. It would be ideal if the command buffer code had an option to implement the full WebGL index checking semantics. It's fine with me to leave that flag in place at this point because the checks are relatively expensive.

[Zhenyao Mo]

Created attachment 65473 [details]
revised patch

[Zhenyao Mo]

Created attachment 65476 [details]
revised patch: fixing an issue in WebCore/ChangeLog

It was introduced during rebasing

[Kenneth Russell]

Comment on attachment 65476 [details]
revised patch: fixing an issue in WebCore/ChangeLog

Thanks for making these changes. This looks much better to me. r+

[Zhenyao Mo]

Committed r66055: <http://trac.webkit.org/changeset/66055>