Force CUBE_MAP_POSITIVE_X texture allocation before CopyTexImage2D on Intel Mac

Force CUBE_MAP_POSITIVE_X texture allocation before CopyTexImage2D on Intel Mac

CopyTexImage2D has bug for cube map texture target on Intel Mac, if the
CUBE_MAP_POSITIVE_X texture is not allocated yet.

This CL works around this bug by doing TexImage2D to allocate the CUBE_MAP_POSITIVE_X
texture before CopyTexImage2D if target is one face of a cube map texture.

BUG=648197
TEST=deqp/functional/gles3/texturespecification/basic_copyteximage2d.html
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

Committed: https://crrev.com/45903b6bfa81d0257af4da576e8edee825093b6b
Cr-Commit-Position: refs/heads/master@{#420575}

[qiankun, 2016-09-19 11:30:20]

Description was changed from

==========
Do TexImage2D before CopyTexImage2D on Intel Mac OSX

CopyTexImage2D copies nothing to GL_TEXTURE_CUBE_MAP_NEGATIVE_X target
on Intel Mac. This CL works around this bug by doing TexImage2D before
CopyTexImage2D if target is GL_TEXTURE_CUBE_MAP_NEGATIVE_X.

BUG=648197
TEST=deqp/functional/gles3/texturespecification/basic_copyteximage2d.html
==========

to

==========
Do TexImage2D before CopyTexImage2D on Intel Mac OSX

CopyTexImage2D copies nothing to GL_TEXTURE_CUBE_MAP_NEGATIVE_X target
on Intel Mac. This CL works around this bug by doing TexImage2D before
CopyTexImage2D if target is GL_TEXTURE_CUBE_MAP_NEGATIVE_X.

BUG=648197
TEST=deqp/functional/gles3/texturespecification/basic_copyteximage2d.html
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
==========

[qiankun, 2016-09-19 11:37:43]

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

[commit-bot: I haz the power, 2016-09-19 11:37:53]

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-09-19 12:28:24]

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

[commit-bot: I haz the power, 2016-09-19 12:28:25]

Dry run: This issue passed the CQ dry run.

[qiankun, 2016-09-19 14:16:14]

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

[qiankun, 2016-09-19 14:16:15]

PTAL.

[yunchao, 2016-09-19 15:13:17]

On 2016/09/19 14:16:15, qiankun wrote:
> PTAL.

Good catch, qiankun. I wonder what about
GL_TEXTURE_CUBE_MAP_{POSITIVE_X|NEGATIVE_Y|POSITIVE_Y|NEGATIVE_Z|POSITIVE_Z}. Do
we have conformance tests against these faces on Mac Intel?

[qiankun, 2016-09-19 15:58:39]

On 2016/09/19 15:13:17, yunchao wrote:
> On 2016/09/19 14:16:15, qiankun wrote:
> > PTAL.
> 
> Good catch, qiankun. I wonder what about
> GL_TEXTURE_CUBE_MAP_{POSITIVE_X|NEGATIVE_Y|POSITIVE_Y|NEGATIVE_Z|POSITIVE_Z}.
Do
> we have conformance tests against these faces on Mac Intel?

Other cube map targets work well.
deqp/functional/gles3/texturespecification/basic_copyteximage2d.html covers
these tests.

[piman, 2016-09-19 20:33:14]

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

https://codereview.chromium.org/2348243002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13294: target ==
GL_TEXTURE_CUBE_MAP_NEGATIVE_X) {
Is it /really/ only GL_TEXTURE_CUBE_MAP_NEGATIVE_X that is affected? Or is is
that maybe if the texture isn't cube complete - or maybe if
GL_TEXTURE_CUBE_MAP_POSITIVE_X specifically isn't defined?

This sounds similar to the workaround we have for TexImage2D - see logic about
'need_cube_map_workaround' in TextureManager::ValidateAndDoTexImage. Could we
reuse that logic?

https://codereview.chromium.org/2348243002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13298: width, height, border,
format, type, 0);
What if those were already defined? This would lose the pixels.

[Ken Russell (switch to Gerrit), 2016-09-19 23:15:10]

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

