Implementing promise-based commit for driving OffscreenCanvas animations

Implementing promise-based commit for driving OffscreenCanvas animations

This is an experimental prototype API for demonstrating a proposal to use
promises instead of requestAnimationFrame for driving animations.  This
initial implementation uses a delayed task to simulate the BeginFrame
signal.  Properly hooking-up BeginFrame will be solved in a follow-up
code change.

This API was discussed here:
https://github.com/whatwg/html/issues/2139

BUG=676131
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

Review-Url: https://codereview.chromium.org/2594843002
Cr-Commit-Position: refs/heads/master@{#441711}
Committed: https://chromium.googlesource.com/chromium/src/+/d59fe166058d3a4c3f2d44ef5c76b0076315f16e

[Justin Novosad, 2016-12-20 22:30:15]

Description was changed from

==========
Implementing promise-based commit for driving OffscreenCanvas animations

This is an experimental prototype API for demonstrating a proposal to use
promises instead of requestAnimationFrame for driving animations.  This
initial implementation uses a delayed task to simulate the BeginFrame
signal.  Properly hooking-up BeginFrame will be solved in a follow-up
code change.

This API was discussed here:
https://github.com/whatwg/html/issues/2139

BUG=676131
==========

to

==========
Implementing promise-based commit for driving OffscreenCanvas animations

This is an experimental prototype API for demonstrating a proposal to use
promises instead of requestAnimationFrame for driving animations.  This
initial implementation uses a delayed task to simulate the BeginFrame
signal.  Properly hooking-up BeginFrame will be solved in a follow-up
code change.

This API was discussed here:
https://github.com/whatwg/html/issues/2139

BUG=676131
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
==========

[Justin Novosad, 2016-12-20 22:30:27]

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

[Justin Novosad, 2016-12-20 22:31:04]

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

[Justin Novosad, 2016-12-20 22:31:04]

PTAL

[commit-bot: I haz the power, 2016-12-20 22:31:11]

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

[Justin Novosad, 2016-12-20 22:32:38]

FYI: the gpu pixel test results should remain unchanged, which is why I did not
bump the version number

[commit-bot: I haz the power, 2016-12-21 00:19:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 00:19:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Justin Novosad, 2016-12-21 14:47:12]

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

[commit-bot: I haz the power, 2016-12-21 14:47:25]

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

[Stephen White, 2016-12-21 15:14:43]

LGTM w/nits

https://codereview.chromium.org/2594843002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc
(right):

https://codereview.chromium.org/2594843002/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:31: //
TODO(crbug.com/): Pass a valid display so that we can get OnBeginFrame
Nit: bug ID?

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

https://codereview.chromium.org/2594843002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:392:
if (framesInFlight < 3) {  // Backpressure guard
Make this be a compile-time constant?

[commit-bot: I haz the power, 2016-12-21 16:32:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 16:32:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Justin Novosad, 2016-12-21 18:16:12]

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

[commit-bot: I haz the power, 2016-12-21 18:16:35]

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

[Justin Novosad, 2016-12-21 18:27:53]

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

[commit-bot: I haz the power, 2016-12-21 18:28:33]

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-12-21 19:54:26]

https://codereview.chromium.org/2594843002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h (right):

https://codereview.chromium.org/2594843002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h:160: //
cc::FrameSinkId is broken into two integer components as this can be used
This comment for uint32_t m_clientId is broken into two parts.

https://codereview.chromium.org/2594843002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/2594843002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h:104:
void doCommit(RefPtr<StaticBitmapImage>);
I am confused that you add a function signature in header without adding its
implementation in cpp. Do I miss anything?

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

https://codereview.chromium.org/2594843002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher.h:17:
class OffscreenCanvasFrameDispatcherClient {
Just a suggestion: I see that the relationship of this new class to
OffscreenCanvasFrameDispatcher is similar to that
CanvasSurfaceLayerBridgeObserver to CanvasSurfaceLayerBridge. Do you think it's
better to rename "*Client" to "*Observer" for consistency or symmetry between
html canvas side and offscreencanvas side?

https://codereview.chromium.org/2594843002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right):

https://codereview.chromium.org/2594843002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:240:
m_overdrawFrame = std::move(image);
I'm wondering if you need to store the scriptState for the overdraw frame as
well. Will the scriptState ever change from frame to frame?

https://codereview.chromium.org/2594843002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:263:
m_overdrawFrameIsWebGLSoftwareRendering);
Hmmmm...After you doCommit on the overdrawFrame, do you need to set
m_overdrawFrame to be null? If not, when the next beginFrame() starts, will you
repeat doCommit on the same frame?

https://codereview.chromium.org/2594843002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:265:
m_commitPromiseResolver->resolve();
So you only resolve the promiseResolver when there is no more overdraw frame?
(as this is else if)

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

https://codereview.chromium.org/2594843002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:736:
exceptionState.throwDOMException(InvalidStateError,
Could you add a subtest to OffscreenCanvas-commit-invalid-call.html to validate
this exception?

[Justin Novosad, 2016-12-21 21:15:07]

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

[commit-bot: I haz the power, 2016-12-21 21:15:31]

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

[Justin Novosad, 2016-12-21 21:15:40]

https://codereview.chromium.org/2594843002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h (right):

https://codereview.chromium.org/2594843002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h:160: //
cc::FrameSinkId is broken into two integer components as this can be used
On 2016/12/21 19:54:25, xlai (Olivia) wrote:
> This comment for uint32_t m_clientId is broken into two parts.

Oop. Bad rebase.  Done.

https://codereview.chromium.org/2594843002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/2594843002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h:104:
void doCommit(RefPtr<StaticBitmapImage>);
On 2016/12/21 19:54:25, xlai (Olivia) wrote:
> I am confused that you add a function signature in header without adding its
> implementation in cpp. Do I miss anything?

Good catch. This is unused. It is a leftover from something I tried earlier.
Done.

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

https://codereview.chromium.org/2594843002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher.h:17:
class OffscreenCanvasFrameDispatcherClient {
On 2016/12/21 19:54:25, xlai (Olivia) wrote:
> Just a suggestion: I see that the relationship of this new class to
> OffscreenCanvasFrameDispatcher is similar to that
> CanvasSurfaceLayerBridgeObserver to CanvasSurfaceLayerBridge. Do you think
it's
> better to rename "*Client" to "*Observer" for consistency or symmetry between
> html canvas side and offscreencanvas side?

We use both in blink. It is really just a matter of semantics.  The term
"Observer" implies that the object is watching another other (subscribing to
change notifications). The term "Client" implies that the object is being served
something (event signals, for example) by another object.  The difference is
subtle and it is the same code pattern. 

IMHO, both classes are aptly named as is.

https://codereview.chromium.org/2594843002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right):

https://codereview.chromium.org/2594843002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:240:
m_overdrawFrame = std::move(image);
On 2016/12/21 19:54:25, xlai (Olivia) wrote:
> I'm wondering if you need to store the scriptState for the overdraw frame as
> well. Will the scriptState ever change from frame to frame?

The overdraw frame does not generate a promise (the commit call re-returns the
previous promise), so we have no use for the scriptState.

https://codereview.chromium.org/2594843002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:263:
m_overdrawFrameIsWebGLSoftwareRendering);
On 2016/12/21 19:54:25, xlai (Olivia) wrote:
> Hmmmm...After you doCommit on the overdrawFrame, do you need to set
> m_overdrawFrame to be null? If not, when the next beginFrame() starts, will
you
> repeat doCommit on the same frame?

std::move already takes care of that.

https://codereview.chromium.org/2594843002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:265:
m_commitPromiseResolver->resolve();
On 2016/12/21 19:54:25, xlai (Olivia) wrote:
> So you only resolve the promiseResolver when there is no more overdraw frame?
> (as this is else if)

Exactly. As long as there is a backlog, we do not need to be triggering new
frames.

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

https://codereview.chromium.org/2594843002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:736:
exceptionState.throwDOMException(InvalidStateError,
On 2016/12/21 19:54:25, xlai (Olivia) wrote:
> Could you add a subtest to OffscreenCanvas-commit-invalid-call.html to
validate
> this exception?

That is actually really hard to do because it requires faking  some kind of GPU
set-up failure.  This case is really weird because we are forced to return a
promise, and we can't just return a promise that will never be resolved. I think
I was doing the wrong thing here.  I am changing the code to do a silent commit
instead.

[commit-bot: I haz the power, 2016-12-21 23:16:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 23:16:52]

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_TIMED_OUT, no
build URL)

[xlai (Olivia), 2016-12-22 02:28:05]

lgtm

[Justin Novosad, 2016-12-22 14:37:13]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2016-12-22 14:37:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
senorblanco@chromium.org
Link to the patchset: https://codereview.chromium.org/2594843002/#ps80001
(title: "applied xlai feedback")

[commit-bot: I haz the power, 2016-12-22 14:37:27]

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-12-22 14:43:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 14:44:00]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Justin Novosad, 2016-12-22 15:04:49]

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

