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

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

 // Copyright 2014 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 "platform/graphics/StaticBitmapImage.h"
 
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "platform/graphics/GraphicsContext.h"
 #include "platform/graphics/ImageObserver.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebGraphicsContext3DProvider.h"
 #include "skia/ext/texture_handle.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #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 {
 
 PassRefPtr<StaticBitmapImage> StaticBitmapImage::create(PassRefPtr<SkImage> image)
 {
     if (!image)
         return nullptr;
     return adoptRef(new StaticBitmapImage(image));
 }
 
 PassRefPtr<StaticBitmapImage> StaticBitmapImage::create(WebExternalTextureMailbox& mailbox)
 {
     return adoptRef(new StaticBitmapImage(mailbox));
 }
 
 StaticBitmapImage::StaticBitmapImage(PassRefPtr<SkImage> image) : m_image(image)
 {
     ASSERT(m_image);

[Justin Novosad, 2016/06/07 15:11:07]

Should have: DCHECK(!m_image->isTextureBacked()); because having a texture
backed SkImage without having a context provider would break stuff.

[xidachen, 2016/06/07 17:49:53]

Not necessary. When I run the layout tests under virtual/gpu/, in those
texImage2D tests for ImageBitmap, ImageBitmaps are texture backed but no
contextProvider.

Also, I think in our previous discussion, we mentioned that when a context
provider consumes the mailbox inside the ImageBitmap, we should cache that
context provider in the ImageBitmap. I found that this won't always work. For
example, when it is a WebGL context that is consuming the ImageBitmap, the
context provider is owned by the drawing buffer, so ImageBitmap cannot just take
the ownership of that context provider.

 }
 
 StaticBitmapImage::StaticBitmapImage(WebExternalTextureMailbox& mailbox) : m_mailbox(mailbox)
 {
 }
 
 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::currentFrameKnownToBeOpaque(MetadataMode)
 {
     return m_image->isOpaque();
 }
 
 void StaticBitmapImage::draw(SkCanvas* canvas, const SkPaint& paint, const FloatRect& dstRect,
     const FloatRect& srcRect, RespectImageOrientationEnum, ImageClampingMode clampMode)
 {
     FloatRect adjustedSrcRect = srcRect;
     adjustedSrcRect.intersect(SkRect::Make(m_image->bounds()));
 
     if (dstRect.isEmpty() || adjustedSrcRect.isEmpty())
         return; // Nothing to draw.
 
     canvas->drawImageRect(m_image.get(), adjustedSrcRect, dstRect, &paint,
         WebCoreClampingModeToSkiaRectConstraint(clampMode));
 
     if (ImageObserver* observer = getImageObserver())
         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;
     backendTexture.fHeight = m_mailbox.textureSize.height;
     backendTexture.fConfig = kSkia8888_GrPixelConfig;
     backendTexture.fTextureHandle = skia::GrGLTextureInfoToGrBackendObject(textureInfo);
     sk_sp<SkImage> skImage = SkImage::MakeFromAdoptedTexture(grContext, backendTexture);
     m_image = fromSkSp(skImage);
+}
+
+void StaticBitmapImage::copyTexture(WebGraphicsContext3DProvider* provider, GLuint texture, GLenum internalFormat, GLenum destType)

[Justin Novosad, 2016/06/07 15:11:07]

Readability nit: From the names of the function and args, it is not clear
whether we this function is meant to copy to or from 'texture'.
copyTexture -> copyToTexture
texture -> destinationTexture

[xidachen, 2016/06/07 17:49:53]

Done.

+{
+    gpu::gles2::GLES2Interface* gl = provider->contextGL();
+    if (!gl)
+        return;
+    gl->CopyTextureCHROMIUM(m_textureId, texture, internalFormat, destType, false, false, false);
+    const GLuint64 fenceSync = gl->InsertFenceSyncCHROMIUM();
+    gl->Flush();

[Justin Novosad, 2016/06/07 15:11:07]

Why is this flush needed? the fence should be enough.

[xidachen, 2016/06/07 17:49:53]

Done.

+    GLbyte syncToken[24];
+    gl->GenSyncTokenCHROMIUM(fenceSync, syncToken);
+}
+
+bool StaticBitmapImage::prepareMailboxForSkImage(WebGraphicsContext3DProvider* provider)

[Justin Novosad, 2016/06/07 15:11:07]

Remove the unused 'provider' argument

[xidachen, 2016/06/07 17:49:54]

Done.

