Implement OffscreenCanvas Unaccelerated 2d commit() on main thread (issue 2360413002 by xlai@chromium.org)

[xl...@chromium.org]

Reviewers: Justin Novosad, Ken Russell
CL: https://codereview.chromium.org/2360413002/

Message:
ju...@chromium.org: Please review changes in OffscreenCanvasFrameDispatcherImpl

k...@chromium.org: Please review changes in gpu pixel test




Description:
Implement OffscreenCanvas Unaccelerated 2d commit() on main thread

BUG=563852
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Affected files (+53, -9 lines):
M content/test/gpu/gpu_tests/pixel_expectations.py
M content/test/gpu/page_sets/pixel_tests.py
M third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h
M third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp


Index: content/test/gpu/gpu_tests/pixel_expectations.py
diff --git a/content/test/gpu/gpu_tests/pixel_expectations.py b/content/test/gpu/gpu_tests/pixel_expectations.py
index 801862a8e8f5dbe2f6688c458ce540f2dc4634ff..f779b4e60f5107db1c8b3182ade02d1a74c971e6 100644
--- a/content/test/gpu/gpu_tests/pixel_expectations.py
+++ b/content/test/gpu/gpu_tests/pixel_expectations.py
@@ -34,6 +34,8 @@ class PixelExpectations(GpuTestExpectations):
# TODO(xidachen) check / generate reference images
self.Fail('Pixel.OffscreenCanvasWebGLRedBoxWorker', bug=563852)
self.Fail('Pixel.OffscreenCanvasWebGLRedBoxWorkerES3', ['mac'], bug=563852)
+ self.Fail('Pixel.OffscreenCanvasAccelerated2D', bug=563852)
+ self.Fail('Pixel.OffscreenCanvasAccelerated2DWorker', bug=563852)

self.Fail('Pixel.OffscreenCanvasAccelerated2D', bug=563852)
self.Fail('Pixel.OffscreenCanvasAccelerated2DES3', ['mac'], bug=563852)
@@ -41,5 +43,7 @@ class PixelExpectations(GpuTestExpectations):
self.Fail('Pixel.OffscreenCanvasAccelerated2DWorkerES3', ['mac'],
bug=563852)

+ self.Fail('Pixel.OffscreenCanvasUnaccelerated2D', bug=563852)
+
# TODO(kbr): flakily timing out on this configuration.
self.Flaky('*', ['linux', 'intel', 'debug'], bug=648369)
Index: content/test/gpu/page_sets/pixel_tests.py
diff --git a/content/test/gpu/page_sets/pixel_tests.py b/content/test/gpu/page_sets/pixel_tests.py
index 28bf9b4ed254b93118d4cbddd6faa5104ef0d1dd..449d934c47c6141bb6bea2383822fd4e644f65f9 100644
--- a/content/test/gpu/page_sets/pixel_tests.py
+++ b/content/test/gpu/page_sets/pixel_tests.py
@@ -48,6 +48,15 @@ class IOSurface2DCanvasSharedPageState(gpu_test_base.GpuSharedPageState):
['--enable-accelerated-2d-canvas',
'--disable-display-list-2d-canvas'])

+class SoftwareOffscreenCanvas2DSharedPageState(
+ gpu_test_base.GpuSharedPageState):
+ def __init__(self, test, finder_options, story_set):
+ super(SoftwareOffscreenCanvas2DSharedPageState, self).__init__(
+ test, finder_options, story_set)
+ finder_options.browser_options.AppendExtraBrowserArgs(
+ ['--disable-accelerated-2d-canvas',
+ '--disable-gpu-compositing',
+ '--enable-experimental-canvas-features'])

class WebGLNonChromiumImageSharedPageState(gpu_test_base.GpuSharedPageState):
def __init__(self, test, finder_options, story_set):
@@ -211,6 +220,15 @@ class PixelTestsStorySet(story_set_module.StorySet):
expectations=expectations))