[Justin Novosad, 2016-12-22 15:04:50]

+kbr for webgl owners

[Ken Russell (switch to Gerrit), 2017-01-04 02:41:35]

Sorry for the long delay reviewing. In the future please feel free to ask zmo@
for review as well, and/or let's discuss adding you as a co-OWNER.

LGTM. Would like to re-evaluate this after more performance and usability tests
are done, before this comes out from under the ExperimentalCanvasFeatures flag.

[Justin Novosad, 2017-01-04 20:22:52]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2017-01-04 20:23:36]

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

[Justin Novosad, 2017-01-04 20:24:39]

On 2017/01/04 02:41:35, Ken Russell wrote:
> LGTM. Would like to re-evaluate this after more performance and usability
tests
> are done, before this comes out from under the ExperimentalCanvasFeatures
flag.

For sure.  This implementation is just for the purpose of demonstration and
experimentation.  The final form of the API has not been decided.

[commit-bot: I haz the power, 2017-01-04 23:40:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 23:40:35]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Russell (switch to Gerrit), 2017-01-05 02:30:41]

On 2017/01/04 23:40:35, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Pushed on and/or filed http://crbug.com/663858, http://crbug.com/678454, and
http://crbug.com/678093 about these flakes.

[Ken Russell (switch to Gerrit), 2017-01-05 02:30:51]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2017-01-05 02:31:22]

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, 2017-01-05 02:40:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 02:40:55]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Russell (switch to Gerrit), 2017-01-05 02:42:28]