+{
+    m_mailbox.textureSize = WebSize(m_image->width(), m_image->height());
+    GrContext* grContext = m_provider->grContext();
+    if (!grContext)
+        return false;
+    grContext->flush();
+    m_image->getTexture()->textureParamsModified();
+    m_mailbox.textureTarget = GL_TEXTURE_2D;
+    gpu::gles2::GLES2Interface* gl = m_provider->contextGL();
+    if (!gl)
+        return false;
+    GLuint textureID = skia::GrBackendObjectToGrGLTextureInfo(m_image->getTextureHandle(true))->fID;
+    gl->BindTexture(GL_TEXTURE_2D, textureID);
+
+    // Re-use the texture's existing mailbox, if there is one.
+    if (m_image->getTexture()->getCustomData()) {

[Justin Novosad, 2016/06/07 15:11:07]

I believe skia team wants to deprecate custom data. Did you check with bsalomon
before using this?  Also packaging the mailbox into skia seems unnecessary. Why
not just use m_mailbox? Having the texture referenced by both an SkImage_Gpu and
a mailbox was a complexity I was hoping to avoid, to make resource management
easier to reason about.  With the current design there are several risks of
introducing resource leaks in the future, though I don't think there are leaks
right now. For example, if StaticBitmapImage is destroyed with an SkImage that
has a mailbox stored in m_image's custom data, the texture is leaked because
we'll be discarding the mailbox name without having consumed it.

[xidachen, 2016/06/07 17:49:53]

change it to use m_mailbox directly.

+        DCHECK(m_image->getTexture()->getCustomData()->size() == sizeof(m_mailbox.name));
+        memcpy(&m_mailbox.name[0], m_image->getTexture()->getCustomData()->data(), sizeof(m_mailbox.name));
+    } else {
+        gl->GenMailboxCHROMIUM(m_mailbox.name);
+        RefPtr<SkData> mailboxNameData = adoptRef(SkData::NewWithCopy(&m_mailbox.name[0], sizeof(m_mailbox.name)));
+        m_image->getTexture()->setCustomData(mailboxNameData.get());
+        gl->ProduceTextureCHROMIUM(GL_TEXTURE_2D, m_mailbox.name);
+    }
+    const GLuint64 fenceSync = gl->InsertFenceSyncCHROMIUM();
+    gl->Flush();

[Justin Novosad, 2016/06/07 15:11:07]

Why is this flush necessary? fence should be enough

[xidachen, 2016/06/07 17:49:54]

Layout test fails when I comment out this line. Also, the
DrawingBuffer::prepareMailbox has a flush.

+    gl->GenSyncTokenCHROMIUM(fenceSync, m_mailbox.syncToken);
+    m_mailbox.validSyncToken = true;
+    gl->BindTexture(GL_TEXTURE_2D, 0);
+    grContext->resetContext(kTextureBinding_GrGLBackendState);
+    return true;
+}
+
+PassRefPtr<SkImage> StaticBitmapImage::imageForCurrentFrame(WebGraphicsContext3DProvider* contextProvider)
+{
+    if (m_image) {
+        if (!m_image->isTextureBacked() || !contextProvider)

[Justin Novosad, 2016/06/07 15:11:07]

What is expected to happen if contextProvider is null when m_image is texture
backed?
Seems like we should have DCHECK(!m_image->isTextureBacked() ||
contextProvider)?

[xidachen, 2016/06/07 17:49:54]

In the case when m_image is not null and is texture backed, then there is always
a m_provider, so it goes through the code path of
prepareMailboxForSkImage-->consumeTextureMailbox.

This DCHECK could cause problem in the above case, isn't it?

+            return m_image;
+        if (m_provider) {
+            if (!prepareMailboxForSkImage(contextProvider))

[Justin Novosad, 2016/06/07 15:11:07]

This should only be necessary if m_provider != contextPRovider

[xidachen, 2016/06/07 17:49:53]

Done.

+                return m_image;
+        }
+        consumeTextureMailbox(contextProvider);
+        return m_image;
+    }
+    if (!contextProvider) {
+        DCHECK(isMainThread());
+        // TODO(xidachen): make this work on a worker thread.
+        OwnPtr<WebGraphicsContext3DProvider> provider = adoptPtr(Platform::current()->createSharedOffscreenGraphicsContext3DProvider());
+        consumeTextureMailbox(provider.get());
+        m_provider = std::move(provider);
+    } else {
+        consumeTextureMailbox(contextProvider);
+    }
     return m_image;
 }
 
 } // namespace blink