self.AddStory(PixelTestsPage(
+ url='file://../../data/gpu/pixel_acceleratedOffscreen2d_commit_main.html',
+ name=base_name + '.OffscreenCanvasUnaccelerated2D' + es3_suffix,
+ test_rect=[0, 0, 350, 350],
+ revision=1,
+ story_set=self,
+ shared_page_state_class=SoftwareOffscreenCanvas2DSharedPageState,
+ expectations=expectations))
+
+ self.AddStory(PixelTestsPage(
url='file://../../data/gpu/pixel_canvas2d.html',
name=base_name + '.Canvas2DRedBox' + es3_suffix,
test_rect=[0, 0, 300, 300],
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()) {
+ 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);
+ resource.mailbox_holder.mailbox = bitmap->id();
+ resource.mailbox_holder.texture_target = 0;
+ resource.is_software = true;
+
+ m_sharedBitmaps.add(m_nextResourceId++, std::move(bitmap));
+ } 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.
+ m_cachedImages.add(m_nextResourceId++, std::move(image));
+ }
// TODO(crbug.com/646022): making this overlay-able.
resource.is_overlay_candidate = false;

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)
+ m_sharedBitmaps.remove(resource->id);
}

bool OffscreenCanvasFrameDispatcherImpl::verifyImageSize(const sk_sp<SkImage>& image)
Index: third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h
diff --git a/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h b/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h
index 52384bb356bd6bf02879d2079d66b16489dd8e4e..1ea3c161e0c52508c6476362ea689bbf75984af1 100644
--- a/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h
+++ b/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h
@@ -6,6 +6,7 @@
#define OffscreenCanvasFrameDispatcherImpl_h

#include "cc/ipc/mojo_compositor_frame_sink.mojom-blink.h"
+#include "cc/resources/shared_bitmap.h"
#include "cc/surfaces/surface_id.h"
#include "mojo/public/cpp/bindings/binding.h"
#include "platform/graphics/OffscreenCanvasFrameDispatcher.h"
@@ -34,6 +35,7 @@ private:

unsigned m_nextResourceId;
HashMap<unsigned, RefPtr<StaticBitmapImage>> m_cachedImages;
+ HashMap<unsigned, std::unique_ptr<cc::SharedBitmap>> m_sharedBitmaps;

bool verifyImageSize(const sk_sp<SkImage>&);

[dan...@chromium.org]

Some drive-by comments about gpu vs software compositing.


https://codereview.chromium.org/2360413002/diff/40001/content/test/gpu/page_sets/pixel_tests.py
File content/test/gpu/page_sets/pixel_tests.py (right):

https://codereview.chromium.org/2360413002/diff/40001/content/test/gpu/page_sets/pixel_tests.py#newcode58
content/test/gpu/page_sets/pixel_tests.py:58:
'--disable-gpu-compositing',
Which this verifies software compositing when it's blacklisted, we also
fall into software compositing when the gpu process fails to make a
context 3 times (see RenderWidgetCompositor for this). In that case you
can't rely on canvas being disabled by runtime flags and stuff. So while
this makes sense for the pixel test, the code needs to handle more cases
and you probably want to unit test that.

https://codereview.chromium.org/2360413002/diff/40001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2360413002/diff/40001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode72
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:72:

