Synchronize commits at end of JS task

Synchronize commits at end of JS task

This CL defers the submission of frames to the end of a JS task, so that the
commits() for multiple OffscreenCanvases in the same execution context are
happening at the same time.

When a commit() comes in, there are three possible scenarios:
1. This is first commit since last resolved promise. In this case, we store the
   frame at m_currentFrame and register the context to task observer.
2. This is not the first commit for the same canvas in the same JS task. In
   this case, we overwrite the m_currentFrame but we do not duplicate
   registration to task observer.
3. This is the overdraw commit (the m_currentFrame has been dispatched but we
   are still waiting for the next OnBeginFrame to resolve the promise). In this
   case, we set the m_overdraw commit.

When JS task finishes, finalizeFrame() will get fired and submit the
current frame.

When OnBeginFrame is fired, we'll resolve the promise if there is no current
frame and no overdraw frame. If there is an overdraw frame, we doCommit for
the overdraw frame and wait for next animation frame to resolve the promise.

I've also modified the GPU pixel tests to test for all these situations.

BUG=697582
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

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

[xlai (Olivia), 2017-03-24 17:28:32]

Description was changed from

==========
Synchronize commits at end of JS task

This CL defers the submission of frames to the end of a JS task, so that the
commits() for multiple OffscreenCanvases in the same execution context are
happening at the same time.

When a commit() comes in, there are three possible scenarios:
1. This is first commit since last resolved promise. In this case, we store the
   frame at m_currentFrame and register the context to task observer.
2. This is not the first commit for the same canvas in the same JS task. In
   this case, we overwrite the m_currentFrame but we do not duplicate
   registration to task observer.
3. This is the overdraw commit (the m_currentFrame has been dispatched but we
   are still waiting for the next OnBeginFrame to resolve the promise). In this
   case, we set the m_overdraw commit.

When JS task finishes, doCommitAtEndOfTask() will get fired and submit the
current frame.

When OnBeginFrame is fired, we'll resolve the promise if there is no current
frame and no overdraw frame. If there is an overdraw frame, we doCommit for
the overdraw frame and wait for next animation frame to resolve the promise.

I've also modified the GPU pixel tests to test for all these situations.

BUG=697582
==========

to

==========
Synchronize commits at end of JS task

This CL defers the submission of frames to the end of a JS task, so that the
commits() for multiple OffscreenCanvases in the same execution context are
happening at the same time.

When a commit() comes in, there are three possible scenarios:
1. This is first commit since last resolved promise. In this case, we store the
   frame at m_currentFrame and register the context to task observer.
2. This is not the first commit for the same canvas in the same JS task. In
   this case, we overwrite the m_currentFrame but we do not duplicate
   registration to task observer.
3. This is the overdraw commit (the m_currentFrame has been dispatched but we
   are still waiting for the next OnBeginFrame to resolve the promise). In this
   case, we set the m_overdraw commit.

When JS task finishes, doCommitAtEndOfTask() will get fired and submit the
current frame.

When OnBeginFrame is fired, we'll resolve the promise if there is no current
frame and no overdraw frame. If there is an overdraw frame, we doCommit for
the overdraw frame and wait for next animation frame to resolve the promise.

I've also modified the GPU pixel tests to test for all these situations.

BUG=697582
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[xlai (Olivia), 2017-03-24 17:31:11]

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

[Justin Novosad, 2017-03-24 17:41:54]

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

https://codereview.chromium.org/2778483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:246:
m_overdrawFrame = nullptr;
We don't to track m_overdrawFrame anymore.

