Add check on Cube Completeness for Cube textures and improve the efficiency of checks associated wi…

Add check on Cube Completeness for Cube textures and improve the efficiency of checks associated with OES_texture_{half_}float_linear extension.

BUG=246815
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152167

[Jun Jiang, 2013-06-05 13:27:24]

According to OpenGL ES Spec 2.0.25 section 3.8.2, calling a sampler from a
fragment shader will return (R; G;B;A) = (0; 0; 0; 1) when either the
corresponding cube map texture image is not cube complete, or TEXTURE_MIN_FILTER
is one that requires a mipmap and the texture is not mipmap cube complete.

Current implementation only considered the second condition and ignores the
first one. This patch adds the check for Cube Completeness and optimize the
check related with OES_texture_{half_}float_linear extension.

[bajones, 2013-06-05 17:28:59]

I'm not an owner, but LGTM with one question.

https://codereview.chromium.org/16437002/diff/1/Source/core/html/canvas/WebGL...
File Source/core/html/canvas/WebGLTexture.cpp (right):

https://codereview.chromium.org/16437002/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLTexture.cpp:355: m_isHalfFloatType =
m_info[0][0].type == GraphicsContext3D::HALF_FLOAT_OES;
This feels unrelated to the cubemap change. What's the reason for changing this
logic?

[Jun Jiang, 2013-06-06 01:19:48]

Hi, bajones. Thanks for your comments. Following is my understanding on this.

https://codereview.chromium.org/16437002/diff/1/Source/core/html/canvas/WebGL...
File Source/core/html/canvas/WebGLTexture.cpp (right):

https://codereview.chromium.org/16437002/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLTexture.cpp:355: m_isHalfFloatType =
m_info[0][0].type == GraphicsContext3D::HALF_FLOAT_OES;
On 2013/06/05 17:29:00, bajones wrote:
> This feels unrelated to the cubemap change. What's the reason for changing
this
> logic?

This change is to optimize the check on whether the texture type is GL_FLOAT or
GL_HALF_FLOAT, which will be used to judge whether RGBA(0,0,0,1) should be
returned when Linear filter is used.

As Kenneth mentioned in https://codereview.chromium.org/14860016/, it is not
necessary to scan all the faces of the Cube to check the type and checking only
the first is enough. If two faces have different types, it is not Cube complete
and m_needToUseBlackTexture will be true. If all faces have the same type, it
may be redundant to check all the faces.

When adding the check for Cube completeness, I optimize the check associated
with the OES_texture_{half_}float_linear extension as well instead of fire a
separate bug.

[Ken Russell (switch to Gerrit), 2013-06-06 23:16:43]

Thanks for fixing this.

LGTM with one additional fix. It looks like the WebGL test
conformance/textures/texture-npot.html needs to check this case; would you mind
submitting a pull request enhancing that test?

https://codereview.chromium.org/16437002/diff/1/Source/core/html/canvas/WebGL...
File Source/core/html/canvas/WebGLTexture.cpp (right):

https://codereview.chromium.org/16437002/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLTexture.cpp:288: return true;
This won't return the correct answer for a cube map texture where all the faces
have the same width and height, but width != height. This will cause a cube map
texture to not be cube complete. This method needs to check if the texture is a
cube map texture, and if so, return false if the new member m_isCubeComplete is
false.

https://codereview.chromium.org/16437002/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLTexture.cpp:321: bool isCubeComplete = true;
Please make isCubeComplete a member of the WebGLTexture class
(m_isCubeComplete). It needs to be checked in at least one other place.

[Jun Jiang, 2013-06-07 13:58:22]

Hi, Kenneth. Thanks for your comments. For the case that width is not equal to
height for Cube texture, a INVALID_VALUE error will return earlier when the
texture is defined. To make the code easier to read, I refined patch for review.
And there may be no adding case to conformance/texture/texture-npot.html.

https://codereview.chromium.org/16437002/diff/1/Source/core/html/canvas/WebGL...
File Source/core/html/canvas/WebGLTexture.cpp (right):

https://codereview.chromium.org/16437002/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLTexture.cpp:288: return true;
On 2013/06/06 23:16:43, kbr wrote:
> This won't return the correct answer for a cube map texture where all the
faces
> have the same width and height, but width != height. This will cause a cube
map
> texture to not be cube complete. This method needs to check if the texture is
a
> cube map texture, and if so, return false if the new member m_isCubeComplete
is
> false.

The code here is a little misleading but in my understanding, it may not
introduce any issues. 

When the Cube texture is defined via texImage2D(), it must pass the validation
check on the parameters,
namely,WebGLRenderingContext::validateTexFuncParameters(), in which will check
if width equals height for Cube texture target(TEXTURE_CUBE_MAP_POSITIVE_X, etc)
and will return INVALID_VALUE when it doesn't. The texture is still in the
invalid state. In WebGLRenderingContext::generateMipmap(), it can't pass the
validateTextureBinding() check and return early. Even when it call
canGenerateMipmaps() directly, the Cube texture is not valid and will return
false.

So whether width equals height is validated when the Cube texture is defined. To
make the code more clear and easy to maintain, I'd like to add the check here as
well.

[Ken Russell (switch to Gerrit), 2013-06-10 19:54:03]

Thanks for pointing out that the TexImage2D call will actually be rejected for
cube map faces if the width and height aren't equal.

The new code LGTM even if the checks are a little redundant or unnecessary.

Submitting to CQ.

[commit-bot: I haz the power, 2013-06-10 19:54:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jun.a.jiang@intel.com/16437002/28001

[commit-bot: I haz the power, 2013-06-10 23:24:46]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-06-10 23:38:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jun.a.jiang@intel.com/16437002/28001

[commit-bot: I haz the power, 2013-06-11 00:23:56]

Change committed as 152167