if (!image->isTextureBacked() &&
!RuntimeEnabledFeatures::accelerated2dCanvasEnabled()) {

RuntimeEnabledFeatures::accelerated2dCanvasEnabled() doesn't reflect if
the compositor is going to expect a software or hardware frame, but I
was expecting at least a todo here.

https://codereview.chromium.org/2360413002/

[ju...@chromium.org]

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode72

third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:72:
if (!image->isTextureBacked() &&
!RuntimeEnabledFeatures::accelerated2dCanvasEnabled()) {

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

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode81
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:81:
resource.mailbox_holder.texture_target = 0;
Is this line really necessary?

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode84
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:84:
m_sharedBitmaps.add(m_nextResourceId++, std::move(bitmap));
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.

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode92
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:92:

// It guarantees that the resource is not re-used or deleted.

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

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode126
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:126:

for (const auto& resource : resources)

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.

https://codereview.chromium.org/2360413002/

[dan...@chromium.org]

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode72
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:72:
if (!image->isTextureBacked() &&
!RuntimeEnabledFeatures::accelerated2dCanvasEnabled()) {

On 2016/09/23 20:54:52, Justin Novosad wrote:
> 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

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.

https://codereview.chromium.org/2360413002/

[xida...@chromium.org]

https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/gpu_tests/pixel_expectations.py
File content/test/gpu/gpu_tests/pixel_expectations.py (right):

https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/gpu_tests/pixel_expectations.py#newcode38
content/test/gpu/gpu_tests/pixel_expectations.py:38:
self.Fail('Pixel.OffscreenCanvasAccelerated2DWorker', bug=563852)
These two entries seem to be duplicated.

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode72
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:72:
if (!image->isTextureBacked() &&
!RuntimeEnabledFeatures::accelerated2dCanvasEnabled()) {

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.

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode92
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:92:
// It guarantees that the resource is not re-used or deleted.

On 2016/09/23 20:54:52, Justin Novosad wrote:

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

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

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode96
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:96:
resource.is_overlay_candidate = false;
This line should be inside the else block.

https://codereview.chromium.org/2360413002/

[xida...@chromium.org]

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode96
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:96:
resource.is_overlay_candidate = false;

On 2016/09/24 01:20:20, xidachen wrote:
> This line should be inside the else block.

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

https://codereview.chromium.org/2360413002/

[k...@chromium.org]

The tests will LGTM once a couple of small changes are made.

I defer to Dana's, Justin's and Xida's double-checking of that area and for
reviews of the rest of the CL.

https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/gpu_tests/pixel_expectations.py
File content/test/gpu/gpu_tests/pixel_expectations.py (right):

https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/gpu_tests/pixel_expectations.py#newcode38
content/test/gpu/gpu_tests/pixel_expectations.py:38:
self.Fail('Pixel.OffscreenCanvasAccelerated2DWorker', bug=563852)

On 2016/09/24 01:20:20, xidachen wrote:

> These two entries seem to be duplicated.

Yup.

https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/gpu_tests/pixel_expectations.py#newcode47
content/test/gpu/gpu_tests/pixel_expectations.py:47:
self.Fail('Pixel.OffscreenCanvasUnaccelerated2DES3', ['mac'],
bug=563852)
The ES3 version of this suppression isn't needed -- see below.

https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/page_sets/pixel_tests.py
File content/test/gpu/page_sets/pixel_tests.py (right):

https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/page_sets/pixel_tests.py#newcode229
content/test/gpu/page_sets/pixel_tests.py:229:
expectations=expectations))
Could you put this in __init__ instead, after the call to
self._AddAllPages(expectations, base_name, False)? As in Xida's
https://codereview.chromium.org/2365653005/ , since this test requires a
special SharedPageState, it won't run in both ES2 and ES3 modes anyway
so doesn't need to be in this block.

https://codereview.chromium.org/2360413002/

[xida...@chromium.org]

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode79
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:79:

image->imageForCurrentFrame()->readPixels(imageInfo, pixels,
imageInfo.minRowBytes(), 0, 0);

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?

https://codereview.chromium.org/2360413002/

[ju...@chromium.org]

That does not work because SharedBitmaps are allocated in shared memory so that
they can be read by another process. The address pointed to by peekPixels will
be in the current process's local address space.
That said, a possible future optimization would be to allocate the canvas
backings in shared memory right off the bat, to avoid the copy later, but we'd
have to be careful about that, to only do it for canvases that use the commit
flow (e.g. Only canvas contexts associated with OffscreenCanvases that have an
associated surface layer, or something like that)

https://codereview.chromium.org/2360413002/

[ju...@chromium.org]

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode92
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:92:
// It guarantees that the resource is not re-used or deleted.

On 2016/09/24 01:20:20, xidachen wrote:

> On 2016/09/23 20:54:52, Justin Novosad wrote:
> > Improve this comment to explain why it is not necessary to do this
for the
> > non-texture code path
>
> junov@: this is not software path, it is hardware path because it is
inside the
> else block.

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.

https://codereview.chromium.org/2360413002/

[dan...@chromium.org]

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode92
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:92:
// It guarantees that the resource is not re-used or deleted.

On 2016/09/27 15:11:15, Justin Novosad wrote:
> On 2016/09/24 01:20:20, xidachen wrote:
> > On 2016/09/23 20:54:52, Justin Novosad wrote:
> > > Improve this comment to explain why it is not necessary to do this
for the
> > > non-texture code path
> >
> > junov@: this is not software path, it is hardware path because it is
inside
> the
> > else block.
>
> 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.

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

https://codereview.chromium.org/2360413002/

[xida...@chromium.org]

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode92
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:92:
// It guarantees that the resource is not re-used or deleted.

On 2016/09/27 19:05:20, danakj wrote:
> On 2016/09/27 15:11:15, Justin Novosad wrote:
> > On 2016/09/24 01:20:20, xidachen wrote:
> > > On 2016/09/23 20:54:52, Justin Novosad wrote:
> > > > Improve this comment to explain why it is not necessary to do
this for the
> > > > non-texture code path
> > >
> > > junov@: this is not software path, it is hardware path because it
is inside
> > the
> > > else block.
> >
> > 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.
>
> I think it is needed. Deleting the shared memory before the browser
receives the
> IPC would be equally bad?

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

[dan...@chromium.org]

Ah okay, yes. Let's make the comments explain this. I was assuming we were giving the SkImage a shared memory backing and shipping that directly, not copying pixels.

 

https://codereview.chromium.org/2360413002/

[xl...@chromium.org]

I've updated the CL and addressed all feedback.

https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/gpu_tests/pixel_expectations.py
File content/test/gpu/gpu_tests/pixel_expectations.py (right):

https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/gpu_tests/pixel_expectations.py#newcode47
content/test/gpu/gpu_tests/pixel_expectations.py:47:
self.Fail('Pixel.OffscreenCanvasUnaccelerated2DES3', ['mac'],
bug=563852)

On 2016/09/26 22:29:29, Ken Russell wrote:
> The ES3 version of this suppression isn't needed -- see below.

Done.

https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/page_sets/pixel_tests.py
File content/test/gpu/page_sets/pixel_tests.py (right):

https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/page_sets/pixel_tests.py#newcode229
content/test/gpu/page_sets/pixel_tests.py:229:
expectations=expectations))

