Avoid copy pixel data in texImage2D(ImageBitmap)

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "modules/webgl/WebGL2RenderingContextBase.h"
 
 #include "bindings/modules/v8/WebGLAny.h"
 #include "core/frame/ImageBitmap.h"
 #include "core/html/HTMLCanvasElement.h"
 #include "core/html/HTMLImageElement.h"
@@ skipping 1054 matching lines @@
         // The UNSIGNED_INT_10F_11F_11F_REV type pack/unpack isn't implemented.
         type = GL_FLOAT;
     }
     Vector<uint8_t> data;
     bool needConversion = true;
     // The data from ImageData is always of format RGBA8.
     // No conversion is needed if destination format is RGBA and type is USIGNED_BYTE and no Flip or Premultiply operation is required.
     if (format == GL_RGBA && type == GL_UNSIGNED_BYTE && !m_unpackFlipY && !m_unpackPremultiplyAlpha) {
         needConversion = false;
     } else {
-        if (!WebGLImageConversion::extractImageData(pixels->data()->data(), pixels->size(), format, type, m_unpackFlipY, m_unpackPremultiplyAlpha, data)) {
+        if (!WebGLImageConversion::extractImageData(pixels->data()->data(), WebGLImageConversion::DataFormat::DataFormatRGBA8, pixels->size(), format, type, m_unpackFlipY, m_unpackPremultiplyAlpha, data)) {
             synthesizeGLError(GL_INVALID_VALUE, "texSubImage3D", "bad image data");
             return;
         }
     }
     resetUnpackParameters();
     contextGL()->TexSubImage3D(target, level, xoffset, yoffset, zoffset, pixels->width(), pixels->height(), 1, format, type, needConversion ? data.data() : pixels->data()->data());
     restoreUnpackParameters();
 }
 
 void WebGL2RenderingContextBase::texSubImage3D(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLenum format, GLenum type, HTMLImageElement* image, ExceptionState& exceptionState)
@@ skipping 56 matching lines @@
         return;
     if (!validateTexture3DBinding("texSubImage3D", target))
         return;
     if (!validateTexFunc("texSubImage3D", TexSubImage, SourceImageBitmap, target, level, 0, bitmap->width(), bitmap->height(), 1, 0, format, type, xoffset, yoffset, zoffset))
         return;
     if (type == GL_UNSIGNED_INT_10F_11F_11F_REV) {
         // The UNSIGNED_INT_10F_11F_11F_REV type pack/unpack isn't implemented.
         type = GL_FLOAT;
     }
     ASSERT(bitmap->bitmapImage());
-    OwnPtr<uint8_t[]> pixelData = bitmap->copyBitmapData(bitmap->isPremultiplied() ? PremultiplyAlpha : DontPremultiplyAlpha);
+    RefPtr<SkImage> skImage = bitmap->bitmapImage()->imageForCurrentFrame();
+    SkPixmap pixmap;
+    uint8_t* pixelData = nullptr;

[Justin Novosad, 2016/05/27 14:16:19]

Should use an OwnPtr for this.

[xidachen, 2016/05/27 15:10:13]

I don't think we can do that. SkImage is in charge of the lifetime of the pixel
data. If we make this OwnPtr, then we are trying to take the ownership which
causes problem. I got a "corrupted memory" when I tried to use OwnPtr here.

[Justin Novosad, 2016/05/27 16:13:48]

That just means you were not doing it right.
Not using OwnPtr here goes against the code style. You should only use raw ptrs
for unowned references (and GC stuff).  The rule helps prevent us from writing
code with memory leaks. This code actually proves that point: it has a memory
leak.  In the case where skImage is texture backed, pixelData is never freed. 
The tricky part is that you have a mix of owned vs unowned cases, using the same
variable.  Here is how you fix that:

OwnPtr<uint8_t> pixelData;
uint8_t* pixelDataPtr = nullptr;
if (peekSucceed)
    pixelDataPtr = static_cast<uint8_t*>(pixmap.writable_addr());
else if (skImage->isTextureBacked()) {
    pixelData = bitmap->copyBitmapData(bitmap->isPremultiplied() ?
PremultiplyAlpha : DontPremultiplyAlpha);
    pixelDataPtr = pixelData.get();
}

