Make gl_clear_broken workaround support core profile and use it under AMD Linux Catalyst driver

Make gl_clear_broken workaround support core profile and use it under AMD Linux Catalyst driver

Make ClearFrameBuffer use a VAO and add version to shaders in core
profile. Use gl_clear_broken workaround under Linux AMD Catalyst driver
because it ignores clear if it's the only thing rendered to the target
before the target is read.

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

Review-Url: https://codereview.chromium.org/2764833003
Cr-Commit-Position: refs/heads/master@{#460282}
Committed: https://chromium.googlesource.com/chromium/src/+/6ca5d0d3ad671604d9a028e71402e9e5ee25f110

[xiangze.zhang, 2017-03-21 11:33:23]

Description was changed from

==========
Make gl_clear_broken workaround support core profile and use it under AMD Linux
Catalyst driver

Make ClearFrameBuffer use a VAO and add version to shaders in core
profile. Use gl_clear_broken workaround under Linux AMD Catalyst driver
because it ignores clear if it's the only thing rendered to the target
before the target is read.

BUG=690122
==========

to

==========
Make gl_clear_broken workaround support core profile and use it under AMD Linux
Catalyst driver

Make ClearFrameBuffer use a VAO and add version to shaders in core
profile. Use gl_clear_broken workaround under Linux AMD Catalyst driver
because it ignores clear if it's the only thing rendered to the target
before the target is read.

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

[xiangze.zhang, 2017-03-21 12:00:33]

xiangze.zhang@intel.com changed reviewers:
+ kbr@chromium.org, zmo@chromium.org

[xiangze.zhang, 2017-03-21 12:00:33]

PTAL. This patch is based on https://codereview.chromium.org/2080943002 and
tries to avoid the failures.

[Ken Russell (switch to Gerrit), 2017-03-23 04:51:04]

Thank you Xiangze for fixing this. The change looks good to me overall. Probably
something we need to fold in to ANGLE, and consider changing Skia in case it
moves into the GPU process.

Let me trigger a CQ dry run for this change. Ideally zmo@ would look at it too,
but I'll give it a thumbs up if the tests come back clean.

[Ken Russell (switch to Gerrit), 2017-03-23 04:51:07]

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

[commit-bot: I haz the power, 2017-03-23 04:51:16]

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-23 04:54:02]

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

[commit-bot: I haz the power, 2017-03-23 04:54:04]

Dry run: Try jobs failed on following builders:
  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 on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Ken Russell (switch to Gerrit), 2017-03-23 05:07:43]

On 2017/03/23 04:51:04, Ken Russell wrote:
> Thank you Xiangze for fixing this. The change looks good to me overall.
Probably
> something we need to fold in to ANGLE, and consider changing Skia in case it
> moves into the GPU process.
> 
> Let me trigger a CQ dry run for this change. Ideally zmo@ would look at it
too,
> but I'll give it a thumbs up if the tests come back clean.

Xiangze, could you please rebase this CL?

[xiangze.zhang, 2017-03-23 05:30:13]

On 2017/03/23 05:07:43, Ken Russell wrote:
> On 2017/03/23 04:51:04, Ken Russell wrote:
> > Thank you Xiangze for fixing this. The change looks good to me overall.
> Probably
> > something we need to fold in to ANGLE, and consider changing Skia in case it
> > moves into the GPU process.
> > 
> > Let me trigger a CQ dry run for this change. Ideally zmo@ would look at it
> too,
> > but I'll give it a thumbs up if the tests come back clean.
> 
> Xiangze, could you please rebase this CL?

CL Rebased.

[Zhenyao Mo, 2017-03-23 17:11:40]

Great work.

LGTM

https://codereview.chromium.org/2764833003/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc (right):

https://codereview.chromium.org/2764833003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc:74: vao_(0),
nit: 0u to be consistent with above and below

[Zhenyao Mo, 2017-03-23 17:11:52]

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

