Fix missing finalizeFrame barriers in WebGL animations

Fix missing finalizeFrame barriers in WebGL animations

BUG=693100
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/2699713005
Cr-Commit-Position: refs/heads/master@{#452223}
Committed: https://chromium.googlesource.com/chromium/src/+/02b0b43a38dfc706a751f8983972e56ef6b81ccb

[Justin Novosad, 2017-02-16 17:13:07]

Description was changed from

==========
Fix missing finalizeFrame barriers in WebGL animations

BUG=693100
==========

to

==========
Fix missing finalizeFrame barriers in WebGL animations

BUG=693100
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, 2017-02-16 17:13:16]

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

[Justin Novosad, 2017-02-16 17:13:40]

junov@chromium.org changed reviewers:
+ kbr@chromium.org, xlai@google.com

[Justin Novosad, 2017-02-16 17:13:40]

PTAL

[commit-bot: I haz the power, 2017-02-16 17:14:03]

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-02-16 18:00:47]

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

[commit-bot: I haz the power, 2017-02-16 18:00:48]

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

[Justin Novosad, 2017-02-16 19:36:36]

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

[commit-bot: I haz the power, 2017-02-16 19:37:46]

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-02-16 20:40:35]

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

[commit-bot: I haz the power, 2017-02-16 20:40:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_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-02-18 00:08:01]

I don't understand the invalidation here but one part of this change seems odd.

To fix the underlying bug and since there's a test, lgtm, but the organization
seems confusing.

https://codereview.chromium.org/2699713005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp
(right):

https://codereview.chromium.org/2699713005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:169:
finalizeFrame();
Why is this being added here instead of inside HTMLCanvasElement::finalizeFrame?
It seems oddly placed.

https://codereview.chromium.org/2699713005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
(right):

https://codereview.chromium.org/2699713005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:137: virtual
void finalizeFrame() {}
Please document this new public method. Who should call it and when?

[Justin Novosad, 2017-02-21 18:03:29]

https://codereview.chromium.org/2699713005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
(right):

https://codereview.chromium.org/2699713005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:137: virtual
void finalizeFrame() {}
On 2017/02/18 00:08:01, Ken Russell wrote:
> Please document this new public method. Who should call it and when?

Done.

[Justin Novosad, 2017-02-21 18:06:21]

https://codereview.chromium.org/2699713005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp
(right):

https://codereview.chromium.org/2699713005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:169:
finalizeFrame();
On 2017/02/18 00:08:01, Ken Russell wrote:
> Why is this being added here instead of inside
HTMLCanvasElement::finalizeFrame?
> It seems oddly placed.

Because not all rendering contexts have an HTMLCanvasElement.  They may have an
OffscreenCanvas.

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

https://codereview.chromium.org/2699713005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1404:
m_animationFrameInProgress = true;
I renamed this variable. I think it is more readable now. Used to be 
m_paintInvalidatedForCurrentFrame

[Justin Novosad, 2017-02-21 18:07:05]

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

[commit-bot: I haz the power, 2017-02-21 18:07:43]

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

[Ken Russell (switch to Gerrit), 2017-02-21 19:23:10]

https://codereview.chromium.org/2699713005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp
(right):

https://codereview.chromium.org/2699713005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:169:
finalizeFrame();
On 2017/02/21 18:06:21, Justin Novosad wrote:
> On 2017/02/18 00:08:01, Ken Russell wrote:
> > Why is this being added here instead of inside
> HTMLCanvasElement::finalizeFrame?
> > It seems oddly placed.
> 
> Because not all rendering contexts have an HTMLCanvasElement.  They may have
an
> OffscreenCanvas.

Ah right. Thanks.

[xlai (Olivia), 2017-02-21 19:56:55]

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

[xlai (Olivia), 2017-02-21 19:56:56]

lgtm with nits.

https://codereview.chromium.org/2699713005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp
(right):