https://codereview.chromium.org/2348243002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13294: target ==
GL_TEXTURE_CUBE_MAP_NEGATIVE_X) {
On 2016/09/19 20:33:13, piman OOO back 2016-09-12 wrote:
> Is it /really/ only GL_TEXTURE_CUBE_MAP_NEGATIVE_X that is affected? Or is is
> that maybe if the texture isn't cube complete - or maybe if
> GL_TEXTURE_CUBE_MAP_POSITIVE_X specifically isn't defined?
> 
> This sounds similar to the workaround we have for TexImage2D - see logic about
> 'need_cube_map_workaround' in TextureManager::ValidateAndDoTexImage. Could we
> reuse that logic?

+1 to piman's comments -- also, if a separate workaround is absolutely required,
please name it something more precise. Perhaps
"do_teximage_before_copyteximage_to_cube_map".

FYI, I think the needs_cube_map_workaround code may be buggy. I am planning to
eliminate the need for it at least for CopyTextureCHROMIUM by changing
CopyTextureCHROMIUM in http://crbug.com/612542 .

https://codereview.chromium.org/2348243002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13298: width, height, border,
format, type, 0);
On 2016/09/19 20:33:13, piman OOO back 2016-09-12 wrote:
> What if those were already defined? This would lose the pixels.

+1. Also, have you verified that it's necessary to define all six faces and not
just GL_TEXTURE_CUBE_MAP_POSITIVE_X?

[qiankun, 2016-09-20 11:04:05]

Description was changed from

==========
Do TexImage2D before CopyTexImage2D on Intel Mac OSX

CopyTexImage2D copies nothing to GL_TEXTURE_CUBE_MAP_NEGATIVE_X target
on Intel Mac. This CL works around this bug by doing TexImage2D before
CopyTexImage2D if target is GL_TEXTURE_CUBE_MAP_NEGATIVE_X.

BUG=648197
TEST=deqp/functional/gles3/texturespecification/basic_copyteximage2d.html
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

==========
Do TexImage2D before CopyTexImage2D for cube map texture on Intel Mac OSX

CopyTexImage2D has bug for cube map texture target on Intel Mac, if the
CUBE_MAP_POSITIVE_X texture is not allocated yet. 

This CL works around this bug by doing TexImage2D to allocate the
CUBE_MAP_POSITIVE_X 
texture before CopyTexImage2D if target is one face of a cube map texture.

BUG=648197
TEST=deqp/functional/gles3/texturespecification/basic_copyteximage2d.html
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
==========

[qiankun, 2016-09-20 11:34:16]

Thanks piman and Ken. I updated the CL. PTAL.

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

https://codereview.chromium.org/2348243002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13294: target ==
GL_TEXTURE_CUBE_MAP_NEGATIVE_X) {
On 2016/09/19 20:33:13, piman OOO back 2016-09-12 wrote:
> Is it /really/ only GL_TEXTURE_CUBE_MAP_NEGATIVE_X that is affected? Or is is
> that maybe if the texture isn't cube complete - or maybe if
> GL_TEXTURE_CUBE_MAP_POSITIVE_X specifically isn't defined?
> 
> This sounds similar to the workaround we have for TexImage2D - see logic about
> 'need_cube_map_workaround' in TextureManager::ValidateAndDoTexImage. Could we
> reuse that logic?

Yes. They are similar. I used the same workaround as TexImage2D. Thanks for
pointing it.

https://codereview.chromium.org/2348243002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13298: width, height, border,
format, type, 0);
On 2016/09/19 23:15:10, Ken Russell wrote:
> On 2016/09/19 20:33:13, piman OOO back 2016-09-12 wrote:
> > What if those were already defined? This would lose the pixels.
> 
> +1. Also, have you verified that it's necessary to define all six faces and
not
> just GL_TEXTURE_CUBE_MAP_POSITIVE_X?

Do initialization for uninitialized GL_TEXTURE_CUBE_MAP_POSITIVE_X texture now.

[qiankun, 2016-09-20 16:55:15]

Description was changed from

==========
Do TexImage2D before CopyTexImage2D for cube map texture on Intel Mac OSX

CopyTexImage2D has bug for cube map texture target on Intel Mac, if the
CUBE_MAP_POSITIVE_X texture is not allocated yet. 

This CL works around this bug by doing TexImage2D to allocate the
CUBE_MAP_POSITIVE_X 
texture before CopyTexImage2D if target is one face of a cube map texture.

