Add Intel macOS workaround for WebGL2 canvas_sub_rectangle tests

Add Intel macOS workaround for WebGL2 canvas_sub_rectangle tests

Reading pixels back from a texture based 2D canvas is flaky with Mac
Intel driver. This workarounds it by doing a redundant texture copy
before reading. The issue happens using TexImage2D and TexSubImage2D in
turn to source from a GPU-based 2D canvas. Currently TexImage2D may be
hardware-accelerated via GPU texture copying, whereas TexSubImage2D
has to do software read-back as blink has no knowledge of destination
texture's internal format. The cases can pass if merely testing either,
but fail testing both with the software and hardware copy mixed.

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

[jchen10, 2016-12-02 13:12:53]

Description was changed from

==========
Add Intel macOS workaround for WebGL2 canvas_sub_rectangle tests

BUG=665656
==========

to

==========
Add Intel macOS workaround for WebGL2 canvas_sub_rectangle tests

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

[jchen10, 2016-12-02 13:13:59]

The CQ bit was checked by jie.a.chen@intel.com

[commit-bot: I haz the power, 2016-12-02 13:14:18]

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-02 13:14:18]

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

[commit-bot: I haz the power, 2016-12-02 13:14:19]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[jchen10, 2016-12-02 13:20:52]

The CQ bit was unchecked by jie.a.chen@intel.com

[jchen10, 2016-12-02 13:55:43]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 13:55:55]

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, 2016-12-02 14:02:08]

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

[commit-bot: I haz the power, 2016-12-02 14:02:09]

Dry run: 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...)

[jchen10, 2016-12-04 10:45:50]

Description was changed from

==========
Add Intel macOS workaround for WebGL2 canvas_sub_rectangle tests

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

to

==========
Add Intel macOS workaround for WebGL2 canvas_sub_rectangle tests

Reading pixels back from a texture based 2D canvas is flaky with Mac
Intel driver. This workarounds it by doing a redundant texture copy
before reading.

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

[jchen10, 2016-12-04 10:46:02]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-04 10:46:06]

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, 2016-12-04 11:08:40]

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

[commit-bot: I haz the power, 2016-12-04 11:08:41]

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[jchen10, 2016-12-04 12:09:14]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-04 12:09:21]

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, 2016-12-04 12:27:51]

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

[commit-bot: I haz the power, 2016-12-04 12:27:52]

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[jchen10, 2016-12-04 12:36:19]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-04 12:36:26]

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, 2016-12-04 12:57:56]

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

[commit-bot: I haz the power, 2016-12-04 12:57:56]

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[jchen10, 2016-12-04 13:38:58]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-04 13:39:05]

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, 2016-12-04 13:58:55]

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

[commit-bot: I haz the power, 2016-12-04 13:58:55]

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[jchen10, 2016-12-04 15:29:06]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-04 15:29:19]

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, 2016-12-04 15:55:43]

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

[commit-bot: I haz the power, 2016-12-04 15:55:44]

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[jchen10, 2016-12-05 00:25:35]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 00:25:49]

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, 2016-12-05 01:04:38]

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

[commit-bot: I haz the power, 2016-12-05 01:04:39]

Dry run: This issue passed the CQ dry run.

[jchen10, 2016-12-05 01:07:45]

jie.a.chen@intel.com changed reviewers:
+ qiankun.miao@intel.com, yang.gu@intel.com, yunchao.he@intel.com

[jchen10, 2016-12-05 01:09:29]

Please have a review first!

[qiankun, 2016-12-05 02:25:12]

qiankun.miao@intel.com changed reviewers:
+ bsalomon@google.com, ericrk@chromium.org, kbr@chromium.org, zmo@chromium.org

[jchen10, 2016-12-05 04:29:47]

Description was changed from

==========
Add Intel macOS workaround for WebGL2 canvas_sub_rectangle tests

Reading pixels back from a texture based 2D canvas is flaky with Mac
Intel driver. This workarounds it by doing a redundant texture copy
before reading.

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

to

==========
Add Intel macOS workaround for WebGL2 canvas_sub_rectangle tests

Reading pixels back from a texture based 2D canvas is flaky with Mac
Intel driver. This workarounds it by doing a redundant texture copy
before reading. The issue happens using TexImage2D and TexSubImage2D in
turn to source from a GPU-based 2D canvas. Currently TexImage2D may be
hardware-accelerated via GPU texture copying, whereas TexSubImage2D
has to do software read-back as blink has no knowledge of destination
texture's internal format. The cases can pass if merely testing either,
but fail testing both with the software and hardware copy mixed.

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

