Force cube map texture complete on Linux AMD

Force cube map texture complete on Linux AMD

CopyTex{Sub}Image2D reports INVALID_OPERATION error if the destination
texture is an integer incomplete cube map texture on Linux AMD. Similarly,
TexSubImage2D reports OUT_OF_MEMORY error if the destination texture is a
srgb incomplete cube map texture on Linux AMD. This is a bug in AMD Linux
driver. This CL workarounds this bug by forcing the texture to be cube map
complete.

BUG=710392, 712117
TEST=webgl2 conformance test
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/2821913002
Cr-Commit-Position: refs/heads/master@{#465915}
Committed: https://chromium.googlesource.com/chromium/src/+/9d28c22de85528d1f87bfbf3f88dc531027b24bd

[qiankun, 2017-04-17 10:29:58]

Description was changed from

==========
Force cube map texture complete on Linux AMD

CopyTex{Sub}Image2D reports INVALID_OPERATION error if the destination
texture is an integer incomplete cube map texture on Linux AMD. This is
a bug in AMD Linux driver. This CL workarounds this bug by forcing the
texture to be cube map complete.

BUG=710392, 712117
TEST=webgl2 conformance test
==========

to

==========
Force cube map texture complete on Linux AMD

CopyTex{Sub}Image2D reports INVALID_OPERATION error if the destination
texture is an integer incomplete cube map texture on Linux AMD. This is
a bug in AMD Linux driver. This CL workarounds this bug by forcing the
texture to be cube map complete.

BUG=710392, 712117
TEST=webgl2 conformance test
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
==========

[qiankun, 2017-04-17 10:34:27]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 10:34: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-04-17 10:37:22]

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

[commit-bot: I haz the power, 2017-04-17 10:37:22]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  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_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[qiankun, 2017-04-18 07:00:00]

Patchset #1 (id:1) has been deleted

[qiankun, 2017-04-18 07:01:50]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 07:02:35]

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-04-18 08:25:30]

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

[commit-bot: I haz the power, 2017-04-18 08:25:31]

Dry run: This issue passed the CQ dry run.

[qiankun, 2017-04-18 08:55:58]

qiankun.miao@intel.com changed reviewers:
+ kbr@chromium.org, yunchao.he@intel.com, zmo@chromium.org

[qiankun, 2017-04-18 08:55:58]

PTAL.
With this workaround conformance test in
https://github.com/KhronosGroup/WebGL/pull/2369 can pass now.

[qiankun, 2017-04-18 09:23:44]

https://codereview.chromium.org/2821913002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/2821913002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:414:
workarounds.unpack_image_height_workaround_with_unpack_buffer) {}
I moved this constructor to .cc file to avoid "[chromium-style] Complex
constructor has an inlined body" build error.

[qiankun, 2017-04-18 11:25:27]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 11:25: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-04-18 12:53:28]

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

[commit-bot: I haz the power, 2017-04-18 12:53:29]

Dry run: This issue passed the CQ dry run.

[qiankun, 2017-04-18 16:45:10]

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

https://codereview.chromium.org/2821913002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:2612:
texture_state->force_int_cube_texture_complete &&
With this workaround, we can fix the integer texture related conformance test
failures on Linux after GPU-GPU path was enabled in blink. I just found the srgb
related failures are similar as integer related failures. So, I would like to
reuse force_cube_complete workaround and check internal_formats are integer
formats and srgb formats here. What do you think?

[Zhenyao Mo, 2017-04-18 17:48:02]

Mostly looks good with one issue

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

https://codereview.chromium.org/2821913002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:2556: defined = defined && width
== args.width && height == args.height;
We should not modify a face level if it's already defined. Otherwise you
internally change something users define.

I think in this case we should just return and don't apply workaround.

https://codereview.chromium.org/2821913002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:2612:
texture_state->force_int_cube_texture_complete &&
On 2017/04/18 16:45:10, qiankun wrote:
> With this workaround, we can fix the integer texture related conformance test
> failures on Linux after GPU-GPU path was enabled in blink. I just found the
srgb
> related failures are similar as integer related failures. So, I would like to
> reuse force_cube_complete workaround and check internal_formats are integer
> formats and srgb formats here. What do you think?

This sounds fine.

[qiankun, 2017-04-19 11:21:02]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 11:21:17]

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-04-19 12:20:36]

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

[commit-bot: I haz the power, 2017-04-19 12:20:37]

Dry run: This issue passed the CQ dry run.

[qiankun, 2017-04-19 12:26:35]

Zhenyao, thanks for your comments. I updated this CL. Please take another look.

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

https://codereview.chromium.org/2821913002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:2556: defined = defined && width
== args.width && height == args.height;
On 2017/04/18 17:48:02, Zhenyao Mo wrote:
> We should not modify a face level if it's already defined. Otherwise you
> internally change something users define.
> 
> I think in this case we should just return and don't apply workaround.

If we don't apply workaround, the bug still exists. 
Per my understanding, all the faces of a cube map texture should be in same size
in the buggy driver. If we will update one face with a different size when the
driver requires force_cube_complete workaround, then it is supposed all other
faces will be updated in future. Otherwise, users will not get what they want
but GL errors. So, we can apply the workaround. But, as your mentioned, it
changs content of other faces users define.

https://codereview.chromium.org/2821913002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:2612:
texture_state->force_int_cube_texture_complete &&
On 2017/04/18 17:48:02, Zhenyao Mo wrote:
> On 2017/04/18 16:45:10, qiankun wrote:
> > With this workaround, we can fix the integer texture related conformance
test
> > failures on Linux after GPU-GPU path was enabled in blink. I just found the
> srgb
> > related failures are similar as integer related failures. So, I would like
to
> > reuse force_cube_complete workaround and check internal_formats are integer
> > formats and srgb formats here. What do you think?
> 
> This sounds fine.

