Avoid GPU readback in tex(Sub)Image2D(ImageBitmap)

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 fb622a1592c3a19a6608e8f9d254a72977d142b9..a31f147d3a0701fc93a0e388c5af93c73f10690a 100644
--- a/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
+++ b/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
@@ -4254,16 +4254,44 @@ void WebGLRenderingContextBase::texImage2D(GLenum target, GLint level, GLint int
     texImageHelperHTMLImageElement(TexImage2D, target, level, internalformat, format, type, 0, 0, 0, image, exceptionState);
 }
 
-bool WebGLRenderingContextBase::canUseTexImageCanvasByGPU(GLint internalformat, GLenum type)
+bool WebGLRenderingContextBase::canUseTexImageByGPU(GLint internalformat, GLenum type)
 {
     if (isFloatType(type) || isIntegerFormat(internalformat) || isSRGBFormat(internalformat))
         return false;
     return true;
 }
 
-void WebGLRenderingContextBase::texImageCanvasByGPU(TexImageByGPUType functionType, WebGLTexture* texture, GLenum target,
-    GLint level, GLint internalformat, GLenum type, GLint xoffset, GLint yoffset, GLint zoffset, HTMLCanvasElement* canvas)
+void WebGLRenderingContextBase::texImageCanvasByGPU(HTMLCanvasElement* canvas, GLuint targetTexture, GLenum targetInternalformat, GLenum targetType, GLint targetLevel)
 {
+    if (!canvas->is3D()) {
+        ImageBuffer* buffer = canvas->buffer();
+        if (!buffer->copyToPlatformTexture(contextGL(), targetTexture, targetInternalformat, targetType,
+            targetLevel, m_unpackPremultiplyAlpha, m_unpackFlipY)) {
+            NOTREACHED();
+        }
+    } else {
+        WebGLRenderingContextBase* gl = toWebGLRenderingContextBase(canvas->renderingContext());
+        ScopedTexture2DRestorer restorer(gl);
+        if (!gl->drawingBuffer()->copyToPlatformTexture(contextGL(), targetTexture, targetInternalformat, targetType,
+            targetLevel, m_unpackPremultiplyAlpha, !m_unpackFlipY, BackBuffer)) {
+            NOTREACHED();
+        }
+    }
+}
+
+void WebGLRenderingContextBase::texImageByGPU(TexImageByGPUType functionType, WebGLTexture* texture, GLenum target,
+    GLint level, GLint internalformat, GLenum type, GLint xoffset, GLint yoffset, GLint zoffset, HTMLCanvasElement* canvas, ImageBitmap* bitmap)

[Justin Novosad, 2016/06/14 19:30:53]

Instead of having these two arguments (canvas + bitmap), you should just use use
a single argument of a common base class, like CanvasImageSource. Then, move any
source-type-specific code into virtual methods that are implemented in both
HTMLCanvasElement and ImageBitmap.  This is a more extensible design that will
make it easy to later go through this path for things like laoding accelerated
video decode and SVG rendered on the GPU and who knows what else.

[xidachen, 2016/06/15 12:18:47]

I do agree that it is a better design to use the base class such as
CanvasImageSource. The only problem is that the width() and height() methods for
derived class of CanvasImageSource has different return types. For example,
ImageBitmap is const, while HTMLImageElement is not const. I don't think we can
make a virtual width() and height() non-const and its derived class const.