https://codereview.chromium.org/2778483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:270: void
OffscreenCanvas::doCommitAtEndOfTask() {
What happens if we reach this point and m_commitPromiseResolver is still
pending?  IMHO, we need to treat m_currentFrame as an overdraw frame in that
case, and postpone the commit until beginFrame().

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

https://codereview.chromium.org/2778483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h:163:
RefPtr<StaticBitmapImage> m_currentFrame;
Could we call this m_frameToDispatchAtEndOfTask

[xlai (Olivia), 2017-03-24 18:18:52]

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

https://codereview.chromium.org/2778483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:270: void
OffscreenCanvas::doCommitAtEndOfTask() {
On 2017/03/24 17:41:53, Justin Novosad wrote:
> What happens if we reach this point and m_commitPromiseResolver is still
> pending?  IMHO, we need to treat m_currentFrame as an overdraw frame in that
> case, and postpone the commit until beginFrame().

Actually, this situation was already handled in the original code. But I think I
used more than enough variables to hold the frames; m_currentFrame can be
reused in the overdraw situation. So I remove the
m_overdrawFrame now.

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

https://codereview.chromium.org/2778483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h:163:
RefPtr<StaticBitmapImage> m_currentFrame;
On 2017/03/24 17:41:53, Justin Novosad wrote:
> Could we call this m_frameToDispatchAtEndOfTask

I don't think this name is suitable. With the latest change,
m_currentFrame can be dispatched in two different situations:
1. at the end of task, in normal situation
2. onBeginFrame, when this particular frame is an overdraw frame that user
commit
when the previous m_currentFrame has been dispatched but
the promise is not resolved yet.

[Justin Novosad, 2017-03-24 18:40:31]

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

https://codereview.chromium.org/2778483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:250:
Platform::current()->currentThread()->addTaskObserver(m_context);
I am concerned that the registration and de-registration of the task observer
can get tangled with CanvasRenderingContext's own mechanism.  Would be cleaner
if you created a new method CanvasRenderingContext::needsFinalizeFrame(), whic
you would call from here and from CanvasRenderingContext::didDraw

https://codereview.chromium.org/2778483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:265: void
OffscreenCanvas::doCommitAtEndOfTask() {
I'd call this finalizeFrame

https://codereview.chromium.org/2778483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:266:
doCommit(std::move(m_currentFrame), m_currentFrameIsWebGLSoftwareRendering);
Should put this inside "if (m_currentFrame)", just in case finalizeFrame was
scheduled for some other reason than commit.

[xlai (Olivia), 2017-03-24 19:50:17]

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

https://codereview.chromium.org/2778483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:250:
Platform::current()->currentThread()->addTaskObserver(m_context);
On 2017/03/24 18:40:31, Justin Novosad wrote:
> I am concerned that the registration and de-registration of the task observer
> can get tangled with CanvasRenderingContext's own mechanism.  Would be cleaner
> if you created a new method CanvasRenderingContext::needsFinalizeFrame(), whic
> you would call from here and from CanvasRenderingContext::didDraw

Done.

https://codereview.chromium.org/2778483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:265: void
OffscreenCanvas::doCommitAtEndOfTask() {
On 2017/03/24 18:40:31, Justin Novosad wrote:
> I'd call this finalizeFrame

Done.

https://codereview.chromium.org/2778483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:266:
doCommit(std::move(m_currentFrame), m_currentFrameIsWebGLSoftwareRendering);
On 2017/03/24 18:40:31, Justin Novosad wrote:
> Should put this inside "if (m_currentFrame)", just in case finalizeFrame was
> scheduled for some other reason than commit.

Done.

[Justin Novosad, 2017-03-24 20:40:59]

lgtm.

This is good enough to ship IMHO OffscreenCanvas, but it is still not
technically compliant.  Please file a follow-up bug for making multiple canvas
updates truly atomic.

[xlai (Olivia), 2017-03-24 21:09:09]

The CQ bit was checked by xlai@chromium.org

[commit-bot: I haz the power, 2017-03-24 21:09:37]

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-03-24 21:12:45]

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

[commit-bot: I haz the power, 2017-03-24 21:12:46]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[xlai (Olivia), 2017-03-24 21:27:38]

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

[xlai (Olivia), 2017-03-24 21:27:38]

+zmo@ to review the content/test/gpu/gpu_tests.

This is just an update of new reference images of the existing gpu pixel tests
for OffscreenCanvas.

[xlai (Olivia), 2017-03-24 21:41:03]

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

[commit-bot: I haz the power, 2017-03-24 21:41:23]

Dry run: 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-03-24 23:18:55]

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

[commit-bot: I haz the power, 2017-03-24 23:18:56]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[xlai (Olivia), 2017-03-27 14:40:13]

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

[commit-bot: I haz the power, 2017-03-27 14:40:38]

Dry run: 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-03-27 16:35:10]

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

[commit-bot: I haz the power, 2017-03-27 16:35:11]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[Zhenyao Mo, 2017-03-27 17:55:22]

On 2017/03/24 21:27:38, xlai (Olivia) wrote:
> +zmo@ to review the content/test/gpu/gpu_tests.
> 
> This is just an update of new reference images of the existing gpu pixel tests
> for OffscreenCanvas.

lgtm

[xlai (Olivia), 2017-03-27 17:57:45]

Description was changed from

==========
Synchronize commits at end of JS task

This CL defers the submission of frames to the end of a JS task, so that the
commits() for multiple OffscreenCanvases in the same execution context are
happening at the same time.

When a commit() comes in, there are three possible scenarios:
1. This is first commit since last resolved promise. In this case, we store the
   frame at m_currentFrame and register the context to task observer.
2. This is not the first commit for the same canvas in the same JS task. In
   this case, we overwrite the m_currentFrame but we do not duplicate
   registration to task observer.
3. This is the overdraw commit (the m_currentFrame has been dispatched but we
   are still waiting for the next OnBeginFrame to resolve the promise). In this
   case, we set the m_overdraw commit.

When JS task finishes, doCommitAtEndOfTask() will get fired and submit the
current frame.

When OnBeginFrame is fired, we'll resolve the promise if there is no current
frame and no overdraw frame. If there is an overdraw frame, we doCommit for
the overdraw frame and wait for next animation frame to resolve the promise.

I've also modified the GPU pixel tests to test for all these situations.

BUG=697582
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Synchronize commits at end of JS task

This CL defers the submission of frames to the end of a JS task, so that the
commits() for multiple OffscreenCanvases in the same execution context are
happening at the same time.

When a commit() comes in, there are three possible scenarios:
1. This is first commit since last resolved promise. In this case, we store the
   frame at m_currentFrame and register the context to task observer.
2. This is not the first commit for the same canvas in the same JS task. In
   this case, we overwrite the m_currentFrame but we do not duplicate
   registration to task observer.
3. This is the overdraw commit (the m_currentFrame has been dispatched but we
   are still waiting for the next OnBeginFrame to resolve the promise). In this
   case, we set the m_overdraw commit.

When JS task finishes, finalizeFrame() will get fired and submit the
current frame.

When OnBeginFrame is fired, we'll resolve the promise if there is no current
frame and no overdraw frame. If there is an overdraw frame, we doCommit for
the overdraw frame and wait for next animation frame to resolve the promise.

I've also modified the GPU pixel tests to test for all these situations.

BUG=697582
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[xlai (Olivia), 2017-03-27 17:57:59]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2017-03-27 17:57:59]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/2778483002/#ps120001
(title: "fix layout test")