[commit-bot: I haz the power, 2017-03-23 17:12: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-23 18:05:38]

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

[commit-bot: I haz the power, 2017-03-23 18:05:50]

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

[xiangze.zhang, 2017-03-24 10:48:12]

The CQ bit was checked by xiangze.zhang@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 10:48:33]

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 10:58:32]

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

[commit-bot: I haz the power, 2017-03-24 10:58:34]

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

[xiangze.zhang, 2017-03-27 03:04:48]

The CQ bit was checked by xiangze.zhang@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 03:05:00]

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 03:48:30]

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

[commit-bot: I haz the power, 2017-03-27 03:48:31]

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

[xiangze.zhang, 2017-03-27 04:54:44]

The CQ bit was checked by xiangze.zhang@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 04:54:56]

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 05:44:22]

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

[commit-bot: I haz the power, 2017-03-27 05:44:23]

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_...)

[xiangze.zhang, 2017-03-27 06:29:24]

The CQ bit was checked by xiangze.zhang@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 06:29:42]

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 07:39:52]

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

[commit-bot: I haz the power, 2017-03-27 07:39:53]

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

[xiangze.zhang, 2017-03-27 08:45:54]

Changed the code to use ShaderTranslator instead of manually modifying the
shaders.

Two tests are failing now but may not related to this CL. One is flaky and one
is already failing for a few days.

[Zhenyao Mo, 2017-03-27 17:38:01]

On 2017/03/27 08:45:54, xiangze.zhang wrote:
> Changed the code to use ShaderTranslator instead of manually modifying the
> shaders.
> 
> Two tests are failing now but may not related to this CL. One is flaky and one
> is already failing for a few days.

not lgtm

The mac_optional_gpu_tests_rel failures are very real.

Since you did a major refactoring, let me take out my original approval.  Once
the bots are green, I am happy to re-review.

[Ken Russell (switch to Gerrit), 2017-03-27 20:14:53]

The mac_optional_gpu_tests_rel failures are real, but they're
http://crbug.com/705156 .

We still mustn't land this until that bot's green again to avoid potentially
regressing things further.

[xiangze.zhang, 2017-03-28 01:08:53]

The CQ bit was checked by xiangze.zhang@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 01:09:34]

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-28 01:09:35]

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

[commit-bot: I haz the power, 2017-03-28 01:09:36]

Dry run: A disapproval has been posted by following reviewers: zmo@chromium.org.
Please make sure to get positive review before another CQ attempt.

[xiangze.zhang, 2017-03-28 02:32:30]

I see the mac_optional_gpu_tests_rel issue is resolved and some flaky tests are
disabled. But I cannot trigger dry run now. Could you run tests for this patch
again?

[Ken Russell (switch to Gerrit), 2017-03-28 03:06:35]

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

[commit-bot: I haz the power, 2017-03-28 03:07:02]

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-03-28 03:07:15]

On 2017/03/28 02:32:30, xiangze.zhang wrote:
> I see the mac_optional_gpu_tests_rel issue is resolved and some flaky tests
are
> disabled. But I cannot trigger dry run now. Could you run tests for this patch
> again?

Yes; done. Hopefully there were no conflicts introduced which would prevent the
patch from applying.

[commit-bot: I haz the power, 2017-03-28 05:50:04]

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

[commit-bot: I haz the power, 2017-03-28 05:50:05]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2017-03-28 20:31:04]

Mo: the mac_optional_gpu_tests_rel failures are resolved. Can you re-review?
Thanks.

[Zhenyao Mo, 2017-03-28 20:50:48]

lgtm

Thanks for improving this CL

https://codereview.chromium.org/2764833003/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc (right):

https://codereview.chromium.org/2764833003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc:9: #include
"base/containers/hash_tables.h"
nit: this is already included in the .h file.

https://codereview.chromium.org/2764833003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc:184:
glGetUniformLocation(program_, name_map_["u_clear_color"].c_str());
nit: can you DCHECK that depth_handle_ != -1 and color_handle_ != -1?