+{
+    DCHECK((!canvas || !bitmap) && (canvas || bitmap));
+    int width;
+    int height;
+    if (canvas) {
+        width = canvas->width();
+        height = canvas->height();
+    } else {
+        width = bitmap->width();
+        height = bitmap->height();
+    }
     ScopedTexture2DRestorer restorer(this);
 
     GLuint targetTexture = texture->object();
@@ -4271,6 +4299,7 @@ void WebGLRenderingContextBase::texImageCanvasByGPU(TexImageByGPUType functionTy
     GLenum targetInternalformat = internalformat;
     GLint targetLevel = level;
     bool possibleDirectCopy = false;
+    // TODO: Make this function support more cases in texSubImage calls.
     if (functionType == TexImage2DByGPU) {
         possibleDirectCopy = Extensions3DUtil::canUseCopyTextureCHROMIUM(target, internalformat, type, level);
     }
@@ -4286,24 +4315,13 @@ void WebGLRenderingContextBase::texImageCanvasByGPU(TexImageByGPUType functionTy
         contextGL()->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
         contextGL()->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
         contextGL()->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
-        contextGL()->TexImage2D(GL_TEXTURE_2D, 0, targetInternalformat, canvas->width(),
-            canvas->height(), 0, GL_RGBA, targetType, 0);
+        contextGL()->TexImage2D(GL_TEXTURE_2D, 0, targetInternalformat, width, height, 0, GL_RGBA, targetType, 0);
     }
 
-    if (!canvas->is3D()) {
-        ImageBuffer* buffer = canvas->buffer();
-        if (!buffer->copyToPlatformTexture(contextGL(), targetTexture, targetInternalformat, targetType,
-            targetLevel, m_unpackPremultiplyAlpha, m_unpackFlipY)) {
-            ASSERT_NOT_REACHED();
-        }
-    } else {
-        WebGLRenderingContextBase* gl = toWebGLRenderingContextBase(canvas->renderingContext());
-        ScopedTexture2DRestorer restorer(gl);
-        if (!gl->drawingBuffer()->copyToPlatformTexture(contextGL(), targetTexture, targetInternalformat, targetType,
-            targetLevel, m_unpackPremultiplyAlpha, !m_unpackFlipY, BackBuffer)) {
-            ASSERT_NOT_REACHED();
-        }
-    }
+    if (canvas)
+        texImageCanvasByGPU(canvas, targetTexture, targetInternalformat, targetType, targetLevel);
+    else
+        texImageBitmapByGPU(bitmap, targetTexture, targetInternalformat, targetType, targetLevel);
 
     if (!possibleDirectCopy) {
         GLuint tmpFBO;
@@ -4312,11 +4330,11 @@ void WebGLRenderingContextBase::texImageCanvasByGPU(TexImageByGPUType functionTy
         contextGL()->FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, targetTexture, 0);
         contextGL()->BindTexture(texture->getTarget(), texture->object());
         if (functionType == TexImage2DByGPU) {
-            contextGL()->CopyTexSubImage2D(target, level, 0, 0, 0, 0, canvas->width(), canvas->height());
+            contextGL()->CopyTexSubImage2D(target, level, 0, 0, 0, 0, width, height);
         } else if (functionType == TexSubImage2DByGPU) {
-            contextGL()->CopyTexSubImage2D(target, level, xoffset, yoffset, 0, 0, canvas->width(), canvas->height());
+            contextGL()->CopyTexSubImage2D(target, level, xoffset, yoffset, 0, 0, width, height);
         } else if (functionType == TexSubImage3DByGPU) {
-            contextGL()->CopyTexSubImage3D(target, level, xoffset, yoffset, zoffset, 0, 0, canvas->width(), canvas->height());
+            contextGL()->CopyTexSubImage3D(target, level, xoffset, yoffset, zoffset, 0, 0, width, height);
         }
         contextGL()->FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, 0, 0);
         restoreCurrentFramebuffer();
@@ -4345,9 +4363,9 @@ void WebGLRenderingContextBase::texImageHelperHTMLCanvasElement(TexImageFunction
     if (!validateTexFunc(funcName, functionType, SourceHTMLCanvasElement, target, level, internalformat, canvas->width(), canvas->height(), 1, 0, format, type, xoffset, yoffset, zoffset))
         return;
     if (functionID == TexImage2D) {
-        // texImageCanvasByGPU relies on copyTextureCHROMIUM which doesn't support float/integer/sRGB internal format.
+        // texImageByGPU relies on copyTextureCHROMIUM which doesn't support float/integer/sRGB internal format.
         // FIXME: relax the constrains if copyTextureCHROMIUM is upgraded to handle more formats.
-        if (!canvas->renderingContext() || !canvas->renderingContext()->isAccelerated() || !canUseTexImageCanvasByGPU(internalformat, type)) {
+        if (!canvas->renderingContext() || !canvas->renderingContext()->isAccelerated() || !canUseTexImageByGPU(internalformat, type)) {
             // 2D canvas has only FrontBuffer.
             texImageImpl(TexImage2D, target, level, internalformat, xoffset, yoffset, zoffset, format, type, canvas->copiedImage(FrontBuffer, PreferAcceleration).get(),
                 WebGLImageConversion::HtmlDomCanvas, m_unpackFlipY, m_unpackPremultiplyAlpha);
@@ -4355,7 +4373,7 @@ void WebGLRenderingContextBase::texImageHelperHTMLCanvasElement(TexImageFunction
         }
 
         texImage2DBase(target, level, internalformat, canvas->width(), canvas->height(), 0, format, type, 0);
-        texImageCanvasByGPU(TexImage2DByGPU, texture, target, level, internalformat, type, 0, 0, 0, canvas);
+        texImageByGPU(TexImage2DByGPU, texture, target, level, internalformat, type, 0, 0, 0, canvas, nullptr);
     } else if (functionID == TexSubImage2D) {
         // FIXME: Implement GPU-to-GPU path for WebGL 2 and more internal formats.
         bool useReadBackPath = isWebGL2OrHigher()
@@ -4371,7 +4389,7 @@ void WebGLRenderingContextBase::texImageHelperHTMLCanvasElement(TexImageFunction
             return;
         }
 
-        texImageCanvasByGPU(TexSubImage2DByGPU, texture, target, level, GL_RGBA, type, xoffset, yoffset, 0, canvas);
+        texImageByGPU(TexSubImage2DByGPU, texture, target, level, GL_RGBA, type, xoffset, yoffset, 0, canvas, nullptr);
     } else {
         DCHECK_EQ(functionID, TexSubImage3D);
         // FIXME: Implement GPU-to-GPU copy path (crbug.com/586269).
@@ -4454,6 +4472,12 @@ void WebGLRenderingContextBase::texImageHelperHTMLVideoElement(TexImageFunctionI
     texImageImpl(functionID, target, level, internalformat, xoffset, yoffset, zoffset, format, type, image.get(), WebGLImageConversion::HtmlDomVideo, m_unpackFlipY, m_unpackPremultiplyAlpha);
 }
 
+void WebGLRenderingContextBase::texImageBitmapByGPU(ImageBitmap* bitmap, GLuint targetTexture, GLenum targetInternalformat, GLenum targetType, GLint targetLevel)
+{
+    RefPtr<SkImage> skImage = bitmap->bitmapImage()->imageForCurrentFrame(drawingBuffer()->contextProvider());
+    bitmap->bitmapImage()->copyToTexture(drawingBuffer()->contextProvider(), targetTexture, targetInternalformat, targetType);

[Justin Novosad, 2016/06/14 19:30:53]

I think this is where your bug is. When copying a GPU-backed ImageBitmap to a
WebGL texture, you are ignoring the WebGL context's m_unpackFlipY state. See
texImageCanvasByGPU which does the right thing.

+}
+
 void WebGLRenderingContextBase::texImage2D(GLenum target, GLint level, GLint internalformat,
     GLenum format, GLenum type, HTMLVideoElement* video, ExceptionState& exceptionState)
 {
@@ -4469,7 +4493,8 @@ void WebGLRenderingContextBase::texImageHelperImageBitmap(TexImageFunctionID fun
         return;
     if (!validateImageBitmap(funcName, bitmap, exceptionState))
         return;
-    if (!validateTexImageBinding(funcName, functionID, target))
+    WebGLTexture* texture = validateTexImageBinding(funcName, functionID, target);
+    if (!texture)
         return;
     TexImageFunctionType functionType;
     if (functionID == TexImage2D)
@@ -4479,24 +4504,29 @@ void WebGLRenderingContextBase::texImageHelperImageBitmap(TexImageFunctionID fun
     if (!validateTexFunc(funcName, functionType, SourceImageBitmap, target, level, internalformat, bitmap->width(), bitmap->height(), 1, 0, format, type, xoffset, yoffset, zoffset))
         return;
     ASSERT(bitmap->bitmapImage());
+    if (bitmap->isTextureBacked() && canUseTexImageByGPU(internalformat, type)) {
+        texImage2DBase(target, level, internalformat, bitmap->width(), bitmap->height(), 0, format, type, 0);
+        texImageByGPU(TexImage2DByGPU, texture, target, level, internalformat, type, 0, 0, 0, nullptr, bitmap);
+        return;
+    }
     RefPtr<SkImage> skImage = bitmap->bitmapImage()->imageForCurrentFrame();
     SkPixmap pixmap;
     OwnPtr<uint8_t[]> pixelData;
     uint8_t* pixelDataPtr = nullptr;
-    // TODO(crbug.com/613411): peekPixels fails if the SkImage is texture-backed
-    // Use texture mailbox in that case.
+    // In the case where an ImageBitmap is not texture backed, peekPixels() always succeed.
+    // However, when it is texture backed and !canUseTexImageByGPU, we do a GPU read back.
     bool peekSucceed = skImage->peekPixels(&pixmap);
     if (peekSucceed) {
         pixelDataPtr = static_cast<uint8_t*>(pixmap.writable_addr());
-    } else if (skImage->isTextureBacked()) {
+    } else {
         pixelData = bitmap->copyBitmapData(bitmap->isPremultiplied() ? PremultiplyAlpha : DontPremultiplyAlpha);
         pixelDataPtr = pixelData.get();
     }
     Vector<uint8_t> data;
     bool needConversion = true;
     bool havePeekableRGBA = (peekSucceed && pixmap.colorType() == SkColorType::kRGBA_8888_SkColorType);
-    bool isPixelDataRBGA = (havePeekableRGBA || !peekSucceed);
-    if (isPixelDataRBGA && format == GL_RGBA && type == GL_UNSIGNED_BYTE) {
+    bool isPixelDataRGBA = (havePeekableRGBA || !peekSucceed);
+    if (isPixelDataRGBA && format == GL_RGBA && type == GL_UNSIGNED_BYTE) {
         needConversion = false;
     } else {
         if (type == GL_UNSIGNED_INT_10F_11F_11F_REV) {
@@ -4504,9 +4534,9 @@ void WebGLRenderingContextBase::texImageHelperImageBitmap(TexImageFunctionID fun
             type = GL_FLOAT;
         }
         // In the case of ImageBitmap, we do not need to apply flipY or premultiplyAlpha.
-        bool isPixelDataBGRA = (peekSucceed && pixmap.colorType() == SkColorType::kBGRA_8888_SkColorType);
+        bool isPixelDataBGRA = pixmap.colorType() == SkColorType::kBGRA_8888_SkColorType;
         if ((isPixelDataBGRA && !WebGLImageConversion::extractImageData(pixelDataPtr, WebGLImageConversion::DataFormat::DataFormatBGRA8, bitmap->size(), format, type, false, false, data))
-            || (isPixelDataRBGA && !WebGLImageConversion::extractImageData(pixelDataPtr, WebGLImageConversion::DataFormat::DataFormatRGBA8, bitmap->size(), format, type, false, false, data))) {
+            || (isPixelDataRGBA && !WebGLImageConversion::extractImageData(pixelDataPtr, WebGLImageConversion::DataFormat::DataFormatRGBA8, bitmap->size(), format, type, false, false, data))) {
             synthesizeGLError(GL_INVALID_VALUE, funcName, "bad image data");
             return;
         }