Bug 33805 – Must implement OpenGL ES 2.0 semantics for NPOT textures

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

[Chris Marrin]

The OpenGL ES 2.0 does not allow mipmapping of NPOT textures. Most desktop OpenGL implementations do. We need to implement the rules specified in the OpenGL ES 2.0 spec, section 3.8.2.

[Alice Liu]

<rdar://problem/7774338>

[Kenneth Russell]

In addition to the mipmapping restrictions, OpenGL ES 2.0 (and therefore WebGL) requires that the repeat mode CLAMP_TO_EDGE be used for NPOT textures. Per the OpenGL ES 2.0 specification section 3.8.2, if for a NPOT texture the minification filter is not GL_NEAREST or GL_LINEAR, or the clamp mode is not CLAMP_TO_EDGE, then sampling the texture in a shader results in (R, G, B, A) = (0, 0, 0, 1). glGenerateMipmaps for a NPOT texture also needs to produce INVALID_OPERATION.

[Zhenyao Mo]

Created attachment 54229 [details]
patch

[WebKit Review Bot]

Attachment 54229 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/html/canvas/WebGLRenderingContext.cpp:472:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/WebGLRenderingContext.cpp:1414:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/html/canvas/WebGLRenderingContext.cpp:1909:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/WebGLRenderingContext.cpp:1981:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/WebGLRenderingContext.cpp:2060:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/WebGLRenderingContext.cpp:2140:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/WebGLTexture.cpp:83:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/WebGLTexture.cpp:161:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/WebGLTexture.cpp:203:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/WebGLTexture.cpp:205:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 10 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

(In reply to comment #3)
> Created an attachment (id=54229) [details]
> patch

Zhenyao you can test for style correctness yourself by running the check-webkit-style script, it may save some time waiting for the style bot to catch up :D

[Zhenyao Mo]

Created attachment 54230 [details]
revised patch: fix style issues

[Zhenyao Mo]

Created attachment 54333 [details]
revised patch: fix a tiny issue

fallback color should be [0, 0, 0, 1] instead of [0, 0, 0, 0] in the original conformance test we copied.

[Zhenyao Mo]

(In reply to comment #5)
> (In reply to comment #3)
> > Created an attachment (id=54229) [details] [details]
> > patch
> 
> Zhenyao you can test for style correctness yourself by running the
> check-webkit-style script, it may save some time waiting for the style bot to
> catch up :D

Thanks for the info.  Sometimes I just get over-confident about my coding style so I unwisely skip the style check. :)

[Zhenyao Mo]

Created attachment 55162 [details]
revised patch

[Zhenyao Mo]

Created attachment 55191 [details]
revised patch: responding to Gregg Tavares comments

The following comments are from Gregg Tavares:

"""
at line 773 you only check m_activeTextureUnit.  You actually have to loop over all the texture units as in

for (GLint ii = 0; ii < max_active_texture_units; ++ii) {
  RefPtr<WebGLTexture> tex2D = m_textureUnits[ii].m_texture2DBinding;

Then, if the texture needs binding you have to call glActiveTexture to make that the active texture unit before you call glBindTexture

Finally, after you call glDraw and you have to loop again, restore all the textures with a similar loop and finally call glActiveTexture to set it back to what it was before you entered this function.
"""

[Kenneth Russell]

Comment on attachment 55191 [details]
revised patch: responding to Gregg Tavares comments

This is good work, but I think it needs some more work before committing.


WebCore/html/canvas/WebGLRenderingContext.cpp:109
 +          addObject(m_blackTextureCubeMap.get());
I think the logic for setting up the 1x1 black texture should be in a private method of the WebGLRenderingContext, not in the WebGLTexture class. That way you can use WebGLRenderingContext::createTexture() to allocate the object and you don't need the manual calls to addObject, which are error prone.


WebCore/html/canvas/WebGLRenderingContext.cpp:458
 +              tex->setSize(target, width, height);
This line should not be executed if the texture is a cube map and the width and height are unequal. That is supposed to yield INVALID_VALUE and the texture's state should remain untouched. Consult Chromium's command buffer service code (TextureManager::ValidForTarget, GLES2DecoderImpl::DoTexImage2D).

WebCore/html/canvas/WebGLRenderingContext.cpp:1833
 +              tex->setSize(target, width, height);
Ditto.

WebCore/html/canvas/WebGLRenderingContext.cpp:3089
 +  }
These two routines contain nearly identical code. Please refactor them into one method taking a boolean argument. Perhaps "handleNPOTTextures(bool preparingForDraw)". Also please add FIXME around use of constant 32.

WebCore/html/canvas/WebGLTexture.cpp:87
 +  void WebGLTexture::setDimensionality(unsigned long dimensionality)
"Dimensionality" is a confusing choice of word. Since everywhere else in the OpenGL spec this concept is "target", please use that term instead.


WebCore/html/canvas/WebGLTexture.cpp:165
 +          if (m_dimensionality == GraphicsContext3D::TEXTURE_CUBE_MAP)
Please factor this "if" clause out of the switch statement.

WebCore/html/canvas/WebGLTexture.cpp:199
 +          return false;
I'm not sure this short circuit is correct. At least it is inconsistent with the command buffer implementation. I think this should read

if (width && (width & (width - 1)))
  return true;

Similarly for height.

WebCore/html/canvas/WebGLTexture.cpp:221
 +              // We only set NPOT for complete cube map textures.
Why is this the case? The command buffer implementation also tracks cube map completeness and manually switches in a black texture if the texture is not cube map complete. I don't think the desktop GL spec has the same guarantees that the OpenGL ES 2.0 spec does in this area (See the OpenGL 2.1 spec, section 3.8.10 "Texture Completeness", subsection "Effects of Completeness on Texture Application") so I think you need all of the logic.


LayoutTests/fast/canvas/webgl/texture-npot.html:85
 +                            canvas2d);         // internalFormat
