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

Source/core/html/canvas/WebGLTexture.cpp

 /*
  * Copyright (C) 2009 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 267 matching lines @@
     if (isNPOT())
         return false;
     const LevelInfo& first = m_info[0][0];
     for (size_t ii = 0; ii < m_info.size(); ++ii) {
         const LevelInfo& info = m_info[ii][0];
         if (!info.valid
             || info.width != first.width || info.height != first.height
             || info.internalFormat != first.internalFormat || info.type != first.type)
             return false;
     }
     return true;

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

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.

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

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.  

 }
 
 GC3Dint WebGLTexture::computeLevelCount(GC3Dsizei width, GC3Dsizei height)
 {
     // return 1 + log2Floor(std::max(width, height));
     GC3Dsizei n = std::max(width, height);
     if (n <= 0)
         return 0;
     GC3Dint log = 0;
     GC3Dsizei value = n;
@@ skipping 12 matching lines @@
 void WebGLTexture::update()
 {
     m_isNPOT = false;
     for (size_t ii = 0; ii < m_info.size(); ++ii) {
         if (isNPOT(m_info[ii][0].width, m_info[ii][0].height)) {
             m_isNPOT = true;
             break;
         }
     }
     m_isComplete = true;
+    bool isCubeComplete = true;

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

Please make isCubeComplete a member of the WebGLTexture class
(m_isCubeComplete). It needs to be checked in at least one other place.

     const LevelInfo& first = m_info[0][0];
     GC3Dint levelCount = computeLevelCount(first.width, first.height);
     if (levelCount < 1)
         m_isComplete = false;
     else {
         for (size_t ii = 0; ii < m_info.size() && m_isComplete; ++ii) {
             const LevelInfo& info0 = m_info[ii][0];
             if (!info0.valid
                 || info0.width != first.width || info0.height != first.height
-                || info0.internalFormat != first.internalFormat || info0.type != first.type) {
+                || info0.internalFormat != first.internalFormat || info0.type != first.type
+                || (m_info.size() > 1 && info0.width != info0.height)) {
+                if (m_info.size() > 1)
+                    isCubeComplete = false;
                 m_isComplete = false;
                 break;
             }
             GC3Dsizei width = info0.width;
             GC3Dsizei height = info0.height;
             for (GC3Dint level = 1; level < levelCount; ++level) {
                 width = std::max(1, width >> 1);
                 height = std::max(1, height >> 1);
                 const LevelInfo& info = m_info[ii][level];
                 if (!info.valid
                     || info.width != width || info.height != height
                     || info.internalFormat != info0.internalFormat || info.type != info0.type) {
                     m_isComplete = false;
                     break;
                 }
 
             }
         }
     }
-    m_isFloatType = false;
-    if (m_isComplete)
-        m_isFloatType = m_info[0][0].type == GraphicsContext3D::FLOAT;
-    else {
-        for (size_t ii = 0; ii < m_info.size(); ++ii) {
-            if (m_info[ii][0].type == GraphicsContext3D::FLOAT) {
-                m_isFloatType = true;
-                break;
-            }
-        }
-    }
-    m_isHalfFloatType = false;
-    if (m_isComplete)
-        m_isHalfFloatType = m_info[0][0].type == GraphicsContext3D::HALF_FLOAT_OES;
-    else {
-        for (size_t ii = 0; ii < m_info.size(); ++ii) {
-            if (m_info[ii][0].type == GraphicsContext3D::HALF_FLOAT_OES) {
-                m_isHalfFloatType = true;
-                break;
-            }
-        }
-    }
+    m_isFloatType = m_info[0][0].type == GraphicsContext3D::FLOAT;
+    m_isHalfFloatType = m_info[0][0].type == GraphicsContext3D::HALF_FLOAT_OES;

[bajones, 2013/06/05 17:29:00]

This feels unrelated to the cubemap change. What's the reason for changing this
logic?

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

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.

 
     m_needToUseBlackTexture = false;
     // NPOT
     if (m_isNPOT && ((m_minFilter != GraphicsContext3D::NEAREST && m_minFilter != GraphicsContext3D::LINEAR)
                      || m_wrapS != GraphicsContext3D::CLAMP_TO_EDGE || m_wrapT != GraphicsContext3D::CLAMP_TO_EDGE))
         m_needToUseBlackTexture = true;
+    // If it is a Cube texture, check Cube Completeness first
+    if (m_info.size() > 1 && !isCubeComplete)
+        m_needToUseBlackTexture = true;
     // Completeness
     if (!m_isComplete && m_minFilter != GraphicsContext3D::NEAREST && m_minFilter != GraphicsContext3D::LINEAR)
         m_needToUseBlackTexture = true;
 }
 
 const WebGLTexture::LevelInfo* WebGLTexture::getLevelInfo(GC3Denum target, GC3Dint level) const
 {
     if (!object() || !m_target)
         return 0;
     int targetIndex = mapTargetToIndex(target);
     if (targetIndex < 0 || targetIndex >= static_cast<int>(m_info.size()))
         return 0;
     if (level < 0 || level >= static_cast<GC3Dint>(m_info[targetIndex].size()))
         return 0;
     return &(m_info[targetIndex][level]);
 }
 
 }