https://codereview.chromium.org/2699713005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:169:
finalizeFrame();
On 2017/02/21 18:06:21, Justin Novosad wrote:
> On 2017/02/18 00:08:01, Ken Russell wrote:
> > Why is this being added here instead of inside
> HTMLCanvasElement::finalizeFrame?
> > It seems oddly placed.
> 
> Because not all rendering contexts have an HTMLCanvasElement.  They may have
an
> OffscreenCanvas.

What would be the scenarios when finalizeFrame() is called for OffscreenCanvas?
Do you think it's ready to add an extra test for OffscreenCanvas in this CL or
there needs to be more refactoring
in the code (probably follow-up CLs) that handles those scenarios?

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

https://codereview.chromium.org/2699713005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1401:
m_markedCanvasDirty = true;
Just a bit curious: does this m_markedCanvasDirty have overlapping usage as with
m_animationFrameInProgress?

[commit-bot: I haz the power, 2017-02-21 20:02:20]

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

[commit-bot: I haz the power, 2017-02-21 20:02:21]

Dry run: This issue passed the CQ dry run.

[Justin Novosad, 2017-02-21 21:46:39]

On 2017/02/21 19:56:56, xlai (Olivia) wrote:
> lgtm with nits.
> 
>
https://codereview.chromium.org/2699713005/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp
> (right):
> 
>
https://codereview.chromium.org/2699713005/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:169:
> finalizeFrame();
> On 2017/02/21 18:06:21, Justin Novosad wrote:
> > On 2017/02/18 00:08:01, Ken Russell wrote:
> > > Why is this being added here instead of inside
> > HTMLCanvasElement::finalizeFrame?
> > > It seems oddly placed.
> > 
> > Because not all rendering contexts have an HTMLCanvasElement.  They may have
> an
> > OffscreenCanvas.
> 
> What would be the scenarios when finalizeFrame() is called for
OffscreenCanvas?
> Do you think it's ready to add an extra test for OffscreenCanvas in this CL or
> there needs to be more refactoring
> in the code (probably follow-up CLs) that handles those scenarios?

For now, not much, but we're starting to add performance heuristics that count
frames, like this one: https://codereview.chromium.org/2687073003/
> 
>
https://codereview.chromium.org/2699713005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
> (right):
> 
>
https://codereview.chromium.org/2699713005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1401:
> m_markedCanvasDirty = true;
> Just a bit curious: does this m_markedCanvasDirty have overlapping usage as
with
> m_animationFrameInProgress?

Not exactly. In patch set 1, I did not realize this which is what caused all the
test failures.  m_markedCanvasDirty is related to the snapshot that lives in the
canvas element (m_copiedImage), while m_animationFrameInProgress tracks commits.

[Justin Novosad, 2017-02-21 21:46:47]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2017-02-21 21:46:47]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2699713005/#ps40001
(title: "Comments + readability improvements")

[commit-bot: I haz the power, 2017-02-21 21:48:19]

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-02-21 23:53:11]

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

[commit-bot: I haz the power, 2017-02-21 23:53:13]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[Justin Novosad, 2017-02-22 20:03:19]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2017-02-22 20:03:48]

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-02-22 21:59:05]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487793799829510,
"parent_rev": "7b01d69adba07ea22bc68412c023f9d27fa397e0", "commit_rev":
"02b0b43a38dfc706a751f8983972e56ef6b81ccb"}

[commit-bot: I haz the power, 2017-02-22 21:59:49]

Description was changed from

==========
Fix missing finalizeFrame barriers in WebGL animations

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

==========
Fix missing finalizeFrame barriers in WebGL animations

BUG=693100
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/2699713005
Cr-Commit-Position: refs/heads/master@{#452223}
Committed:
https://chromium.googlesource.com/chromium/src/+/02b0b43a38dfc706a751f8983972...
==========

[commit-bot: I haz the power, 2017-02-22 21:59:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/02b0b43a38dfc706a751f8983972...