Implement OffscreenCanvas Accelerated 2D commit()

Implement OffscreenCanvas Accelerated 2D commit()

This CL makes commit() on accelerated 2d context on OffscreenCanvas work.

BUG=563852
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/971faed800d30243b9ff37e5dbdfe3d55209d4ad
Cr-Commit-Position: refs/heads/master@{#420785}

[xlai (Olivia), 2016-09-21 17:32:48]

Description was changed from

==========
Implement OffscreenCanvasRenderingContext2D commit()



BUG=563852
==========

to

==========
Implement OffscreenCanvasRenderingContext2D commit()



BUG=563852
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[xlai (Olivia), 2016-09-22 16:08:39]

Description was changed from

==========
Implement OffscreenCanvasRenderingContext2D commit()



BUG=563852
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Implement OffscreenCanvas Accelerated 2D commit()

This CL makes commit() on accelerated 2d context on OffscreenCanvas work. 

BUG=563852
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[xlai (Olivia), 2016-09-22 16:09:01]

xlai@chromium.org changed reviewers:
+ junov@chromium.org, xidachen@chromium.org

[xlai (Olivia), 2016-09-22 16:09:27]

xlai@chromium.org changed reviewers:
+ kbr@chromium.org

[xlai (Olivia), 2016-09-22 16:09:48]

xlai@chromium.org changed reviewers:
- xidachen@chromium.org

[xlai (Olivia), 2016-09-22 16:12:24]

PTAL. Most of the code is just reuse of existing webgl
rendering context commit(), except that I added some verification
functions and fixed a few small mistakes.

Explanations of my code are inline.

I will follow up with another CL that implements
OffscreenCanvas unaccelerated 2d commit() on main thread.
For the unaccelerated 2d commit() on worker thread, it's
going to be more complex.

https://codereview.chromium.org/2359723003/diff/80001/content/test/data/gpu/p...
File content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_main.html
(right):

https://codereview.chromium.org/2359723003/diff/80001/content/test/data/gpu/p...
content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_main.html:53: <canvas
id="c" width="320" height="320" class="nomargin"></canvas>
The reason why I use a larger canvas is to trigger acceleration on 2d canvas.

https://codereview.chromium.org/2359723003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2359723003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:668:
getOffscreenCanvas()->getOrCreateFrameDispatcher()->dispatchFrame(std::move(drawingBuffer()->transferToStaticBitmapImage()),
this->isAccelerated());
In WebGL, this->isAccelerated() is confirmed to return true. But in 2D context,
it can return either true or false.

https://codereview.chromium.org/2359723003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(left):

https://codereview.chromium.org/2359723003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:51:
unsigned nextResourceId = 1u;
I'm moving nextResourceId to m_nextResourceId, because I think this variable
should be incremented each time when a new frame is submitted.

https://codereview.chromium.org/2359723003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2359723003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:43:
if (!verifyImageSize(image->imageForCurrentFrame()))
I'm adding this verification function. OffscreenCanvas currently does not accept
resizing; it's not right to submit a frame that has not the same size as
OffscreenCanvas.

[Justin Novosad, 2016-09-22 18:06:19]

https://codereview.chromium.org/2359723003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher.h
(right):

https://codereview.chromium.org/2359723003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher.h:19:
virtual void dispatchFrame(RefPtr<StaticBitmapImage>, bool isAccelerated) = 0;
You should not need the isAccelerated argument.  The statciBitmapImage can be
queried

https://codereview.chromium.org/2359723003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(left):

https://codereview.chromium.org/2359723003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:51:
unsigned nextResourceId = 1u;
On 2016/09/22 16:12:24, xlai (Olivia) wrote:
> I'm moving nextResourceId to m_nextResourceId, because I think this variable
> should be incremented each time when a new frame is submitted.

Correct.  This was a mistake in previous CL IMHO. Thanks for spotting it.

https://codereview.chromium.org/2359723003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2359723003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:113:
if (!!image && image->width() == m_width && image->height() == m_height)
!!image -> image

[xidachen, 2016-09-22 18:41:55]

xidachen@chromium.org changed reviewers:
+ xidachen@chromium.org

[xlai (Olivia), 2016-09-22 18:51:49]

xlai@chromium.org changed reviewers:
- kbr@chromium.org

[xlai (Olivia), 2016-09-22 18:51:50]

-kbr from reviewers because there is no more change to Webgl now.

Fixed according to junov@'s feedback.

[xlai (Olivia), 2016-09-22 18:53:42]

xlai@chromium.org changed reviewers:
+ kbr@chromium.org

[xlai (Olivia), 2016-09-22 18:53:43]

Adding kbr@ back to reviewers because the addition to GPU pixel tests need to be
reviewed...

[xidachen, 2016-09-22 19:05:55]

https://codereview.chromium.org/2359723003/diff/120001/content/test/data/gpu/...
File content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_main.html
(right):

https://codereview.chromium.org/2359723003/diff/120001/content/test/data/gpu/...
content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_main.html:11:
<title>OffscreenCanvas 2d commit flow on main thread: green square on white
background.</title>
It should be "blue" square.

https://codereview.chromium.org/2359723003/diff/120001/content/test/data/gpu/...
File content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_worker.html
(right):

https://codereview.chromium.org/2359723003/diff/120001/content/test/data/gpu/...
content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_worker.html:2: 
Since the implementation for worker is not ready yet, I would suggest removing
the test for worker at this moment. Otherwise it is a little confusing that you
have fillStyle = "blue" but seeing green in the results.

Test should be added when the implementation is done.

https://codereview.chromium.org/2359723003/diff/120001/content/test/gpu/gpu_t...
File content/test/gpu/gpu_tests/pixel_expectations.py (right):

https://codereview.chromium.org/2359723003/diff/120001/content/test/gpu/gpu_t...
content/test/gpu/gpu_tests/pixel_expectations.py:38:
self.Fail('Pixel.OffscreenCanvasAccelerated2DWorker', bug=563852)
You should rebase before committing this CL.

https://codereview.chromium.org/2359723003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/2359723003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h:91:
PassRefPtr<StaticBitmapImage> transferToStaticBitmapImage();
Please change this to RefPtr<> instead of PassRefPtr<> as we are deprecating the
latter one.

https://codereview.chromium.org/2359723003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2359723003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:113:
if (image && image->width() == m_width && image->height() == m_height)
Is if (image) needed? Before this function, we have 
if (!image)
    return;

so I feel like we can remove the |image| here.

[Justin Novosad, 2016-09-22 19:27:39]

https://codereview.chromium.org/2359723003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2359723003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp:57:
DCHECK(image->isTextureBacked() == this->isAccelerated());
Why this DCHECK?  If for some reason this ever becomes false in the future (an
accelerated context produces a non-accelerated snapshot, for example), it should
not be a problem.

[xlai (Olivia), 2016-09-22 19:35:35]

Patchset #8 (id:140001) has been deleted

[xlai (Olivia), 2016-09-22 19:36:18]

https://codereview.chromium.org/2359723003/diff/120001/content/test/data/gpu/...
File content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_main.html
(right):

https://codereview.chromium.org/2359723003/diff/120001/content/test/data/gpu/...
content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_main.html:11:
<title>OffscreenCanvas 2d commit flow on main thread: green square on white
background.</title>
On 2016/09/22 19:05:54, xidachen wrote:
> It should be "blue" square.

Done.

https://codereview.chromium.org/2359723003/diff/120001/content/test/data/gpu/...
File content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_worker.html
(right):

https://codereview.chromium.org/2359723003/diff/120001/content/test/data/gpu/...
content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_worker.html:2: 
Not really. The implementation for worker on accelerated 2d case is already
completed. And this test is working for accelerated 2d case.

Filling in blue is to distinguish with the solid color frame in 2d.
We shouldn't see green in this test's reference result. If it is, then it means
the gpu pixel test is doing the unaccelerated work.


On 2016/09/22 19:05:54, xidachen wrote:
> Since the implementation for worker is not ready yet, I would suggest removing
> the test for worker at this moment. Otherwise it is a little confusing that
you
> have fillStyle = "blue" but seeing green in the results.
> 
> Test should be added when the implementation is done.

https://codereview.chromium.org/2359723003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2359723003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp:57:
DCHECK(image->isTextureBacked() == this->isAccelerated());
On 2016/09/22 19:27:39, Justin Novosad wrote:
> Why this DCHECK?  If for some reason this ever becomes false in the future (an
> accelerated context produces a non-accelerated snapshot, for example), it
should
> not be a problem.

Done.

https://codereview.chromium.org/2359723003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/2359723003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h:91:
PassRefPtr<StaticBitmapImage> transferToStaticBitmapImage();
On 2016/09/22 19:05:55, xidachen wrote:
> Please change this to RefPtr<> instead of PassRefPtr<> as we are deprecating
the
> latter one.

Done.

https://codereview.chromium.org/2359723003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2359723003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:113:
if (image && image->width() == m_width && image->height() == m_height)
On 2016/09/22 19:05:55, xidachen wrote:
> Is if (image) needed? Before this function, we have 
> if (!image)
>     return;
> 
> so I feel like we can remove the |image| here.

Yes, it is needed. The "image" variable is referring to the SkImage inside the
StaticBitmapImage.

[xidachen, 2016-09-22 19:42:34]

https://codereview.chromium.org/2359723003/diff/120001/content/test/data/gpu/...
File content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_worker.html
(right):

https://codereview.chromium.org/2359723003/diff/120001/content/test/data/gpu/...
content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_worker.html:2: 
On 2016/09/22 19:36:18, xlai (Olivia) wrote:
> Not really. The implementation for worker on accelerated 2d case is already
> completed. And this test is working for accelerated 2d case.
> 
> Filling in blue is to distinguish with the solid color frame in 2d.
> We shouldn't see green in this test's reference result. If it is, then it
means
> the gpu pixel test is doing the unaccelerated work.
> 
> 
> On 2016/09/22 19:05:54, xidachen wrote:
> > Since the implementation for worker is not ready yet, I would suggest
removing
> > the test for worker at this moment. Otherwise it is a little confusing that
> you
> > have fillStyle = "blue" but seeing green in the results.
> > 
> > Test should be added when the implementation is done.
> 

That makes sense, I didn't realize that this is for *accelerated* case.

https://codereview.chromium.org/2359723003/diff/160001/content/test/data/gpu/...
File content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_worker.html
(right):

https://codereview.chromium.org/2359723003/diff/160001/content/test/data/gpu/...
content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_worker.html:23:
offscreen2d.commit();
As discussed with kbr@, please add a self.postMessage(""), it should be safer
than what it is now. At least not worse.

https://codereview.chromium.org/2359723003/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2359723003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp:128:
bool OffscreenCanvasRenderingContext2D::isAccelerated() const
Is there a reason to keep this function here, since we can obtain it from the
StaticBitmapImage?

[xlai (Olivia), 2016-09-22 19:53:51]

https://codereview.chromium.org/2359723003/diff/160001/content/test/data/gpu/...
File content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_worker.html
(right):

https://codereview.chromium.org/2359723003/diff/160001/content/test/data/gpu/...
content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_worker.html:23:
offscreen2d.commit();
On 2016/09/22 19:42:33, xidachen wrote:
> As discussed with kbr@, please add a self.postMessage(""), it should be safer
> than what it is now. At least not worse.

Done.

https://codereview.chromium.org/2359723003/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2359723003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp:128:
bool OffscreenCanvasRenderingContext2D::isAccelerated() const
On 2016/09/22 19:42:33, xidachen wrote:
> Is there a reason to keep this function here, since we can obtain it from the
> StaticBitmapImage?

Done.

[xidachen, 2016-09-22 20:20:34]

lgtm

[Justin Novosad, 2016-09-22 20:35:37]

lgtm 2

[Ken Russell (switch to Gerrit), 2016-09-22 22:47:28]

LGTM with caveat about the test.

https://codereview.chromium.org/2359723003/diff/180001/content/test/data/gpu/...
File content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_worker.html
(right):

https://codereview.chromium.org/2359723003/diff/180001/content/test/data/gpu/...
content/test/data/gpu/pixel_acceleratedOffscreen2d_commit_worker.html:24:
self.postMessage("");
I think that you should change this test so it waits for this message to be
received on the main thread before calling waitForFinish(). That will improve
the likelihood that the commit() will actually be processed by the rest of the
system before the harness grabs its screenshot, and reduce the possibility of
flakiness. (I gave Xida similar feedback on
https://codereview.chromium.org/2359263002/ .)

[xlai (Olivia), 2016-09-23 17:23:55]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 17:24:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xlai (Olivia), 2016-09-23 19:18:33]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2016-09-23 19:18:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
xidachen@chromium.org, junov@chromium.org, kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2359723003/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-23 19:19:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xlai (Olivia), 2016-09-23 19:27:17]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2016-09-23 19:27:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
xidachen@chromium.org, junov@chromium.org, kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2359723003/#ps260001
(title: "Fix rebase error")

[commit-bot: I haz the power, 2016-09-23 19:27:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 23:33:41]

Description was changed from

==========
Implement OffscreenCanvas Accelerated 2D commit()

This CL makes commit() on accelerated 2d context on OffscreenCanvas work. 

BUG=563852
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Implement OffscreenCanvas Accelerated 2D commit()

This CL makes commit() on accelerated 2d context on OffscreenCanvas work. 

BUG=563852
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-09-23 23:33:43]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2016-09-23 23:35:43]

Description was changed from

==========
Implement OffscreenCanvas Accelerated 2D commit()

This CL makes commit() on accelerated 2d context on OffscreenCanvas work. 

BUG=563852
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Implement OffscreenCanvas Accelerated 2D commit()

This CL makes commit() on accelerated 2d context on OffscreenCanvas work.

BUG=563852
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/971faed800d30243b9ff37e5dbdfe3d55209d4ad
Cr-Commit-Position: refs/heads/master@{#420785}
==========

[commit-bot: I haz the power, 2016-09-23 23:35:45]

Patchset 13 (id:??) landed as
https://crrev.com/971faed800d30243b9ff37e5dbdfe3d55209d4ad
Cr-Commit-Position: refs/heads/master@{#420785}