On 2017/01/05 02:40:55, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
>   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Unfortunately, now this needs a rebase.

[Justin Novosad, 2017-01-05 16:36:57]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2017-01-05 16:36:57]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
xlai@chromium.org, senorblanco@chromium.org
Link to the patchset: https://codereview.chromium.org/2594843002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-05 16:37:20]

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, 2017-01-05 18:42:25]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483634217082760,
"parent_rev": "0eac439a92b063357b9e6b43aa360b8844ecc5f3", "commit_rev":
"d59fe166058d3a4c3f2d44ef5c76b0076315f16e"}

[commit-bot: I haz the power, 2017-01-05 18:46:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483634217082760,
"parent_rev": "0eac439a92b063357b9e6b43aa360b8844ecc5f3", "commit_rev":
"d59fe166058d3a4c3f2d44ef5c76b0076315f16e"}

[commit-bot: I haz the power, 2017-01-05 18:46:05]

Description was changed from

==========
Implementing promise-based commit for driving OffscreenCanvas animations

This is an experimental prototype API for demonstrating a proposal to use
promises instead of requestAnimationFrame for driving animations.  This
initial implementation uses a delayed task to simulate the BeginFrame
signal.  Properly hooking-up BeginFrame will be solved in a follow-up
code change.

This API was discussed here:
https://github.com/whatwg/html/issues/2139

BUG=676131
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
==========

to

==========
Implementing promise-based commit for driving OffscreenCanvas animations

This is an experimental prototype API for demonstrating a proposal to use
promises instead of requestAnimationFrame for driving animations.  This
initial implementation uses a delayed task to simulate the BeginFrame
signal.  Properly hooking-up BeginFrame will be solved in a follow-up
code change.

This API was discussed here:
https://github.com/whatwg/html/issues/2139

BUG=676131
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

Review-Url: https://codereview.chromium.org/2594843002
Cr-Commit-Position: refs/heads/master@{#441711}
Committed:
https://chromium.googlesource.com/chromium/src/+/d59fe166058d3a4c3f2d44ef5c76...
==========

[commit-bot: I haz the power, 2017-01-05 18:46:10]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d59fe166058d3a4c3f2d44ef5c76...