Initialize default texture for GL_TEXTURE_3D and GL_TEXTURE_2D_ARRAY

Initialize default texture for GL_TEXTURE_3D and GL_TEXTURE_2D_ARRAY

ES3 supports GL_TEXTURE_3D and GL_TEXTURE_2D_ARRAY for glBindTexture. So
we should create default texture the the two targets.

BUG=429053
Committed: https://crrev.com/4e2f0d09bae5ad062f5f35de0349f53f0f99122e
Cr-Commit-Position: refs/heads/master@{#349602}

[qiankun, 2015-09-14 11:01:43]

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

[qiankun, 2015-09-14 11:01:43]

PTAL.

[Zhenyao Mo, 2015-09-14 21:12:06]

Thanks for your contribution.  I think testing part of this CL still needs some
more work.

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

https://codereview.chromium.org/1335873002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:171:
InitDecoderWithCommandLine(init, &command_line);
No, we definitely don't want to test all decoder tests with ES3 enabled if
underlying is ES3.

Look at GLES3DecoderTest.  We want to explicitly control if we enable ES3 or
not.

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

https://codereview.chromium.org/1335873002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_extensions.cc:146:
command_line.AppendSwitch(switches::kEnableUnsafeES3APIs);
No, don't append this. ES2 is still our shipping config, so we need to test that
path.

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

https://codereview.chromium.org/1335873002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/test_helper.cc:206: if (gl_info.is_es3) {
This is also incorrect.  I assume you want to setup 3D texture stuff if ES3
support is enabled in command buffer, not the underlying driver is ES3.  Say you
are running on Linux desktop with GL 4.3, you may still want to handle 3D
textures if ES3 is enabled.

So I think you want to pass in a boolean (or enum) to control if you wanna
enable es3 or not.

https://codereview.chromium.org/1335873002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/test_helper.cc:275: const char* gl_version,
same here.

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

https://codereview.chromium.org/1335873002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1416: if
(feature_info_->IsES3Capable()) {
IsES3Enabled(). Otherwise we are still emulating ES2 behaviors in command buffer
and there is no need to deal with 3D textures.

[Zhenyao Mo, 2015-09-14 21:12:20]

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

[qiankun, 2015-09-15 09:14:59]

Thanks for review. I updated the CL. PTAL.

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

https://codereview.chromium.org/1335873002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:171:
InitDecoderWithCommandLine(init, &command_line);
On 2015/09/14 21:12:06, Zhenyao Mo wrote:
> No, we definitely don't want to test all decoder tests with ES3 enabled if
> underlying is ES3.
> 
> Look at GLES3DecoderTest.  We want to explicitly control if we enable ES3 or
> not.

Done.

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

https://codereview.chromium.org/1335873002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_extensions.cc:146:
command_line.AppendSwitch(switches::kEnableUnsafeES3APIs);
On 2015/09/14 21:12:06, Zhenyao Mo wrote:
> No, don't append this. ES2 is still our shipping config, so we need to test
that
> path.

Done.

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

https://codereview.chromium.org/1335873002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/test_helper.cc:206: if (gl_info.is_es3) {
On 2015/09/14 21:12:06, Zhenyao Mo wrote:
> This is also incorrect.  I assume you want to setup 3D texture stuff if ES3
> support is enabled in command buffer, not the underlying driver is ES3.  Say
you
> are running on Linux desktop with GL 4.3, you may still want to handle 3D
> textures if ES3 is enabled.
> 
> So I think you want to pass in a boolean (or enum) to control if you wanna
> enable es3 or not.

Done.

https://codereview.chromium.org/1335873002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/test_helper.cc:275: const char* gl_version,
On 2015/09/14 21:12:06, Zhenyao Mo wrote:
> same here.

Done.

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

https://codereview.chromium.org/1335873002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1416: if
(feature_info_->IsES3Capable()) {
On 2015/09/14 21:12:06, Zhenyao Mo wrote:
> IsES3Enabled(). Otherwise we are still emulating ES2 behaviors in command
buffer
> and there is no need to deal with 3D textures.

Done.

[Zhenyao Mo, 2015-09-15 18:37:16]

Mostly look good.

https://codereview.chromium.org/1335873002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/test_helper.cc (right):

https://codereview.chromium.org/1335873002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/test_helper.cc:310: EXPECT_CALL(*gl,
DeleteTextures(6, _))
This needs to be conditional.  If it's ES2, then 4.

https://codereview.chromium.org/1335873002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager_unittest.cc (right):

https://codereview.chromium.org/1335873002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager_unittest.cc:180:
TestHelper::SetupFeatureInfoInitExpectationsWithGLVersion(
Can you add a function to handle feature_info_ setup for ES3? So it can be
shared here and reused in the feature.

Also, can you test the ES2 cases AND the ES3 case? i.e., instead of modifying
UseDefaultTextures{True|False}, add UseDefaultTextures{True|False}ES3.

[qiankun, 2015-09-17 09:39:27]

https://codereview.chromium.org/1335873002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/test_helper.cc (right):

https://codereview.chromium.org/1335873002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/test_helper.cc:310: EXPECT_CALL(*gl,
DeleteTextures(6, _))
On 2015/09/15 18:37:15, Zhenyao Mo wrote:
> This needs to be conditional.  If it's ES2, then 4.

This tests "glDeleteTextures(arraysize(black_texture_ids_),
black_texture_ids_);" in TextureManager::Destroy(). So the size should equal
arraysize(black_texture_ids_). If no black texture is generated for 3D target,
then the related id is set as 0. glDeleteTextures silently ignores 0's and names
that do not correspond to existing textures.

https://codereview.chromium.org/1335873002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager_unittest.cc (right):

https://codereview.chromium.org/1335873002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager_unittest.cc:180:
TestHelper::SetupFeatureInfoInitExpectationsWithGLVersion(
On 2015/09/15 18:37:16, Zhenyao Mo wrote:
> Can you add a function to handle feature_info_ setup for ES3? So it can be
> shared here and reused in the feature.
> 
> Also, can you test the ES2 cases AND the ES3 case? i.e., instead of modifying
> UseDefaultTextures{True|False}, add UseDefaultTextures{True|False}ES3.

Done.

[Zhenyao Mo, 2015-09-17 14:47:05]

Mostly look good with some nits fix.

https://codereview.chromium.org/1335873002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/test_helper.cc (right):

https://codereview.chromium.org/1335873002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/test_helper.cc:310: EXPECT_CALL(*gl,
DeleteTextures(6, _))
On 2015/09/17 09:39:27, qiankun wrote:
> On 2015/09/15 18:37:15, Zhenyao Mo wrote:
> > This needs to be conditional.  If it's ES2, then 4.
> 
> This tests "glDeleteTextures(arraysize(black_texture_ids_),
> black_texture_ids_);" in TextureManager::Destroy(). So the size should equal
> arraysize(black_texture_ids_). If no black texture is generated for 3D target,
> then the related id is set as 0. glDeleteTextures silently ignores 0's and
names
> that do not correspond to existing textures.