[jchen10, 2016-12-05 04:29:47]

jie.a.chen@intel.com changed reviewers:
+ kainino@chromium.org

[jchen10, 2016-12-05 04:30:00]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 04:30:14]

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, 2016-12-05 06:14:01]

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

[commit-bot: I haz the power, 2016-12-05 06:14:02]

Dry run: This issue passed the CQ dry run.

[qiankun, 2016-12-05 08:25:31]

Looks good to me.

https://codereview.chromium.org/2547013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2547013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:1124: //
snapshot, on Intel MacOS platform(crbug.com/665656).
Brian & Eric, we found this workaround but we didn't root cause why result of
read back from canvas is wrong. Do you have any ideas?

[bsalomon, 2016-12-05 14:12:41]

https://codereview.chromium.org/2547013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2547013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:1124: //
snapshot, on Intel MacOS platform(crbug.com/665656).
On 2016/12/05 08:25:31, qiankun wrote:
> Brian & Eric, we found this workaround but we didn't root cause why result of
> read back from canvas is wrong. Do you have any ideas?

Sorry, but I don't think we've encountered this particular quirk in Skia before.

[Zhenyao Mo, 2016-12-05 18:13:35]

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

[Zhenyao Mo, 2016-12-05 18:13:36]

+junov for insights

https://codereview.chromium.org/2547013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2547013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:1126:
m_surface->makeSurface(m_surface->getCanvas()->imageInfo()).reset();
This seems a big perf regression.  If we only encounter this bug when uploading
2d canvas to WebGL, should we only trigger this on-demand when we use this 2d
canvas as texture source in WebGL? (I haven't looked into how feasible it is
though, but doing it all the time on Intel Mac seems an overkill).

[Kai Ninomiya, 2016-12-05 18:41:32]

On 2016/12/05 18:13:36, Zhenyao Mo wrote:
> +junov for insights
> 
>
https://codereview.chromium.org/2547013002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
> (right):
> 
>
https://codereview.chromium.org/2547013002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:1126:
> m_surface->makeSurface(m_surface->getCanvas()->imageInfo()).reset();
> This seems a big perf regression.  If we only encounter this bug when
uploading
> 2d canvas to WebGL, should we only trigger this on-demand when we use this 2d
> canvas as texture source in WebGL? (I haven't looked into how feasible it is
> though, but doing it all the time on Intel Mac seems an overkill).

I agree that this looks like a big perf regression and it would be best to
narrow it down as much as possible (or fix it some other way).

jchen, I'm still investigating so I can try to file a radar (if it turns out to
be necessary) - sorry for not sending an update before the weekend. I just sent
a partial update to the bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=665656
But I'll continue to work on it today.

[Justin Novosad, 2016-12-05 20:54:02]

https://codereview.chromium.org/2547013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2547013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:1125: if
(hint == PreferAccelerationWithExtraSurfaceCopy)
This is a misuse of "hint". As it's name suggests, it is just a hint. The
purpose of the hint parameter is to indicate an opportunistic preference that
may or may not be ignored.  Using it for other purposes is confusing and bad for
readability.  Proof of confusingness: this code will be making an unnecessary
copy in cases where the canvas is not gpu accelerated (because the hint does not
force the canvas to be accelerated).  I think it would be cleaner to use the
SnapshotReason instead.  I have an idea how to do this cleanly.  Let me land a
small patch first.

[Ken Russell (switch to Gerrit), 2016-12-06 00:01:55]

Note also from talking with that the reason we are going down the readback path
in the first place is that CopySubTextureCHROMIUM doesn't yet have the necessary
validation to reject illegal calls when the internal format differs from the
texture's allocated internal format:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webgl/...
.

I agree with zmo's concern that this is a potentially very expensive workaround.
I think that we should do some more work to identify what might be going wrong
in the driver so that we can create a reduced test case, and possibly a more
targeted workaround.

[Kai Ninomiya, 2016-12-06 06:57:06]

On 2016/12/06 00:01:55, Ken Russell wrote:
> Note also from talking with that the reason we are going down the readback
path
> in the first place is that CopySubTextureCHROMIUM doesn't yet have the
necessary
> validation to reject illegal calls when the internal format differs from the
> texture's allocated internal format:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webgl/...
> .
> 
> I agree with zmo's concern that this is a potentially very expensive
workaround.
> I think that we should do some more work to identify what might be going wrong
> in the driver so that we can create a reduced test case, and possibly a more
> targeted workaround.

See https://crbug.com/665656#c22 for update on my investigation