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

third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp

Index: third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp b/third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp
index ac88b3d3d8c066136131b2f7aa9719f45976c96e..c5fb57326af450b9006f685fe62e3ba9f3823524 100644
--- a/third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp
+++ b/third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp
@@ -14,6 +14,7 @@
 #include "third_party/skia/include/core/SkImage.h"
 #include "third_party/skia/include/core/SkPaint.h"
 #include "third_party/skia/include/core/SkShader.h"
+#include "third_party/skia/include/gpu/GrContext.h"
 
 namespace blink {
 
@@ -42,9 +43,16 @@ StaticBitmapImage::~StaticBitmapImage() { }
 
 IntSize StaticBitmapImage::size() const
 {
+    if (!m_image)
+        return IntSize(m_mailbox.textureSize.width, m_mailbox.textureSize.height);
     return IntSize(m_image->width(), m_image->height());
 }
 
+bool StaticBitmapImage::isTextureBacked()
+{
+    return m_image && m_image->isTextureBacked();
+}
+
 bool StaticBitmapImage::currentFrameKnownToBeOpaque(MetadataMode)
 {
     return m_image->isOpaque();
@@ -66,28 +74,21 @@ void StaticBitmapImage::draw(SkCanvas* canvas, const SkPaint& paint, const Float
         observer->didDraw(this);
 }
 
-PassRefPtr<SkImage> StaticBitmapImage::imageForCurrentFrame()
+void StaticBitmapImage::consumeTextureMailbox(WebGraphicsContext3DProvider* provider)
 {
-    if (m_image)
-        return m_image;
-    DCHECK(isMainThread());
-    // In the place when we consume an ImageBitmap that is gpu texture backed,
-    // create a new SkImage from that texture.
-    // TODO(xidachen): make this work on a worker thread.
-    OwnPtr<WebGraphicsContext3DProvider> provider = adoptPtr(Platform::current()->createSharedOffscreenGraphicsContext3DProvider());
     if (!provider)
-        return nullptr;
+        return;
     GrContext* grContext = provider->grContext();
     if (!grContext)
-        return nullptr;
+        return;
     gpu::gles2::GLES2Interface* gl = provider->contextGL();
     if (!gl)
-        return nullptr;
+        return;
     gl->WaitSyncTokenCHROMIUM(m_mailbox.syncToken);
-    GLuint textureId = gl->CreateAndConsumeTextureCHROMIUM(GL_TEXTURE_2D, m_mailbox.name);
+    m_textureId = gl->CreateAndConsumeTextureCHROMIUM(GL_TEXTURE_2D, m_mailbox.name);
     GrGLTextureInfo textureInfo;
     textureInfo.fTarget = GL_TEXTURE_2D;
-    textureInfo.fID = textureId;
+    textureInfo.fID = m_textureId;
     GrBackendTextureDesc backendTexture;
     backendTexture.fOrigin = kBottomLeft_GrSurfaceOrigin;
     backendTexture.fWidth = m_mailbox.textureSize.width;
@@ -96,6 +97,79 @@ PassRefPtr<SkImage> StaticBitmapImage::imageForCurrentFrame()
     backendTexture.fTextureHandle = skia::GrGLTextureInfoToGrBackendObject(textureInfo);
     sk_sp<SkImage> skImage = SkImage::MakeFromAdoptedTexture(grContext, backendTexture);
     m_image = fromSkSp(skImage);
+}
+
+void StaticBitmapImage::copyToTexture(WebGraphicsContext3DProvider* provider, GLuint destinationTexture, GLenum internalFormat, GLenum destType)
+{
+    gpu::gles2::GLES2Interface* gl = provider->contextGL();
+    if (!gl)
+        return;
+    gl->CopyTextureCHROMIUM(m_textureId, destinationTexture, internalFormat, destType, false, false, false);
+    const GLuint64 fenceSync = gl->InsertFenceSyncCHROMIUM();
+    GLbyte syncToken[24];
+    gl->GenSyncTokenCHROMIUM(fenceSync, syncToken);
+}
+
+bool StaticBitmapImage::prepareMailboxForSkImage(WebGraphicsContext3DProvider* provider)

[Justin Novosad, 2016/06/10 17:50:40]

Could we call this switchStorageToMailbox, and the "consume" method could be
switchStorageToSkImage.  This naming would make it clear that only one form of
storage is current at a given time and that the code is meant to toggle between
the two.

[xidachen, 2016/06/14 18:09:40]

Done.

+{
+    m_mailbox.textureSize = WebSize(m_image->width(), m_image->height());
+    GrContext* grContext = provider->grContext();
+    if (!grContext)
+        return false;
+    grContext->flush();
+    m_image->getTexture()->textureParamsModified();

[Justin Novosad, 2016/06/09 20:21:29]

I am pretty sure this call is unnecessary

[xidachen, 2016/06/10 15:36:08]

Done.

+    m_mailbox.textureTarget = GL_TEXTURE_2D;
+    gpu::gles2::GLES2Interface* gl = provider->contextGL();
+    if (!gl)
+        return false;
+    GLuint textureID = skia::GrBackendObjectToGrGLTextureInfo(m_image->getTextureHandle(true))->fID;
+    gl->BindTexture(GL_TEXTURE_2D, textureID);
+
+    gl->GenMailboxCHROMIUM(m_mailbox.name);
+    RefPtr<SkData> mailboxNameData = adoptRef(SkData::NewWithCopy(&m_mailbox.name[0], sizeof(m_mailbox.name)));

[Justin Novosad, 2016/06/09 20:21:29]

No need for this. consumeTextureMailbox gets all it needs from m_mailbox.

[xidachen, 2016/06/10 15:36:08]

Done.

+    m_image->getTexture()->setCustomData(mailboxNameData.get());
+    gl->ProduceTextureCHROMIUM(GL_TEXTURE_2D, m_mailbox.name);
+    const GLuint64 fenceSync = gl->InsertFenceSyncCHROMIUM();
+    gl->Flush();
+    gl->GenSyncTokenCHROMIUM(fenceSync, m_mailbox.syncToken);
+    m_mailbox.validSyncToken = true;
+    gl->BindTexture(GL_TEXTURE_2D, 0);
+    grContext->resetContext(kTextureBinding_GrGLBackendState);

[Justin Novosad, 2016/06/09 20:21:29]

You should discard m_image here IMHO, to make sure it does not get confused for
a texture-backed image that is in the shared context.

[xidachen, 2016/06/10 15:36:08]

I am a little confused. In the code path of imageForCurrentFrame(), we return
m_image after calling this function in some cases. If we discard m_image, then
we are returning nullptr?

[Justin Novosad, 2016/06/10 17:50:40]

Right, in that case you should capture a local ref before calling this method.

+    return true;
+}
+
+PassRefPtr<SkImage> StaticBitmapImage::imageForCurrentFrame(WebGraphicsContext3DProvider* contextProvider)
+{

[xidachen, 2016/06/09 15:09:53]

The reason I put a lot of if statement here is that each if stands for one case.
I can certainly merge some of them to be one if statement if that is preferable.

[Justin Novosad, 2016/06/09 20:21:29]

+1 this it is very readable. 

+    if (m_image) {
+        // If the SkImage is not texture-backed
+        if (!m_image->isTextureBacked())
+            return m_image;
+        // SkImage is texture-backed, but no mailbox exists. virtual/gpu/ cases.
+        if (!hasMailbox())

[Justin Novosad, 2016/06/09 20:21:29]

This does not seem correct to me. If you have an image bitmap that was from an
accelerated 2D canvas, and pass it to texSubImage, you need to go through a
mailbox to bring it into the right context. I think you need something like
this:
if (!hasMailbox()) {
   // SkImage is texture backed on the shared context
   if (contextProvider) {
      OwnPtr<WebGraphicsContext3DProvider> sharedProvider =
adoptPtr(Platform::current()->createSharedOffscreenGraphicsContext3DProvider());
      prepareMailboxForSkImage(sharedProvider);
      consumeTextureMailbox(contextProvider);
      prepareMailboxForSkImage(contextProvider); // Because we can't retain a
texture in the WebGL context.
   }
   return m_image;
}

To test this, make sure you have a layout test that covers the following case:
Draw to a 2D canvas, createImageBitmap from the 2d canvas, upload the
ImageBitmap to a webgl texture.  under the virtual/gpu suite, that test should
exercise the case I am concerned about here.

[xidachen, 2016/06/10 15:36:08]

We actually have a layout test for that:
fast/canvas/webgl/texImage-imageBitmap-from-canvas.html

Before your suggested change, the way it works is to do a GPU-CPU read back and
then upload the pixelData as texture.

Now after your suggested change, I use fprintf and I can see that it goes
through the code path of GPU-GPU texture copy. Unfortunately that test is
broken. I tried to debug and what I see is this:
When the expected color is [255, 0, 0], the actual color that got back is [0,
255, 0]. When the expected color is [26, 0, 0], the actual color is [0, 26, 0].
Any idea why red & green channel gets shifted?

[Justin Novosad, 2016/06/10 17:50:40]

Would be worth investigating what happens to the other component to determine if
we are dealing with a format or endian-ness issue or a buffer offset issue.

+            return m_image;
+        // Has mailbox, but contextProvider is null (2D).
+        if (!contextProvider)
+            return m_image;
+        // 3D context, consume mailbox, prepare a new mailbox.
+        consumeTextureMailbox(contextProvider);
+        if (prepareMailboxForSkImage(contextProvider))
+            return nullptr;
+        return m_image;
+    }
+    // No mailbox, return null;
+    if (!hasMailbox())
+        return m_image;

[Justin Novosad, 2016/06/09 20:21:29]

might as well make this nullptr

[xidachen, 2016/06/10 15:36:08]

Done.

+    // Has mailbox, consume mailbox, prepare a new mailbox if contextProvider is not null (3D).
+    if (!contextProvider) {
+        DCHECK(isMainThread());
+        // TODO(xidachen): make this work on a worker thread.
+        OwnPtr<WebGraphicsContext3DProvider> provider = adoptPtr(Platform::current()->createSharedOffscreenGraphicsContext3DProvider());
+        consumeTextureMailbox(provider.get());
+    } else {
+        consumeTextureMailbox(contextProvider);
+        if (prepareMailboxForSkImage(contextProvider))
+            return nullptr;
+    }
     return m_image;
 }