Then, in the code below, you replace uses of pixelData with pixelDataPtr.

[xidachen, 2016/05/27 17:14:56]

Yes, that makes perfect sense. Thanks for pointing it out.

+    bool peekSucceed = skImage->peekPixels(&pixmap);
+    if (peekSucceed)
+        pixelData = static_cast<uint8_t*>(pixmap.writable_addr());
+    else if (skImage->isTextureBacked())
+        pixelData = bitmap->copyBitmapData(bitmap->isPremultiplied() ? PremultiplyAlpha : DontPremultiplyAlpha).leakPtr();
     Vector<uint8_t> data;
     bool needConversion = true;
-    if (format == GL_RGBA && type == GL_UNSIGNED_BYTE) {
+    if (((peekSucceed && pixmap.colorType() == SkColorType::kRGBA_8888_SkColorType) || !peekSucceed) && format == GL_RGBA && type == GL_UNSIGNED_BYTE) {

[Zhenyao Mo, 2016/05/27 13:49:21]

if peekSucceed is false, why needConversion is set to false? Isn't the above
code if skImage->isTextureBacked() used pixelData?

[xidachen, 2016/05/27 13:54:44]

Sorry for this long if statement. Allow me to explain a little bit.
The first half of the if statement says: check either one of these two items,
1. peekSucceed && pixmap.colorType() == SkColorType::kRGBA_8888_SkColorType
2. !peekSucceed

No.1 means peekPixels succeed and the pixelData is stored in RGBA format
No.2 means peekPixels failed, in which case we use the pixelData returned from
copyBitmapData() function and that pixelData is always in RGBA format

If anyone of these two is true, and format == GL_RGBA && type ==
GL_UNSIGNED_BYTE, then we don't need to convert.

Does that make sense?

[Zhenyao Mo, 2016/05/27 14:08:30]

You are right.  I got confused. Should not read code at this early hour.

[Justin Novosad, 2016/05/27 14:16:19]

Please break-up long expressions make the code more readable. Split it into
intermediate steps.
Ex: bool havePeekableRGBA8 = ...  bool needReadback = ... etc.

[xidachen, 2016/05/27 15:10:13]

Done.

         needConversion = false;
     } else {
         // In the case of ImageBitmap, we do not need to apply flipY or premultiplyAlpha.
-        if (!WebGLImageConversion::extractImageData(pixelData.get(), bitmap->size(), format, type, false, false, data)) {
+        if ((peekSucceed && pixmap.colorType() == SkColorType::kBGRA_8888_SkColorType && !WebGLImageConversion::extractImageData(pixelData, WebGLImageConversion::DataFormat::DataFormatBGRA8, bitmap->size(), format, type, false, false, data))
+            || (((peekSucceed && pixmap.colorType() == SkColorType::kRGBA_8888_SkColorType) || !peekSucceed) && !WebGLImageConversion::extractImageData(pixelData, WebGLImageConversion::DataFormat::DataFormatRGBA8, bitmap->size(), format, type, false, false, data))) {
             synthesizeGLError(GL_INVALID_VALUE, "texSubImage3D", "bad image data");
             return;
         }
     }
     resetUnpackParameters();
-    contextGL()->TexSubImage3D(target, level, xoffset, yoffset, zoffset, bitmap->width(), bitmap->height(), 1, format, type, needConversion ? data.data() : pixelData.get());
+    contextGL()->TexSubImage3D(target, level, xoffset, yoffset, zoffset, bitmap->width(), bitmap->height(), 1, format, type, needConversion ? data.data() : pixelData);
     restoreUnpackParameters();
 }
 
 void WebGL2RenderingContextBase::copyTexSubImage3D(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLint x, GLint y, GLsizei width, GLsizei height)
 {
     if (isContextLost())
         return;
     if (!validateTexture3DBinding("copyTexSubImage3D", target))
         return;
     WebGLFramebuffer* readFramebufferBinding = nullptr;
@@ skipping 2413 matching lines @@
     params.skipPixels = m_unpackSkipPixels;
     params.skipRows = m_unpackSkipRows;
     if (dimension == Tex3D) {
         params.imageHeight = m_unpackImageHeight;
         params.skipImages = m_unpackSkipImages;
     }
     return params;
 }
 
 } // namespace blink