Enable CopyTextureCHROMIUM in Blink for Tex{Sub}Image2D with more cases

third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp

Index: third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
diff --git a/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp b/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
index 0960615b9382ddd8515f3daff969a8b748bc5cfc..11e16fbb2f569c48b37537f64cefbbe242addd5f 100644
--- a/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
+++ b/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
@@ -818,9 +818,13 @@ sk_sp<SkImage> WebGLRenderingContextBase::makeImageSnapshot(
                          surface->getTextureHandle(
                              SkSurface::kDiscardWrite_TextureHandleAccess))
                          ->fID;
+  GLenum textureTarget = skia::GrBackendObjectToGrGLTextureInfo(
+                             surface->getTextureHandle(
+                                 SkSurface::kDiscardWrite_TextureHandleAccess))

[Zhenyao Mo, 2017/03/10 00:10:24]

Can we cache this and reuse it for both textureId and textureTarget?

[qiankun, 2017/03/10 02:48:04]

Done.

+                             ->fTarget;
 
   drawingBuffer()->copyToPlatformTexture(
-      gl, textureId, GL_RGBA, GL_UNSIGNED_BYTE, 0, true, false, IntPoint(0, 0),
+      gl, textureTarget, textureId, true, false, IntPoint(0, 0),
       IntRect(IntPoint(0, 0), drawingBuffer()->size()), BackBuffer);
   return surface->makeImageSnapshot();
 }
@@ -984,64 +988,6 @@ static const GLenum kSupportedTypesTexImageSourceES3[] = {
     GL_HALF_FLOAT, GL_FLOAT, GL_UNSIGNED_INT_10F_11F_11F_REV,
 };
 
