Implement OffscreenCanvas Unaccelerated 2d commit() on 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 d97c9a85a77abd71a28cfce4c079bd2593759e95..46011a0a7bb4e7d2bfe08ba11c08b12de8914752 100644
--- a/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
+++ b/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
@@ -11,6 +11,7 @@
 #include "cc/quads/solid_color_draw_quad.h"
 #include "cc/quads/texture_draw_quad.h"
 #include "cc/resources/returned_resource.h"
+#include "platform/RuntimeEnabledFeatures.h"
 #include "public/platform/InterfaceProvider.h"
 #include "public/platform/Platform.h"
 #include "public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom-blink.h"
@@ -54,8 +55,9 @@ void OffscreenCanvasFrameDispatcherImpl::dispatchFrame(RefPtr<StaticBitmapImage>
     cc::SharedQuadState* sqs = pass->CreateAndAppendSharedQuadState();
     sqs->SetAll(gfx::Transform(), bounds.size(), bounds, bounds, false, 1.f, SkXfermode::kSrcOver_Mode, 0);
 
-    if (!image->isTextureBacked()) {
-        // TODO(xlai): Make unaccelerated 2d canvas work. See crbug.com/563858
+    if (!image->isTextureBacked() && !isMainThread()) {
+        // TODO(xlai): Implement unaccelerated 2d canvas on worker.
+        // See crbug.com/563858.
         // This is a temporary code that submits a solidColor frame.
         cc::SolidColorDrawQuad* quad = pass->CreateAndAppendDrawQuad<cc::SolidColorDrawQuad>();
         const bool forceAntialiasingOff = false;
@@ -67,18 +69,34 @@ void OffscreenCanvasFrameDispatcherImpl::dispatchFrame(RefPtr<StaticBitmapImage>
         // 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();
-        resource.mailbox_holder = gpu::MailboxHolder(image->getMailbox(), image->getSyncToken(), GL_TEXTURE_2D);
-        resource.read_lock_fences_enabled = false;
-        resource.is_software = false;
+        if (!image->isTextureBacked() && !RuntimeEnabledFeatures::accelerated2dCanvasEnabled()) {

[Justin Novosad, 2016/09/23 20:54:52]

You do not need to check RuntimeEnabledFeatures::accelerated2dCanvasEnabled(). 
The only thing that matters is whether this particular image is in software. 
For example, if acceleratedCanvases are enabled, but this particular canvas is
not accelerated (which can happen for many reasons), then you'd want to use the
SharedBitmap path

[danakj, 2016/09/23 20:56:02]

Can the image be not hardware but the compositor is using GPU compositing?
Because if that's the case we need to upload to a texture to hand to the
compositor.

[xidachen, 2016/09/24 01:20:20]

danakj@: Yes, that is possible. I will have a CL implementing that. In that
case, my opinion is to upload the |image| from cpu memory to gpu memory, and let
CompositorFrame refer to the gpu memory.

junov@: In my opinion, there should be two code path for the case when
!image->isTextureBacked() depending on whether compositor is gpu or software.

[xlai (Olivia), 2016/09/29 15:53:07]

Done.

+            std::unique_ptr<cc::SharedBitmap> bitmap = Platform::current()->allocateSharedBitmap(IntSize(m_width, m_height));
+            if (!bitmap)
+                return;
+            unsigned char* pixels = bitmap->pixels();
+            DCHECK(pixels);
+            SkImageInfo imageInfo = SkImageInfo::Make(m_width, m_height, kN32_SkColorType, image->isPremultiplied() ? kPremul_SkAlphaType : kUnpremul_SkAlphaType);
+            image->imageForCurrentFrame()->readPixels(imageInfo, pixels, imageInfo.minRowBytes(), 0, 0);

[xidachen, 2016/09/27 11:22:44]

junov@: I have some thoughts about this line. Right now it is using readPixels()
which makes a copy of the SkImage, and then cc::SharedBitmaps refers to that
copy.

Why don't we use peekPixels()? We know that |image| is not texture backed, so it
has to live in cpu memory. We can use peekPixels() and let the cc::SharedBitmaps
pointing to the memory that SkImage is holding. peekPixels() doesn't make an
extra copy which saves a lot.

Does that make sense?

+            resource.mailbox_holder.mailbox = bitmap->id();
+            resource.mailbox_holder.texture_target = 0;

[Justin Novosad, 2016/09/23 20:54:52]

Is this line really necessary?

[xlai (Olivia), 2016/09/29 15:53:07]

Done.

+            resource.is_software = true;
+
+            m_sharedBitmaps.add(m_nextResourceId++, std::move(bitmap));

[Justin Novosad, 2016/09/23 20:54:52]

I know this is not currently a bug, but could we do the increment
(m_nextResourceId++) in only one place.  Make the code more readable.

[xlai (Olivia), 2016/09/29 15:53:07]

Done. Put that in the function getNextResourceIdAndIncrement().

+        } else {
+            image->ensureMailbox();
+            resource.mailbox_holder = gpu::MailboxHolder(image->getMailbox(), image->getSyncToken(), GL_TEXTURE_2D);
+            resource.read_lock_fences_enabled = false;
+            resource.is_software = false;
+
+            // Hold ref to |image|, to keep it alive until the browser ReturnResources.
+            // It guarantees that the resource is not re-used or deleted.

[Justin Novosad, 2016/09/23 20:54:52]

Improve this comment to explain why it is not necessary to do this for the
non-texture code path

[xidachen, 2016/09/24 01:20:20]

junov@: this is not software path, it is hardware path because it is inside the
else block.

[Justin Novosad, 2016/09/27 15:11:15]

Let me rephrase. Please explain, in the comments, why the software path (above)
does not need to ref |image|, as we do here in the GPU path.

[danakj, 2016/09/27 19:05:20]

I think it is needed. Deleting the shared memory before the browser receives the
IPC would be equally bad?

[xidachen, 2016/09/27 20:17:23]

In my opinion, this is not needed for software path. The code:
image->imageForCurrentFrame()->readPixels(...)

makes a copy of the pixel data, and put it in bitmap->pixels(), so as long as
m_sharedBitmaps keeps the bitmap, it should be fine. The |image| should not be
needed anymore because the pixel has been copied. Does that make sense?

[xlai (Olivia), 2016/09/29 15:53:07]

Exactly. In the software path, after pixels are finished copying, the |image| is
no longer needed so we don't care whether it dies off or not.
Only the shared bitmap needs to be kept alive, and that's why I maintain the
m_sharedBitmaps. 
For this part, I am adding a symmetric comment on top of the line
m_sharedBitmaps.add(...). I think such a comment is quite clear to tell readers
that exactly what references of images need to be hold alive.

+            m_cachedImages.add(m_nextResourceId++, std::move(image));
+        }
         // TODO(crbug.com/646022): making this overlay-able.
         resource.is_overlay_candidate = false;

[xidachen, 2016/09/24 01:20:20]

This line should be inside the else block.

[xidachen, 2016/09/24 01:24:52]

Oh, my bad, I believe your intention is to have this line for both software and
hardware path. It is fine, then.

 
         frame.delegated_frame_data->resource_list.push_back(std::move(resource));
 
-        // Hold ref to |image|, to keep it alive until the browser ReturnResources.
-        // It guarantees that the resource is not re-used or deleted.
-        m_cachedImages.add(m_nextResourceId++, std::move(image));
 
         cc::TextureDrawQuad* quad = pass->CreateAndAppendDrawQuad<cc::TextureDrawQuad>();
         gfx::Size rectSize(m_width, m_height);
@@ -105,6 +123,8 @@ void OffscreenCanvasFrameDispatcherImpl::ReturnResources(Vector<cc::mojom::blink
 {
     for (const auto& resource : resources)
         m_cachedImages.remove(resource->id);
+    for (const auto& resource : resources)

[Justin Novosad, 2016/09/23 20:54:52]

You could have a single 'for' loop here.
Also, for each resource, you can check resource.is_software to know which map it
needs to be removed from. 
You should also put some DCHECKs here to verify that the resource is in fact
present in the map where you expect to find it.

[xlai (Olivia), 2016/09/29 15:53:07]

I can't. The ReturnedResource does not have is_software or any other equivalent
attribute that can tell me whether is resource is for software or accelerated
mode.

But I put it in one for loop now.

+        m_sharedBitmaps.remove(resource->id);
 }
 
 bool OffscreenCanvasFrameDispatcherImpl::verifyImageSize(const sk_sp<SkImage>& image)