[commit-bot: I haz the power, 2017-03-27 17:58:41]

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-03-27 18:12:50]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490637479317760,
"parent_rev": "bedba279ba5dc64257efc7bc8a13d697c049d3de", "commit_rev":
"a2095f6c1946afe6c434cf05eb4d419dd9b74275"}

[commit-bot: I haz the power, 2017-03-27 18:13:33]

Description was changed from

==========
Synchronize commits at end of JS task

This CL defers the submission of frames to the end of a JS task, so that the
commits() for multiple OffscreenCanvases in the same execution context are
happening at the same time.

When a commit() comes in, there are three possible scenarios:
1. This is first commit since last resolved promise. In this case, we store the
   frame at m_currentFrame and register the context to task observer.
2. This is not the first commit for the same canvas in the same JS task. In
   this case, we overwrite the m_currentFrame but we do not duplicate
   registration to task observer.
3. This is the overdraw commit (the m_currentFrame has been dispatched but we
   are still waiting for the next OnBeginFrame to resolve the promise). In this
   case, we set the m_overdraw commit.

When JS task finishes, finalizeFrame() will get fired and submit the
current frame.

When OnBeginFrame is fired, we'll resolve the promise if there is no current
frame and no overdraw frame. If there is an overdraw frame, we doCommit for
the overdraw frame and wait for next animation frame to resolve the promise.

I've also modified the GPU pixel tests to test for all these situations.

BUG=697582
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Synchronize commits at end of JS task

This CL defers the submission of frames to the end of a JS task, so that the
commits() for multiple OffscreenCanvases in the same execution context are
happening at the same time.

When a commit() comes in, there are three possible scenarios:
1. This is first commit since last resolved promise. In this case, we store the
   frame at m_currentFrame and register the context to task observer.
2. This is not the first commit for the same canvas in the same JS task. In
   this case, we overwrite the m_currentFrame but we do not duplicate
   registration to task observer.
3. This is the overdraw commit (the m_currentFrame has been dispatched but we
   are still waiting for the next OnBeginFrame to resolve the promise). In this
   case, we set the m_overdraw commit.

When JS task finishes, finalizeFrame() will get fired and submit the
current frame.

When OnBeginFrame is fired, we'll resolve the promise if there is no current
frame and no overdraw frame. If there is an overdraw frame, we doCommit for
the overdraw frame and wait for next animation frame to resolve the promise.

I've also modified the GPU pixel tests to test for all these situations.

BUG=697582
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

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

[commit-bot: I haz the power, 2017-03-27 18:13:35]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/a2095f6c1946afe6c434cf05eb4d...