Implement WebGL's commit on the main thread

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

Index: third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp b/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
index 137299ee91d4b4ba89685e00a6a616e0bef09cb3..5c3c56fe3c3027288f2eb0024302803eb1fb4f2e 100644
--- a/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
+++ b/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
@@ -9,8 +9,11 @@
 #include "cc/quads/render_pass.h"
 #include "cc/quads/shared_quad_state.h"
 #include "cc/quads/solid_color_draw_quad.h"
+#include "cc/quads/texture_draw_quad.h"
+#include "cc/resources/returned_resource.h"
 #include "public/platform/InterfaceProvider.h"
 #include "public/platform/Platform.h"
+#include "third_party/khronos/GLES2/gl2.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "third_party/skia/include/core/SkXfermode.h"
 #include "ui/gfx/geometry/rect.h"
@@ -22,15 +25,16 @@ OffscreenCanvasFrameDispatcherImpl::OffscreenCanvasFrameDispatcherImpl(uint32_t
     : m_surfaceId(cc::SurfaceId(clientId, localId, nonce))
     , m_width(width)
     , m_height(height)
+    , m_nextResourceId(1u)
+    , m_binding(this)
 {
     DCHECK(!m_service.is_bound());
     Platform::current()->interfaceProvider()->getInterface(mojo::GetProxy(&m_service));
+    m_service->SetClient(m_binding.CreateInterfacePtrAndBind());
 }
 
-void OffscreenCanvasFrameDispatcherImpl::dispatchFrame()
+void OffscreenCanvasFrameDispatcherImpl::dispatchFrame(RefPtr<StaticBitmapImage> image)

[danakj, 2016/09/14 17:32:04]

One more Q: why is this a StaticBitmapImage instead of an
AcceleratedBitmapImage? Do you intend to support passing
non-AcceleratedBitmapImage here?

[xidachen, 2016/09/14 17:35:57]

danakj@: yes, for example, when we support
OffscreenCanvasRenderingContext2D.commit(), it can be a StaticBitmap that is in
cpu memory, instead of texture backed.

[danakj, 2016/09/14 17:36:56]

Ok. *if* you can strongly type that (ie make that a different method) that might
be nice. If not then makes sense!

 {
-    // TODO(563852/xlai): Currently this is just a simple solid-color compositor
-    // frame. We need to update this function to extract the image data from canvas.
     cc::CompositorFrame frame;
     frame.metadata.device_scale_factor = 1.0f;
     frame.delegated_frame_data.reset(new cc::DelegatedFrameData);
@@ -43,13 +47,56 @@ void OffscreenCanvasFrameDispatcherImpl::dispatchFrame()
     cc::SharedQuadState* sqs = pass->CreateAndAppendSharedQuadState();
     sqs->SetAll(gfx::Transform(), bounds.size(), bounds, bounds, false, 1.f, SkXfermode::kSrcOver_Mode, 0);
 
-    cc::SolidColorDrawQuad* quad = pass->CreateAndAppendDrawQuad<cc::SolidColorDrawQuad>();
-    const bool forceAntialiasingOff = false;
-    quad->SetNew(sqs, bounds, bounds, SK_ColorGREEN, forceAntialiasingOff);
+    // TODO(xidachen): for now, we just submit a SolidColor frame to compositor in the 2d case,
+    // we should extract data from image.
+    if (!image) {
+        cc::SolidColorDrawQuad* quad = pass->CreateAndAppendDrawQuad<cc::SolidColorDrawQuad>();
+        const bool forceAntialiasingOff = false;
+        quad->SetNew(sqs, bounds, bounds, SK_ColorGREEN, forceAntialiasingOff);
+    } else { // WebGL
+        DCHECK(image->isTextureBacked());
+        cc::TransferableResource resource;
+        resource.id = m_nextResourceId;

[danakj, 2016/09/14 02:28:33]

can you store the id you will use in a local var here and increment there, then
use the local var throughout. that will be less fragile.

[xidachen, 2016/09/14 13:06:43]

Done.

+        resource.format = cc::ResourceFormat::RGBA_8888;
+        // TODO(crbug.com/645590): filter should respect the image-rendering CSS property of associated canvas element.
+        resource.filter = GL_LINEAR;
+        resource.size = gfx::Size(m_width, m_height);
+        image->ensureMailbox();

[danakj, 2016/09/14 02:28:33]

it feels weird that you're doing this here. why are we getting here with an
|image| that doesn't have a mailbox yet?

[xidachen, 2016/09/14 13:06:43]

It was suggested by junov@ that calling this function should be no harm to the
current situation, and would prevent future bugs in 2D canvas cases.

[danakj, 2016/09/14 17:32:04]

Ok I'll defer to junov then, it just feels like an enapsulation violation to me
to put this here (and maybe to have this method public at all). But I have to
admit I'm not super familiar with the classes involved.

+        resource.mailbox_holder = gpu::MailboxHolder(image->getMailbox(), image->getSyncToken(), GL_TEXTURE_2D);
+        resource.read_lock_fences_enabled = false;
+        resource.is_software = false;
+        // TODO(crbug.com/646022): making this overlay-able.
+        resource.is_overlay_candidate = false;
+        frame.delegated_frame_data->resource_list.push_back(std::move(resource));
+
+        // Increment ref to the |image| by 1, to keep it alive until the browser ReturnResources.
+        m_cachedImages.add(m_nextResourceId++, image);
+
+        cc::TextureDrawQuad* quad = pass->CreateAndAppendDrawQuad<cc::TextureDrawQuad>();
+        gfx::Size rectSize(m_width, m_height);
+
+        const bool needsBlending = true;
+        // TOOD(crbug.com/645993): this should be inherited from WebGL context's creation settings.
+        const bool premultipliedAlpha = true;
+        const gfx::PointF uvTopLeft(0.f, 0.f);
+        const gfx::PointF uvBottomRight(1.f, 1.f);
+        float vertexOpacity[4] = { 1.f, 1.f, 1.f, 1.f };
+        const bool yflipped = false;
+        // TODO(crbug.com/645994): this should be true when using style "image-rendering: pixelated".
+        const bool nearestNeighbor = false;
+        quad->SetAll(sqs, bounds, bounds, bounds, needsBlending, resource.id, gfx::Size(), premultipliedAlpha,
+            uvTopLeft, uvBottomRight, SK_ColorTRANSPARENT, vertexOpacity, yflipped, nearestNeighbor, false);
+    }
 
     frame.delegated_frame_data->render_pass_list.push_back(std::move(pass));
 
     m_service->SubmitCompositorFrame(m_surfaceId, std::move(frame));
 }
 
+void OffscreenCanvasFrameDispatcherImpl::ReturnResources(Vector<cc::mojom::blink::ReturnedResourcePtr> resources)
+{
+    for (const cc::mojom::blink::ReturnedResourcePtr& resource : resources)

[danakj, 2016/09/14 02:28:33]

use const auto&

[xidachen, 2016/09/14 13:06:43]

Done.

+        m_cachedImages.remove(resource->id);

[danakj, 2016/09/14 02:28:33]

Does this delete the texture in the StaticBitmapImage? If so can you comment to
explain so (if this was the last ref I guess, can other refs exist?). If not you
need to delete it at some point, where would it happen?

also TODO to recycle the texture and reuse it for future frames?

[xidachen, 2016/09/14 13:06:43]

I believe this is the last ref to the StaticBitmapImage, and no other refs
should exist when we come to this point.

junov@: I believe that when a StaticBitmapImage is texture backed, then deleting
the last ref will delete its associated texture as well. Could you confirm that?

+}
+
 } // namespace blink