On 2016/09/26 22:29:30, Ken Russell wrote:
> Could you put this in __init__ instead, after the call to
> self._AddAllPages(expectations, base_name, False)? As in Xida's
> https://codereview.chromium.org/2365653005/ , since this test requires
a special
> SharedPageState, it won't run in both ES2 and ES3 modes anyway so
doesn't need
> to be in this block.

Done.

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode72
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:72:
if (!image->isTextureBacked() &&
!RuntimeEnabledFeatures::accelerated2dCanvasEnabled()) {

On 2016/09/23 20:54:52, Justin Novosad wrote:

> 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

Done.

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode81
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:81:
resource.mailbox_holder.texture_target = 0;

On 2016/09/23 20:54:52, Justin Novosad wrote:

> Is this line really necessary?

Done.

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode84
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:84:
m_sharedBitmaps.add(m_nextResourceId++, std::move(bitmap));

On 2016/09/23 20:54:52, Justin Novosad wrote:

> 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.

Done. Put that in the function getNextResourceIdAndIncrement().

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode92
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:92:
// It guarantees that the resource is not re-used or deleted.

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.

https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode126
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:126:
for (const auto& resource : resources)

On 2016/09/23 20:54:52, Justin Novosad wrote:

> 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.

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.

https://codereview.chromium.org/2360413002/

[xida...@chromium.org]

lgtm with nits.

Also, please wait for Ken's CL to land first:
https://codereview.chromium.org/2363343002/


https://codereview.chromium.org/2360413002/diff/100001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2360413002/diff/100001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode95
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:95:
m_cachedImages.add(getNextResourceIdAndIncrement(), std::move(image));
My understanding of junov@'s comment is that you do:
m_sharedBitmaps.add(m_nextResourceId, ...) and
m_cachedImages.add(m_nextResourceId, ...)

and then outside of this if--else block, you do a m_nextResourceId++;

If there is any early exit in the if--else block, the m_nextResourceId
will not increase, so it should be safe.

https://codereview.chromium.org/2360413002/

[k...@chromium.org]

https://codereview.chromium.org/2360413002/diff/100001/content/test/gpu/gpu_tests/pixel_expectations.py
File content/test/gpu/gpu_tests/pixel_expectations.py (right):

https://codereview.chromium.org/2360413002/diff/100001/content/test/gpu/gpu_tests/pixel_expectations.py#newcode35
content/test/gpu/gpu_tests/pixel_expectations.py:35:
self.Fail('Pixel.OffscreenCanvasUnaccelerated2D', bug=563852)
The naming convention's changed in
https://codereview.chromium.org/2363343002/ ; should now be
"Pixel_OffscreenCanvasUnaccelerated2D".

https://codereview.chromium.org/2360413002/diff/100001/content/test/gpu/page_sets/pixel_tests.py
File content/test/gpu/page_sets/pixel_tests.py (right):

https://codereview.chromium.org/2360413002/diff/100001/content/test/gpu/page_sets/pixel_tests.py#newcode149
content/test/gpu/page_sets/pixel_tests.py:149:
self.AddStory(PixelTestsPage(
This new page should now be added to
src/content/test/gpu/gpu_tests/pixel_test_pages.py instead, now that
https://codereview.chromium.org/2363343002/ has landed. Please let me
know if you have any questions about the new format. You no longer need
to specify a shared page state, just the browser command line arguments
you want per-page.

https://codereview.chromium.org/2360413002/

[xl...@chromium.org]

ken@: can you take a look at the new patch again? I've rebased it based on your
recently landed patch https://codereview.chromium.org/2363343002/.
I rename the html files because now I know that they are
supposed to be shared by both accelerated and unaccelerated cases, and thus the
name "accelerated" is no longer appropriate.

junov@: gentle ping on reviews on OffscreenCanvasFrameDispatcherImpl

https://codereview.chromium.org/2360413002/

[ju...@chromium.org]

lgtm

https://codereview.chromium.org/2360413002/

[k...@chromium.org]

lgtm with one small test change.



https://codereview.chromium.org/2360413002/diff/140001/content/test/gpu/gpu_tests/pixel_test_pages.py
File content/test/gpu/gpu_tests/pixel_test_pages.py (right):

https://codereview.chromium.org/2360413002/diff/140001/content/test/gpu/gpu_tests/pixel_test_pages.py#newcode190
content/test/gpu/gpu_tests/pixel_test_pages.py:190: PixelTestPage(
Could you just add this to the above ExperimentalCanvasFeaturesPages
set, above? You can just say:

browser_args = browser_args + [
'--disable-accelerated-2d-canvas',
'--disable-gpu-compositing']

That way you don't need to touch pixel_integration_test at all.

https://codereview.chromium.org/2360413002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2360413002/160001


https://codereview.chromium.org/2360413002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #9 (id:160001)

https://codereview.chromium.org/2360413002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 9 (id:??) landed as
https://crrev.com/f2246b3d3631e26bdf958bbd59cadd57938826b8
Cr-Commit-Position: refs/heads/master@{#422231}

https://codereview.chromium.org/2360413002/