Avoid copy pixel data in texImage2D(ImageBitmap)

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

Index: third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
diff --git a/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp b/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
index 67204a9facc19aa68490ebbe714478df25078954..fd8f73a7acaafae896960cf580391d9ff1b19fe2 100644
--- a/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
+++ b/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
@@ -1072,7 +1072,7 @@ void WebGL2RenderingContextBase::texSubImage3D(GLenum target, GLint level, GLint
     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;
         }
@@ -1149,20 +1149,28 @@ void WebGL2RenderingContextBase::texSubImage3D(GLenum target, GLint level, GLint
         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();
 }