[xiangze.zhang, 2017-03-29 01:00:24]

The CQ bit was checked by xiangze.zhang@intel.com

[xiangze.zhang, 2017-03-29 01:00:24]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/2764833003/#ps100001
(title: "fix nits")

[commit-bot: I haz the power, 2017-03-29 01:01:17]

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-29 01:16:18]

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

[commit-bot: I haz the power, 2017-03-29 01:16:22]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[xiangze.zhang, 2017-03-29 01:29:00]

The CQ bit was checked by xiangze.zhang@intel.com

[xiangze.zhang, 2017-03-29 01:29:00]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/2764833003/#ps120001
(title: "uniform location should be GLint")

[commit-bot: I haz the power, 2017-03-29 01:29: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-03-29 03:31:05]

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

[commit-bot: I haz the power, 2017-03-29 03:31:06]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[xiangze.zhang, 2017-03-29 04:58:23]

The CQ bit was checked by xiangze.zhang@intel.com

[commit-bot: I haz the power, 2017-03-29 04:59:07]

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-29 05:05:23]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490763503886300,
"parent_rev": "8c8e69e14bebb66cba7f31887865d76b24817080", "commit_rev":
"6ca5d0d3ad671604d9a028e71402e9e5ee25f110"}

[commit-bot: I haz the power, 2017-03-29 05:06:58]

Description was changed from

==========
Make gl_clear_broken workaround support core profile and use it under AMD Linux
Catalyst driver

Make ClearFrameBuffer use a VAO and add version to shaders in core
profile. Use gl_clear_broken workaround under Linux AMD Catalyst driver
because it ignores clear if it's the only thing rendered to the target
before the target is read.

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

==========
Make gl_clear_broken workaround support core profile and use it under AMD Linux
Catalyst driver

Make ClearFrameBuffer use a VAO and add version to shaders in core
profile. Use gl_clear_broken workaround under Linux AMD Catalyst driver
because it ignores clear if it's the only thing rendered to the target
before the target is read.

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

Review-Url: https://codereview.chromium.org/2764833003
Cr-Commit-Position: refs/heads/master@{#460282}
Committed:
https://chromium.googlesource.com/chromium/src/+/6ca5d0d3ad671604d9a028e71402...
==========

[commit-bot: I haz the power, 2017-03-29 05:07:00]

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

[sugoi1, 2017-03-30 14:41:27]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2786963002/ by sugoi@chromium.org.

The reason for reverting is: This cl broke the "Builder: Linux Release (AMD R7
240)" bot. See:
https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20(AMD...

.

[Ken Russell (switch to Gerrit), 2017-03-30 17:15:12]

On 2017/03/30 14:41:27, sugoi1 wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
> https://codereview.chromium.org/2786963002/ by mailto:sugoi@chromium.org.
> 
> The reason for reverting is: This cl broke the "Builder: Linux Release (AMD R7
> 240)" bot. See:
>
https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20(AMD...
> 
> .

See http://crbug.com/690122 for more details on the test failures.

[sunnyps, 2017-04-03 21:58:42]

On 2017/03/30 17:15:12, Ken Russell wrote:
> On 2017/03/30 14:41:27, sugoi1 wrote:
> > A revert of this CL (patchset #7 id:120001) has been created in
> > https://codereview.chromium.org/2786963002/ by mailto:sugoi@chromium.org.
> > 
> > The reason for reverting is: This cl broke the "Builder: Linux Release (AMD
R7
> > 240)" bot. See:
> >
>
https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20(AMD...
> > 
> > .
> 
> See http://crbug.com/690122 for more details on the test failures.

This also caused the failures on PowerVR on Android reported here:
https://bugs.chromium.org/p/chromium/issues/detail?id=707161#c7

I took a quick look at the patch but couldn't figure out the issue. Let's figure
it out before relanding this.