BUG=648197
TEST=deqp/functional/gles3/texturespecification/basic_copyteximage2d.html
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

==========
Force CUBE_MAP_POSITIVE_X texture allocation before CopyTexImage2D on Intel Mac

CopyTexImage2D has bug for cube map texture target on Intel Mac, if the
CUBE_MAP_POSITIVE_X texture is not allocated yet. 

This CL works around this bug by doing TexImage2D to allocate the
CUBE_MAP_POSITIVE_X 
texture before CopyTexImage2D if target is one face of a cube map texture.

BUG=648197
TEST=deqp/functional/gles3/texturespecification/basic_copyteximage2d.html
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
==========

[piman, 2016-09-20 20:51:45]

https://codereview.chromium.org/2348243002/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/2348243002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2424: } else {
Do we need to restrict this path to if args.target !=
GL_TEXTURE_CUBE_MAP_POSITIVE_X? This is ensured in the ValidateAndDoTexImage
path (which explicitly checks args.target != GL_TEXTURE_CUBE_MAP_POSITIVE_X),
but not in the new copy path.

https://codereview.chromium.org/2348243002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2444: DoTexImage(texture_state,
state, framebuffer_state,
The comment on l.2472 explains that this code only works if the unpack buffer is
not bound (because we're passing a pointer to CPU data, not buffer data), and
indeed it was only called if !feature_info_->IsES3Enabled(), which ensures we
don't have it bound.
In the new copy path there is no such check (and indeed it sounds like it
matters on mac even when ES3 is enabled), so maybe we need to resolve this TODO
first?

https://codereview.chromium.org/2348243002/diff/120001/gpu/config/gpu_driver_...
File gpu/config/gpu_driver_bug_workaround_type.h (right):

https://codereview.chromium.org/2348243002/diff/120001/gpu/config/gpu_driver_...
gpu/config/gpu_driver_bug_workaround_type.h:81:
do_teximage_before_copyteximage_to_cube_map)        \
Do we need this new workaround type, or would reusing
force_cube_map_positive_x_allocation be appropriate?

[qiankun, 2016-09-21 02:38:17]

https://codereview.chromium.org/2348243002/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/2348243002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2424: } else {
On 2016/09/20 20:51:44, piman OOO back 2016-09-12 wrote:
> Do we need to restrict this path to if args.target !=
> GL_TEXTURE_CUBE_MAP_POSITIVE_X? This is ensured in the ValidateAndDoTexImage
> path (which explicitly checks args.target != GL_TEXTURE_CUBE_MAP_POSITIVE_X),
> but not in the new copy path.

Add the restriction to WorkaroundCopyTexImageCubeMap() and add back the ASSERT.
GL_TEXTURE_CUBE_MAP_POSITIVE_X doesn't need this workaround.

https://codereview.chromium.org/2348243002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2444: DoTexImage(texture_state,
state, framebuffer_state,
On 2016/09/20 20:51:44, piman OOO back 2016-09-12 wrote:
> The comment on l.2472 explains that this code only works if the unpack buffer
is
> not bound (because we're passing a pointer to CPU data, not buffer data), and
> indeed it was only called if !feature_info_->IsES3Enabled(), which ensures we
> don't have it bound.
> In the new copy path there is no such check (and indeed it sounds like it
> matters on mac even when ES3 is enabled), so maybe we need to resolve this
TODO
> first?

I resolved this TODO: reset unpack buffer before this workaround and restore it
after the workaround.

https://codereview.chromium.org/2348243002/diff/120001/gpu/config/gpu_driver_...
File gpu/config/gpu_driver_bug_workaround_type.h (right):

https://codereview.chromium.org/2348243002/diff/120001/gpu/config/gpu_driver_...
gpu/config/gpu_driver_bug_workaround_type.h:81:
do_teximage_before_copyteximage_to_cube_map)        \
On 2016/09/20 20:51:44, piman OOO back 2016-09-12 wrote:
> Do we need this new workaround type, or would reusing
> force_cube_map_positive_x_allocation be appropriate?

I didn't see this CopyTexImage2D bug for cases which enable
force_cube_map_positive_x_allocation. Currently, it only occurs on Intel Mac. So
I added a new workaround type. In future, if this bug can be reproduced on all
cases which require force_cube_map_positive_x_allocation workaround, I can reuse
force_cube_map_positive_x_allocation type.

