Support GL_OES_EGL_image_external

Add support for GL_OES_EGL_image_external to the cmdbuffer code.


BUG=
TEST=new unit_tests

[no sievers, 2011-07-20 00:58:15]

Some questions for gman came up while hacking on this, see my comments.

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/build_gles2_...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/build_gles2_...
gpu/command_buffer/build_gles2_cmd_buffer.py:586: 'type': 'GLenum',
Adding this as a separate type is an easy way to allow glGetTexParameter() with
GL_TEXTURE_EXTERNAL_OES, but disallow TexImage/TexSubImage/CopyTexImage etc.

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:4392:
EXPECT_TRUE(info->wrap_t() == GL_CLAMP_TO_EDGE);
Darn, although it's easy to setup the 'default texture' for this target with the
right defaults, how about this test?
Shouldn't this have the defaults set according to the spec? 
If so, it seems like I have to initialize the TextureInfo with different
defaults.
Or is this testcase invalid?

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (left):

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:79:
EXPECT_TRUE(group_->Initialize(DisallowedExtensions(), NULL));
Is it intentional that we pass NULL here which ends up settings up feature_info
without any extensions?
While we do set the expectation for glGetString(GL_EXTENSIONS) to match the
extensions passed in the call above.

Changing this breaks two tests (but I haven't looked at why exactly yet):
GLES2DecoderManualInitTest.RenderbufferStorageMultisampleGLError
GLES2DecoderManualInitTest.GetCompressedTextureFormats

[greggman, 2011-07-20 01:41:17]

this looks GREAT!

I'm glad you were able to figure out something that seems to fit well with
what's there and I'm amazed you did such a great job without asking any
questions. If the roles were reversed I'd probably been at your desk for a week.

Please run the OpenGL ES 2.0 Conformance tests before submitting. You can only
run them on Linux or Windows currently (I hope that's not too much trouble)

I'll send direction in email

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/feat...
File gpu/command_buffer/service/feature_info_unittest.cc (right):

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/feat...
gpu/command_buffer/service/feature_info_unittest.cc:62:
EXPECT_FALSE(info_.feature_flags().chromium_webglsl);
Can you add a test that oes_egl_image_external is false here and a test that
it's true down below?

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2835: if
(feature_info_->feature_flags().oes_egl_image_external)
I don't think you need to check here. If the feature is not enabled validation
will prevent this getting called with GL_TEXTURE_EXTERNAL_OES

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4143:
TextureManager::TextureInfo* texture_info =
This seems like it should be a function now that it's complicated? Like
TextureUnit::GetInfoForTarget(GLenum target) ?

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4154: (uniform_info->type ==
GL_SAMPLER_EXTERNAL_OES ?
This also seems like it might be better as a function.

GetBindTargetForSampleType

or something?

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:4392:
EXPECT_TRUE(info->wrap_t() == GL_CLAMP_TO_EDGE);
Yea, it seems like you need to set the defaults inside TextureInfo::SetTarget

On 2011/07/20 00:58:16, Daniel Sievers wrote:
> Darn, although it's easy to setup the 'default texture' for this target with
the
> right defaults, how about this test?
> Shouldn't this have the defaults set according to the spec? 
> If so, it seems like I have to initialize the TextureInfo with different
> defaults.
> Or is this testcase invalid?

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (left):

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:79:
EXPECT_TRUE(group_->Initialize(DisallowedExtensions(), NULL));
Yes, NULL for that parameter means "give me all extensions that are available"
so about at like 76 we set which extensions the OpenGL mock will say are
available where as this line is saying which ones we want to enable.

On 2011/07/20 00:58:16, Daniel Sievers wrote:
> Is it intentional that we pass NULL here which ends up settings up
feature_info
> without any extensions?
> While we do set the expectation for glGetString(GL_EXTENSIONS) to match the
> extensions passed in the call above.
> 
> Changing this breaks two tests (but I haven't looked at why exactly yet):
> GLES2DecoderManualInitTest.RenderbufferStorageMultisampleGLError
> GLES2DecoderManualInitTest.GetCompressedTextureFormats

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/test...
File gpu/command_buffer/service/test_helper.cc (right):

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/test...
gpu/command_buffer/service/test_helper.cc:56: const char* extensions) {
Would it be possible to pass in a FeatureInfo here instead of this string that
needs to be parsed? That makes more sense to me because it leaves it up to the
real code whether or not it has been turned on.

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/text...
File gpu/command_buffer/service/texture_manager.h (right):

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/text...
gpu/command_buffer/service/texture_manager.h:281: case GL_TEXTURE_2D:
What about if this and the next function had all 3 cases and a 

default:
  NOTREACHED()
  return 0;
}

?

[no sievers, 2011-07-20 23:00:37]

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:4392:
EXPECT_TRUE(info->wrap_t() == GL_CLAMP_TO_EDGE);
That was the place I was looking for. Also eliminates the SetParameter() calls
when setting up the external default texture.

On 2011/07/20 01:41:18, greggman wrote:
> Yea, it seems like you need to set the defaults inside TextureInfo::SetTarget
> 
> On 2011/07/20 00:58:16, Daniel Sievers wrote:
> > Darn, although it's easy to setup the 'default texture' for this target with
> the
> > right defaults, how about this test?
> > Shouldn't this have the defaults set according to the spec? 
> > If so, it seems like I have to initialize the TextureInfo with different
> > defaults.
> > Or is this testcase invalid?
>

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (left):

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/gles...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:79:
EXPECT_TRUE(group_->Initialize(DisallowedExtensions(), NULL));
I think I confused myself here due to a typo I had somewhere else. I reverted
the change, it works now with passing NULL here as expected.

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/test...
File gpu/command_buffer/service/test_helper.cc (right):

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/test...
gpu/command_buffer/service/test_helper.cc:56: const char* extensions) {
Is it ok to interleave the expectation setup and GL mock calls here though?

It would have to be:
EXPECT_CALL(glGetString())
FeatureInfo info; 
info.Initialize(); // calls glGetString()
...
// set up other expectations

It seems to work. But it breaks the test using GL_EXT_texture_compression_s3tc
and the one using GL_EXT_framebuffer_multisample, because FeatureInfo expects
one to either request/desire GL_CHROMIUM_framebuffer_multisample or
GL_EXT_texture_compression_dxt1, GL_CHROMIUM_texture_compression_dxt3,
GL_CHROMIUM_texture_compression_dxt5.

At the same time I cannot change the unit test to use those extension strings,
because they are also used to mock the glGetString() result. 

However, I can change FeatureInfo to make it work and treat these EXT strings
accordingly.


On 2011/07/20 01:41:18, greggman wrote:
> Would it be possible to pass in a FeatureInfo here instead of this string that
> needs to be parsed? That makes more sense to me because it leaves it up to the
> real code whether or not it has been turned on.

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/text...
File gpu/command_buffer/service/texture_manager.h (right):

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/text...
gpu/command_buffer/service/texture_manager.h:281: case GL_TEXTURE_2D:
I originally had it that way but it broke stuff, because this gets also called
from e.g. DoCopyTexImage2D() -> ValidForTarget() where the target is a cubemap
face (so it really has 9 valid cases here).

I added the corresponding testcases in (TextureManagerTest, MaxValues) in
texture_manager_unittest.cc.

On 2011/07/20 01:41:18, greggman wrote:
> What about if this and the next function had all 3 cases and a 
> 
> default:
>   NOTREACHED()
>   return 0;
> }
> 
> ?

[greggman, 2011-07-21 07:49:32]

lgtm

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/test...
File gpu/command_buffer/service/test_helper.cc (right):

http://codereview.chromium.org/7458008/diff/1/gpu/command_buffer/service/test...
gpu/command_buffer/service/test_helper.cc:56: const char* extensions) {
I think I see your point. I guess leave it the way you first had it if it was
working.

[commit-bot: I haz the power, 2011-07-26 12:03:31]

Presubmit check for 7458008-17001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  gpu/command_buffer/client/gles2_implementation_autogen.h, line 674, 224 chars
\
  gpu/command_buffer/client/gles2_implementation_autogen.h, line 693, 224 chars
\
  gpu/command_buffer/build_gles2_cmd_buffer.py, line 133, 136 chars \
  gpu/command_buffer/build_gles2_cmd_buffer.py, line 134, 134 chars

License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
  gpu/GLES2/gl2ext.h

Presubmit checks took 1.7s to calculate.

[commit-bot: I haz the power, 2011-07-26 12:47:32]

Presubmit check for 7458008-17001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  gpu/command_buffer/client/gles2_implementation_autogen.h, line 674, 224 chars
\
  gpu/command_buffer/client/gles2_implementation_autogen.h, line 693, 224 chars
\
  gpu/command_buffer/build_gles2_cmd_buffer.py, line 133, 136 chars \
  gpu/command_buffer/build_gles2_cmd_buffer.py, line 134, 134 chars

License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
  gpu/GLES2/gl2ext.h

Presubmit checks took 1.7s to calculate.

[bulach, 2011-07-26 15:31:10]

Landed on Daniel's behalf at:

http://codereview.chromium.org/7480014