-bool isUnsignedIntegerFormat(GLenum internalformat) {
-  switch (internalformat) {
-    case GL_R8UI:
-    case GL_R16UI:
-    case GL_R32UI:
-    case GL_RG8UI:
-    case GL_RG16UI:
-    case GL_RG32UI:
-    case GL_RGB8UI:
-    case GL_RGB16UI:
-    case GL_RGB32UI:
-    case GL_RGBA8UI:
-    case GL_RGB10_A2UI:
-    case GL_RGBA16UI:
-    case GL_RGBA32UI:
-      return true;
-    default:
-      return false;
-  }
-}
-
-bool isSignedIntegerFormat(GLenum internalformat) {
-  switch (internalformat) {
-    case GL_R8I:
-    case GL_R16I:
-    case GL_R32I:
-    case GL_RG8I:
-    case GL_RG16I:
-    case GL_RG32I:
-    case GL_RGB8I:
-    case GL_RGB16I:
-    case GL_RGB32I:
-    case GL_RGBA8I:
-    case GL_RGBA16I:
-    case GL_RGBA32I:
-      return true;
-    default:
-      return false;
-  }
-}
-
-bool isIntegerFormat(GLenum internalformat) {
-  return (isUnsignedIntegerFormat(internalformat) ||
-          isSignedIntegerFormat(internalformat));
-}
-
-bool isFloatType(GLenum type) {
-  switch (type) {
-    case GL_FLOAT:
-    case GL_HALF_FLOAT:
-    case GL_HALF_FLOAT_OES:
-    case GL_UNSIGNED_INT_10F_11F_11F_REV:
-      return true;
-    default:
-      return false;
-  }
-}
-
 bool isSRGBFormat(GLenum internalformat) {
   switch (internalformat) {
     case GL_SRGB_EXT:
@@ -4953,17 +4899,26 @@ bool WebGLRenderingContextBase::canUseTexImageByGPU(
     TexImageFunctionID functionID,
     GLint internalformat,
     GLenum type) {
-  if (functionID == TexImage2D &&
-      (isFloatType(type) || isIntegerFormat(internalformat) ||
-       isSRGBFormat(internalformat)))
+  // SRGB format has color-space conversion issue, see
+  // https://github.com/KhronosGroup/WebGL/issues/2165.
+  if (isSRGBFormat(internalformat))
+    return false;
+  // Before we can detect internalformat in TexSubImage2D, we disable GPU path
+  // if EXT_sRGB extension is enabled.
+  if (functionID == TexSubImage2D && extensionEnabled(EXTsRGBName))

[Zhenyao Mo, 2017/03/10 00:10:24]

This is a big fallback.  Can we look into lifting this?

[qiankun, 2017/03/10 02:48:04]

When we get clear on how to handle sRGB texture, we can think about lifting
this. Add a TODO here.

     return false;
-  // TODO(crbug.com/622958): Implement GPU-to-GPU path for WebGL 2 and more
-  // internal formats.
-  if (functionID == TexSubImage2D &&
-      (isWebGL2OrHigher() || extensionEnabled(OESTextureFloatName) ||
-       extensionEnabled(OESTextureHalfFloatName) ||
-       extensionEnabled(EXTsRGBName)))
+#if OS(MACOSX)
+  // RGB5_A1 is not color-renderable on NVIDIA Mac, see crbug.com/676209.
+  if (type == GL_UNSIGNED_SHORT_5_5_5_1)
     return false;
+#endif
+  // OES_texture_half_float doesn't support HALF_FLOAT_OES type for
+  // CopyTexImage/CopyTexSubImage. And OES_texture_half_float doesn't require
+  // HALF_FLOAT_OES type texture to be renderable. So, HALF_FLOAT_OES type
+  // texture cannot be copied to or drawn to by glCopyTextureCHROMIUM.
+  if (type == GL_HALF_FLOAT_OES)
+    return false;
+
   return true;
 }
 
@@ -4986,10 +4941,8 @@ SnapshotReason WebGLRenderingContextBase::functionIDToSnapshotReason(
 void WebGLRenderingContextBase::texImageCanvasByGPU(
     TexImageFunctionID functionID,
     HTMLCanvasElement* canvas,
+    GLenum target,
     GLuint targetTexture,
-    GLenum targetInternalformat,
-    GLenum targetType,
-    GLint targetLevel,
     GLint xoffset,
     GLint yoffset,
     const IntRect& sourceSubRectangle) {
@@ -4997,10 +4950,9 @@ void WebGLRenderingContextBase::texImageCanvasByGPU(
     ImageBuffer* buffer = canvas->buffer();
     if (buffer &&
         !buffer->copyToPlatformTexture(
-            functionIDToSnapshotReason(functionID), contextGL(), targetTexture,
-            targetInternalformat, targetType, targetLevel,
-            m_unpackPremultiplyAlpha, m_unpackFlipY, IntPoint(xoffset, yoffset),
-            sourceSubRectangle)) {
+            functionIDToSnapshotReason(functionID), contextGL(), target,
+            targetTexture, m_unpackPremultiplyAlpha, m_unpackFlipY,
+            IntPoint(xoffset, yoffset), sourceSubRectangle)) {
       NOTREACHED();
     }
   } else {
@@ -5008,9 +4960,9 @@ void WebGLRenderingContextBase::texImageCanvasByGPU(
         toWebGLRenderingContextBase(canvas->renderingContext());
     ScopedTexture2DRestorer restorer(gl);
     if (!gl->drawingBuffer()->copyToPlatformTexture(
-            contextGL(), targetTexture, targetInternalformat, targetType,
-            targetLevel, m_unpackPremultiplyAlpha, !m_unpackFlipY,
-            IntPoint(xoffset, yoffset), sourceSubRectangle, BackBuffer)) {
+            contextGL(), target, targetTexture, m_unpackPremultiplyAlpha,
+            !m_unpackFlipY, IntPoint(xoffset, yoffset), sourceSubRectangle,
+            BackBuffer)) {
       NOTREACHED();
     }
   }
@@ -5021,8 +4973,6 @@ void WebGLRenderingContextBase::texImageByGPU(
     WebGLTexture* texture,
     GLenum target,
     GLint level,
-    GLint internalformat,
-    GLenum type,
     GLint xoffset,
     GLint yoffset,
     GLint zoffset,
@@ -5035,24 +4985,18 @@ void WebGLRenderingContextBase::texImageByGPU(
   ScopedTexture2DRestorer restorer(this);
 
   GLuint targetTexture = texture->object();
-  GLenum targetType = type;
-  GLenum targetInternalformat = internalformat;
-  GLint targetLevel = level;
   bool possibleDirectCopy = false;
-  if (functionID == TexImage2D) {
-    possibleDirectCopy = Extensions3DUtil::canUseCopyTextureCHROMIUM(
-        target, internalformat, type, level);
+  if (functionID == TexImage2D || functionID == TexSubImage2D) {
+    possibleDirectCopy = Extensions3DUtil::canUseCopyTextureCHROMIUM(target);
   }
 
   GLint copyXOffset = xoffset;
   GLint copyYOffset = yoffset;
+  GLenum copyTarget = target;
 
   // if direct copy is not possible, create a temporary texture and then copy
   // from canvas to temporary texture to target texture.
   if (!possibleDirectCopy) {
-    targetLevel = 0;
-    targetInternalformat = GL_RGBA;
-    targetType = GL_UNSIGNED_BYTE;
     contextGL()->GenTextures(1, &targetTexture);
     contextGL()->BindTexture(GL_TEXTURE_2D, targetTexture);
     contextGL()->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER,
@@ -5063,21 +5007,21 @@ void WebGLRenderingContextBase::texImageByGPU(
                                GL_CLAMP_TO_EDGE);
     contextGL()->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T,
                                GL_CLAMP_TO_EDGE);
-    contextGL()->TexImage2D(GL_TEXTURE_2D, 0, targetInternalformat, width,
-                            height, 0, GL_RGBA, targetType, 0);
+    contextGL()->TexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, width, height, 0,
+                            GL_RGBA, GL_UNSIGNED_BYTE, 0);
     copyXOffset = 0;
     copyYOffset = 0;
+    copyTarget = GL_TEXTURE_2D;
   }
 
   if (image->isCanvasElement()) {
     texImageCanvasByGPU(functionID, static_cast<HTMLCanvasElement*>(image),
-                        targetTexture, targetInternalformat, targetType,
-                        targetLevel, copyXOffset, copyYOffset,
+                        copyTarget, targetTexture, copyXOffset, copyYOffset,
                         sourceSubRectangle);
   } else {
-    texImageBitmapByGPU(static_cast<ImageBitmap*>(image), targetTexture,
-                        targetInternalformat, targetType, targetLevel,
-                        !m_unpackFlipY);
+    texImageBitmapByGPU(static_cast<ImageBitmap*>(image), copyTarget,
+                        targetTexture, !m_unpackFlipY, copyXOffset, copyYOffset,
+                        sourceSubRectangle);
   }
 
   if (!possibleDirectCopy) {
@@ -5177,11 +5121,11 @@ void WebGLRenderingContextBase::texImageHelperHTMLCanvasElement(
     if (functionID == TexImage2D) {
       texImage2DBase(target, level, internalformat, sourceSubRectangle.width(),
                      sourceSubRectangle.height(), 0, format, type, 0);
-      texImageByGPU(functionID, texture, target, level, internalformat, type, 0,
-                    0, 0, canvas, adjustedSourceSubRectangle);
+      texImageByGPU(functionID, texture, target, level, 0, 0, 0, canvas,
+                    adjustedSourceSubRectangle);
     } else {
-      texImageByGPU(functionID, texture, target, level, GL_RGBA, type, xoffset,
-                    yoffset, 0, canvas, adjustedSourceSubRectangle);
+      texImageByGPU(functionID, texture, target, level, xoffset, yoffset, 0,
+                    canvas, adjustedSourceSubRectangle);
     }
   } else {
     // 3D functions.
@@ -5263,9 +5207,14 @@ void WebGLRenderingContextBase::texImageHelperHTMLVideoElement(
       sourceImageRect == sentinelEmptyRect() ||
       sourceImageRect ==
           IntRect(0, 0, video->videoWidth(), video->videoHeight());
+  // Format of source video may be 16-bit format, e.g. Y16 format.
+  // glCopyTextureCHROMIUM will cause precision lost when uploading such video
+  // texture to half float or float texture.

[Zhenyao Mo, 2017/03/10 00:10:24]

Well, the same can be said for RGBA8 formats, so I am not sure if we want to put
in this limit.

[qiankun, 2017/03/10 02:48:04]

Or we can increase the tolerance in the tests? I met this issue for
depth_capture_tests where tolerance == 1.5/65535 :
https://cs.chromium.org/chromium/src/content/test/data/media/getusermedia-dep....

[Zhenyao Mo, 2017/03/10 22:45:43]

What will be the tolerance to make it pass? I think slightly increase the
tolerance is fine if the benefit is a much faster path.

[qiankun, 2017/03/10 23:54:19]

When calling glCopy{Sub}TextureCHROMIUM, source_internal_format of the source
texture in this case is BGRA8 or RGBA8 on different OSes. So, we should set the
tolerance to 1.0 / 255 to make the test pass.

[aleksandar.stojiljkovic, 2017/03/14 13:43:46]

Thanks.
Yes, that would break the 16 bit video. Another work that could be affected is
HDR video - hubbe@ has started work on HDR video decoding
(https://crbug.com/682416) and I'm planning to work on support for GL_RGB10_A2.

Could we try this as a fix (so that we also enable copy texture for
isFloatType(type) case):
[L.5213-L.5217] is guarding two things:
1. copyVideoTextureToPlatformTexture (no precision loss, I think)
2. accelerated image buffer (painting to RGBA is introducing cutting precision
to 8 bits).
So, would splitting 1. and 2. and inserting 16-bit video code on L.5269 between
them fix this?
I remember doing that [1] but changed my mind later to avoid introducing
unnecessary change:
[1]
L.5095
https://codereview.chromium.org/2476693002/diff/100001/third_party/WebKit/Sou...





 
 

[qiankun, 2017/03/31 09:54:05]

Done as aleksandar suggested.

   if (functionID == TexImage2D && sourceImageRectIsDefault && depth == 1 &&
-      GL_TEXTURE_2D == target && Extensions3DUtil::canUseCopyTextureCHROMIUM(
-                                     target, internalformat, type, level)) {
+      GL_TEXTURE_2D == target &&
+      canUseTexImageByGPU(functionID, internalformat, type) &&
+      (type != GL_FLOAT && type != GL_HALF_FLOAT &&
+       type != GL_UNSIGNED_INT_10F_11F_11F_REV)) {
     DCHECK_EQ(xoffset, 0);
     DCHECK_EQ(yoffset, 0);
     DCHECK_EQ(zoffset, 0);
@@ -5307,9 +5256,9 @@ void WebGLRenderingContextBase::texImageHelperHTMLVideoElement(
         // was handled in the paintCurrentFrameInContext() call.
 
         if (imageBuffer->copyToPlatformTexture(
-                functionIDToSnapshotReason(functionID), contextGL(),
-                texture->object(), internalformat, type, level,
-                m_unpackPremultiplyAlpha, m_unpackFlipY, IntPoint(0, 0),
+                functionIDToSnapshotReason(functionID), contextGL(), target,
+                texture->object(), m_unpackPremultiplyAlpha, m_unpackFlipY,
+                IntPoint(0, 0),
                 IntRect(0, 0, video->videoWidth(), video->videoHeight()))) {
           return;
         }
@@ -5341,15 +5290,17 @@ void WebGLRenderingContextBase::texImageHelperHTMLVideoElement(
                unpackImageHeight);
 }
 
-void WebGLRenderingContextBase::texImageBitmapByGPU(ImageBitmap* bitmap,
-                                                    GLuint targetTexture,
-                                                    GLenum targetInternalformat,
-                                                    GLenum targetType,
-                                                    GLint targetLevel,
-                                                    bool flipY) {
-  bitmap->bitmapImage()->copyToTexture(drawingBuffer()->contextProvider(),
-                                       targetTexture, targetInternalformat,
-                                       targetType, flipY);
+void WebGLRenderingContextBase::texImageBitmapByGPU(
+    ImageBitmap* bitmap,
+    GLenum target,
+    GLuint targetTexture,
+    bool flipY,
+    GLint xoffset,
+    GLint yoffset,
+    const IntRect& sourceSubRectangle) {
+  bitmap->bitmapImage()->copyToTexture(
+      drawingBuffer()->contextProvider(), target, targetTexture, flipY,
+      IntPoint(xoffset, yoffset), sourceSubRectangle);
 }
 
 void WebGLRenderingContextBase::texImage2D(GLenum target,
@@ -5417,11 +5368,11 @@ void WebGLRenderingContextBase::texImageHelperImageBitmap(
     if (functionID == TexImage2D) {
       texImage2DBase(target, level, internalformat, width, height, 0, format,
                      type, 0);
-      texImageByGPU(functionID, texture, target, level, internalformat, type, 0,
-                    0, 0, bitmap, sourceSubRect);
+      texImageByGPU(functionID, texture, target, level, 0, 0, 0, bitmap,
+                    sourceSubRect);
     } else if (functionID == TexSubImage2D) {
-      texImageByGPU(functionID, texture, target, level, GL_RGBA, type, xoffset,
-                    yoffset, 0, bitmap, sourceSubRect);
+      texImageByGPU(functionID, texture, target, level, xoffset, yoffset, 0,
+                    bitmap, sourceSubRect);
     }
     return;
   }