Incorrect comment.


LayoutTests/fast/canvas/webgl/texture-npot.html:92
 +                            canvas2d);         // internalFormat
Incorrect comment.


LayoutTests/fast/canvas/webgl/texture-npot.html:138
 +                    //      buf[offset + 3]);
Please delete commented out code.

[Zhenyao Mo]

(In reply to comment #11)
> (From update of attachment 55191 [details])
> This is good work, but I think it needs some more work before committing.
> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:109
>  +          addObject(m_blackTextureCubeMap.get());
> I think the logic for setting up the 1x1 black texture should be in a private method of the WebGLRenderingContext, not in the WebGLTexture class. That way you can use WebGLRenderingContext::createTexture() to allocate the object and you don't need the manual calls to addObject, which are error prone.
Done.

> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:458
>  +              tex->setSize(target, width, height);
> This line should not be executed if the texture is a cube map and the width and height are unequal. That is supposed to yield INVALID_VALUE and the texture's state should remain untouched. Consult Chromium's command buffer service code (TextureManager::ValidForTarget, GLES2DecoderImpl::DoTexImage2D).
Done inside setSize().

> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:1833
>  +              tex->setSize(target, width, height);
> Ditto.
> 
Ditto.

> WebCore/html/canvas/WebGLRenderingContext.cpp:3089
>  +  }
> These two routines contain nearly identical code. Please refactor them into one method taking a boolean argument. Perhaps "handleNPOTTextures(bool preparingForDraw)". Also please add FIXME around use of constant 32.
> 
Done.

> WebCore/html/canvas/WebGLTexture.cpp:87
>  +  void WebGLTexture::setDimensionality(unsigned long dimensionality)
> "Dimensionality" is a confusing choice of word. Since everywhere else in the OpenGL spec this concept is "target", please use that term instead.
> 
Done.

> 
> WebCore/html/canvas/WebGLTexture.cpp:165
>  +          if (m_dimensionality == GraphicsContext3D::TEXTURE_CUBE_MAP)
> Please factor this "if" clause out of the switch statement.
Done.

> 
> WebCore/html/canvas/WebGLTexture.cpp:199
>  +          return false;
> I'm not sure this short circuit is correct. At least it is inconsistent with the command buffer implementation. I think this should read
> 
> if (width && (width & (width - 1)))
>   return true;
> 
> Similarly for height.
Per discussion, we won't raise NPOT for incomplete textures.  Desktop GL will handle those case.

> 
> WebCore/html/canvas/WebGLTexture.cpp:221
>  +              // We only set NPOT for complete cube map textures.
> Why is this the case? The command buffer implementation also tracks cube map completeness and manually switches in a black texture if the texture is not cube map complete. I don't think the desktop GL spec has the same guarantees that the OpenGL ES 2.0 spec does in this area (See the OpenGL 2.1 spec, section 3.8.10 "Texture Completeness", subsection "Effects of Completeness on Texture Application") so I think you need all of the logic.
Per discussion, we won't raise NPOT for incomplete textures.  Desktop GL will handle those case.

> 
> 
> LayoutTests/fast/canvas/webgl/texture-npot.html:85
>  +                            canvas2d);         // internalFormat
> Incorrect comment.
Done.

> 
> 
> LayoutTests/fast/canvas/webgl/texture-npot.html:92
>  +                            canvas2d);         // internalFormat
> Incorrect comment.
Done (need to fix this in webgl conformance text also).

> 
> 
> LayoutTests/fast/canvas/webgl/texture-npot.html:138
>  +                    //      buf[offset + 3]);
> Please delete commented out code.
Done.

[Zhenyao Mo]

Created attachment 55789 [details]
revised patch : responding to Ken Russell's review

[Zhenyao Mo]

Created attachment 55790 [details]
revised patch: tiny modification

After setting up black 1x1 textures, need to unbind them.

[Kenneth Russell]

Comment on attachment 55790 [details]
revised patch: tiny modification

Looks good. Thanks for doing a thorough job on this.

[Dimitri Glazkov (Google)]

Comment on attachment 55790 [details]
revised patch: tiny modification

rs=me.

[WebKit Commit Bot]

Comment on attachment 55790 [details]
revised patch: tiny modification

Clearing flags on attachment: 55790

Committed r59470: <http://trac.webkit.org/changeset/59470>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/59470 might have broken Leopard Intel Debug (Tests)

[Eric Seidel (no email)]

The sherriff is right. This broke leopard.   Zmo: please advise.

[Kenneth Russell]

Test results linked from here:

http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r59472%20(14407)/results.html

[Zhenyao Mo]

The NPOT test is failing on a bot.  Reopen this to investigate the cause.

[Zhenyao Mo]

Created attachment 56090 [details]
patch

Removed the test cases that failed on leopard bot (temporarily).
Created another bug to track this: https://bugs.webkit.org/show_bug.cgi?id=39128

[Eric Seidel (no email)]

Comment on attachment 56090 [details]
patch

OK.  CQ won't work for this since the tree is red.  But I"ll land manually.

[Eric Seidel (no email)]

Comment on attachment 56090 [details]
patch

Clearing flags on attachment: 56090

Committed r59479: <http://trac.webkit.org/changeset/59479>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.