I see.  You can use kNumDefaultTextures here instead.

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

https://codereview.chromium.org/1335873002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager_unittest.cc:262:
feature_info_->EnableES3Validators();
You can add a "bool enable_es3" to function SetupFeatureInfo, so you can also
hide the above details into the function, so here and above you don't have to
duplicate the code.

[qiankun, 2015-09-17 15:18:31]

Thanks for careful review!

https://codereview.chromium.org/1335873002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/test_helper.cc (right):

https://codereview.chromium.org/1335873002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/test_helper.cc:310: EXPECT_CALL(*gl,
DeleteTextures(6, _))
On 2015/09/17 14:47:05, Zhenyao Mo wrote:
> On 2015/09/17 09:39:27, qiankun wrote:
> > On 2015/09/15 18:37:15, Zhenyao Mo wrote:
> > > This needs to be conditional.  If it's ES2, then 4.
> > 
> > This tests "glDeleteTextures(arraysize(black_texture_ids_),
> > black_texture_ids_);" in TextureManager::Destroy(). So the size should equal
> > arraysize(black_texture_ids_). If no black texture is generated for 3D
target,
> > then the related id is set as 0. glDeleteTextures silently ignores 0's and
> names
> > that do not correspond to existing textures.
> 
> I see.  You can use kNumDefaultTextures here instead.

Done.

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

https://codereview.chromium.org/1335873002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager_unittest.cc:262:
feature_info_->EnableES3Validators();
On 2015/09/17 14:47:05, Zhenyao Mo wrote:
> You can add a "bool enable_es3" to function SetupFeatureInfo, so you can also
> hide the above details into the function, so here and above you don't have to
> duplicate the code.

Done.

[Zhenyao Mo, 2015-09-17 16:57:02]

LGTM after the default arg issue is fixed.

https://codereview.chromium.org/1335873002/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager_unittest.cc (right):

https://codereview.chromium.org/1335873002/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager_unittest.cc:101: bool enable_es3 =
false) {
OK, default argument is not allowed in Chrome.
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argu...

[qiankun, 2015-09-18 02:16:27]

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

[qiankun, 2015-09-18 02:16:28]

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/1335873002/#ps120001
(title: "fix default argument")

[commit-bot: I haz the power, 2015-09-18 02:16:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1335873002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1335873002/120001

[commit-bot: I haz the power, 2015-09-18 03:34:55]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-09-18 03:36:04]

Patchset 7 (id:??) landed as
https://crrev.com/4e2f0d09bae5ad062f5f35de0349f53f0f99122e
Cr-Commit-Position: refs/heads/master@{#349602}