[piman, 2016-09-21 05:37:20]

lgtm

https://codereview.chromium.org/2348243002/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/2348243002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:326:
state_->SetBoundBuffer(GL_PIXEL_UNPACK_BUFFER, nullptr);
nit: I don't think we need to modify state_ here (or below), we can simply set
the GL state, and reset it in the destructor (using state_ then, turns out)

[Ken Russell (switch to Gerrit), 2016-09-22 02:05:35]

I defer to piman's review. The code looks good to me.

[qiankun, 2016-09-23 03:01:37]

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

[qiankun, 2016-09-23 03:01:38]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2348243002/#ps200001
(title: "fix nit")

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

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-09-23 03:04:44]

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

[commit-bot: I haz the power, 2016-09-23 03:04:45]

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-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[qiankun, 2016-09-23 03:20:14]

https://codereview.chromium.org/2348243002/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/2348243002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:326:
state_->SetBoundBuffer(GL_PIXEL_UNPACK_BUFFER, nullptr);
On 2016/09/21 05:37:20, piman OOO back 2016-09-26 wrote:
> nit: I don't think we need to modify state_ here (or below), we can simply set
> the GL state, and reset it in the destructor (using state_ then, turns out)

Removed state_ modification.

[qiankun, 2016-09-23 03:20:25]

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

[qiankun, 2016-09-23 03:20:26]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2348243002/#ps220001
(title: "rebase only")

[commit-bot: I haz the power, 2016-09-23 03:20:47]

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-09-23 04:29:16]

Description was changed from

==========
Force CUBE_MAP_POSITIVE_X texture allocation before CopyTexImage2D on Intel Mac

CopyTexImage2D has bug for cube map texture target on Intel Mac, if the
CUBE_MAP_POSITIVE_X texture is not allocated yet. 

This CL works around this bug by doing TexImage2D to allocate the
CUBE_MAP_POSITIVE_X 
texture before CopyTexImage2D if target is one face of a cube map texture.

BUG=648197
TEST=deqp/functional/gles3/texturespecification/basic_copyteximage2d.html
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

==========
Force CUBE_MAP_POSITIVE_X texture allocation before CopyTexImage2D on Intel Mac

CopyTexImage2D has bug for cube map texture target on Intel Mac, if the
CUBE_MAP_POSITIVE_X texture is not allocated yet. 

This CL works around this bug by doing TexImage2D to allocate the
CUBE_MAP_POSITIVE_X 
texture before CopyTexImage2D if target is one face of a cube map texture.

BUG=648197
TEST=deqp/functional/gles3/texturespecification/basic_copyteximage2d.html
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
==========

[commit-bot: I haz the power, 2016-09-23 04:29:18]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-09-23 04:31:01]

Description was changed from

==========
Force CUBE_MAP_POSITIVE_X texture allocation before CopyTexImage2D on Intel Mac

CopyTexImage2D has bug for cube map texture target on Intel Mac, if the
CUBE_MAP_POSITIVE_X texture is not allocated yet. 

This CL works around this bug by doing TexImage2D to allocate the
CUBE_MAP_POSITIVE_X 
texture before CopyTexImage2D if target is one face of a cube map texture.

BUG=648197
TEST=deqp/functional/gles3/texturespecification/basic_copyteximage2d.html
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

==========
Force CUBE_MAP_POSITIVE_X texture allocation before CopyTexImage2D on Intel Mac

CopyTexImage2D has bug for cube map texture target on Intel Mac, if the
CUBE_MAP_POSITIVE_X texture is not allocated yet.

This CL works around this bug by doing TexImage2D to allocate the
CUBE_MAP_POSITIVE_X
texture before CopyTexImage2D if target is one face of a cube map texture.

BUG=648197
TEST=deqp/functional/gles3/texturespecification/basic_copyteximage2d.html
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

Committed: https://crrev.com/45903b6bfa81d0257af4da576e8edee825093b6b
Cr-Commit-Position: refs/heads/master@{#420575}
==========

[commit-bot: I haz the power, 2016-09-23 04:31:02]

Patchset 12 (id:??) landed as
https://crrev.com/45903b6bfa81d0257af4da576e8edee825093b6b
Cr-Commit-Position: refs/heads/master@{#420575}