I found we can not reuse force_cube_complete workaround, since there are not the
same situation on Linux AMD as other platforms. I added a standalone
force_int_or_srgb_cube_complete workaround for Linux AMD. This workaround should
only be applied on Linux AMD. We use the same method DoCubeMapWrokaround to do
the real things.

[Zhenyao Mo, 2017-04-19 17:11:53]

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

https://codereview.chromium.org/2821913002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:2556: defined = defined && width
== args.width && height == args.height;
I understand the issue, but still, I think modifying user defined texture
silently internally is worse than having a driver bug.  I'd rather mark these
tests as fail.

https://codereview.chromium.org/2821913002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:2611:
(feature_info_->IsWebGL2OrES3Context() &&
This isn't correct.  You can also get sRGB formats on ES2/WebGL1 through
extension.  Let's just remove this condition.

[qiankun, 2017-04-20 01:35:53]

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

https://codereview.chromium.org/2821913002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:2556: defined = defined && width
== args.width && height == args.height;
On 2017/04/19 17:11:53, Zhenyao Mo wrote:
> I understand the issue, but still, I think modifying user defined texture
> silently internally is worse than having a driver bug.  I'd rather mark these
> tests as fail.

Sounds good. Removed it.

https://codereview.chromium.org/2821913002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:2611:
(feature_info_->IsWebGL2OrES3Context() &&
On 2017/04/19 17:11:53, Zhenyao Mo wrote:
> This isn't correct.  You can also get sRGB formats on ES2/WebGL1 through
> extension.  Let's just remove this condition.

What I meant is other platforms doesn't have such bug about integer and srgb
issue like Linux AMD. They require force_cube_complete due to other reasons. If
we reuse force_cube_complete workaround, other platforms which don't need to
apply the workaround for integer and srgb cube texture will apply the
workaround.

[qiankun, 2017-04-20 01:43:10]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 01:43:53]

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

[qiankun, 2017-04-20 01:47:52]

Description was changed from

==========
Force cube map texture complete on Linux AMD

CopyTex{Sub}Image2D reports INVALID_OPERATION error if the destination
texture is an integer incomplete cube map texture on Linux AMD. This is
a bug in AMD Linux driver. This CL workarounds this bug by forcing the
texture to be cube map complete.

BUG=710392, 712117
TEST=webgl2 conformance test
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

==========
Force cube map texture complete on Linux AMD

CopyTex{Sub}Image2D reports INVALID_OPERATION error if the destination
texture is an integer incomplete cube map texture on Linux AMD. Similarly,
TexSubImage2D reports OUT_OF_MEMORY error if the destination texture is a
srgb incomplete cube map texture on Linux AMD. This is a bug in AMD Linux
driver. This CL workarounds this bug by forcing the texture to be cube map
complete.

BUG=710392, 712117
TEST=webgl2 conformance test
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
==========

[qiankun, 2017-04-20 02:12:32]

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

https://codereview.chromium.org/2821913002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:2611:
(feature_info_->IsWebGL2OrES3Context() &&
On 2017/04/20 01:35:53, qiankun wrote:
> On 2017/04/19 17:11:53, Zhenyao Mo wrote:
> > This isn't correct.  You can also get sRGB formats on ES2/WebGL1 through
> > extension.  Let's just remove this condition.
> 
> What I meant is other platforms doesn't have such bug about integer and srgb
> issue like Linux AMD. They require force_cube_complete due to other reasons.
If
> we reuse force_cube_complete workaround, other platforms which don't need to
> apply the workaround for integer and srgb cube texture will apply the
> workaround.

Never mind. Please ignore my last comments. I removed the condition.

[Zhenyao Mo, 2017-04-20 03:46:17]

lgtm

[qiankun, 2017-04-20 03:54:42]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

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

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-04-20 04:52:04]

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

[commit-bot: I haz the power, 2017-04-20 04:52:06]

Dry run: This issue passed the CQ dry run.

[qiankun, 2017-04-20 06:18:20]

The CQ bit was checked by qiankun.miao@intel.com

[commit-bot: I haz the power, 2017-04-20 06:18: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-04-20 06:22:11]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492669100669720,
"parent_rev": "50ca7fd5a6467bb5203cff50d81bd09bcb8cfacd", "commit_rev":
"9d28c22de85528d1f87bfbf3f88dc531027b24bd"}

[commit-bot: I haz the power, 2017-04-20 06:22:17]

Description was changed from

==========
Force cube map texture complete on Linux AMD

CopyTex{Sub}Image2D reports INVALID_OPERATION error if the destination
texture is an integer incomplete cube map texture on Linux AMD. Similarly,
TexSubImage2D reports OUT_OF_MEMORY error if the destination texture is a
srgb incomplete cube map texture on Linux AMD. This is a bug in AMD Linux
driver. This CL workarounds this bug by forcing the texture to be cube map
complete.

BUG=710392, 712117
TEST=webgl2 conformance test
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

==========
Force cube map texture complete on Linux AMD

CopyTex{Sub}Image2D reports INVALID_OPERATION error if the destination
texture is an integer incomplete cube map texture on Linux AMD. Similarly,
TexSubImage2D reports OUT_OF_MEMORY error if the destination texture is a
srgb incomplete cube map texture on Linux AMD. This is a bug in AMD Linux
driver. This CL workarounds this bug by forcing the texture to be cube map
complete.

BUG=710392, 712117
TEST=webgl2 conformance test
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/2821913002
Cr-Commit-Position: refs/heads/master@{#465915}
Committed:
https://chromium.googlesource.com/chromium/src/+/9d28c22de85528d1f87bfbf3f88d...
==========

[commit-bot: I haz the power, 2017-04-20 06:22:18]

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