Adds support for the GL_ANGLE_texture_usage and GL_EXT_texture_storage

Adds support for the GL_ANGLE_texture_usage and GL_EXT_texture_storage
extensions to the command buffer and the WebGraphicsContext3D APIs.

BUG=106142
TEST=unittests 
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113223

[Vangelis Kokkevis, 2011-12-02 06:16:11]

Please review...

[vangelis, 2011-12-02 08:27:42]

On 2011/12/02 06:16:11, Vangelis Kokkevis wrote:
> Please review...

Having trouble getting the TextureManagerTests to pass.  I somehow need to
enable the ANGLE extension to get the validator to accept the new enum.

[Vangelis Kokkevis, 2011-12-02 17:30:25]

On 2011/12/02 08:27:42, vangelis wrote:
> On 2011/12/02 06:16:11, Vangelis Kokkevis wrote:
> > Please review...
> 
> Having trouble getting the TextureManagerTests to pass.  I somehow need to
> enable the ANGLE extension to get the validator to accept the new enum.

The unit test issue should be fixed now.  Doing another try run.

[greggman, 2011-12-02 18:33:24]

looks good. Just a few issues. Sorry this is so painful

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/build_gle...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/build_gle...
gpu/command_buffer/build_gles2_cmd_buffer.py:1058: 'GL_RGBA8_OES',
these formats probably need to be added to
src/gpu/command_buffer/common/gles2_cmd_utils.cc 
in ElementsPerGroup and GetChannelsForFormat

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6740: if
(teximage2d_faster_than_texsubimage2d_ && xoffset == 0 && yoffset == 0) {
You'll need to check the texture is not immutable (see below)

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7696: {
This is not chromium style

should be

type function(args) {
  switch (value) {
    case CONST1:
      return value;
    case CONST2:
      return value;
   ...
   }
}

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7722: GLenum
ExtractTypeFromStorageFormat(GLenum internalformat)
style issues same as above. Should be static?

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7749: error::Error
GLES2DecoderImpl::DoTexStorage2DEXT(
There's no reason for this to return an error. should be void.

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7756: feature_info_, target, 1,
width, height, 1) ||
the 1 after target should be 0

Also, you probably need to check that levels is correct for the given width and
height

There's a function, ComputeMipMapCount, in TextureManger.cc you could expose as
a static function on TextureManager

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7777:
texture_manager()->SetLevelInfo(
I think you're going to need to add a field 'immutable' to
TextureManager::TextureInfo and set it here

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7783:
texture_manager()->MarkMipmapsGenerated(feature_info_, info);
This should be inside the if. 

You should also consider changing MarkMipmapsGenerated so it takes another bool
arg "cleared"

Pass that value as the last argument to SetLevelInfo (I'll put a comment over
there)

Here you'd pass false. In DoGenerateMipmap you'd pass true.

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager.cc (right):

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:114: const FeatureInfo*
feature_info) {
add 'bool cleared'?

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:140: true);
change this true to 'cleared'

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager_unittest.cc (right):

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager_unittest.cc:127:
TEST_F(TextureManagerTest, TextureUsageExt) {
Should there be a test that SetParameter returns false if the extension is not
enabled?

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager_unittest.cc:128:
TestHelper::SetupTextureManagerInitExpectations(gl_.get(),
"GL_ANGLE_texture_usage");
> 80 cols

[greggman, 2011-12-02 18:33:24]

looks good. Just a few issues. Sorry this is so painful

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/build_gle...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/build_gle...
gpu/command_buffer/build_gles2_cmd_buffer.py:1058: 'GL_RGBA8_OES',
these formats probably need to be added to
src/gpu/command_buffer/common/gles2_cmd_utils.cc 
in ElementsPerGroup and GetChannelsForFormat

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6740: if
(teximage2d_faster_than_texsubimage2d_ && xoffset == 0 && yoffset == 0) {
You'll need to check the texture is not immutable (see below)

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7696: {
This is not chromium style

should be

type function(args) {
  switch (value) {
    case CONST1:
      return value;
    case CONST2:
      return value;
   ...
   }
}

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7722: GLenum
ExtractTypeFromStorageFormat(GLenum internalformat)
style issues same as above. Should be static?

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7749: error::Error
GLES2DecoderImpl::DoTexStorage2DEXT(
There's no reason for this to return an error. should be void.

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7756: feature_info_, target, 1,
width, height, 1) ||
the 1 after target should be 0

Also, you probably need to check that levels is correct for the given width and
height

There's a function, ComputeMipMapCount, in TextureManger.cc you could expose as
a static function on TextureManager

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7777:
texture_manager()->SetLevelInfo(
I think you're going to need to add a field 'immutable' to
TextureManager::TextureInfo and set it here

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7783:
texture_manager()->MarkMipmapsGenerated(feature_info_, info);
This should be inside the if. 

You should also consider changing MarkMipmapsGenerated so it takes another bool
arg "cleared"

Pass that value as the last argument to SetLevelInfo (I'll put a comment over
there)

Here you'd pass false. In DoGenerateMipmap you'd pass true.

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager.cc (right):

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:114: const FeatureInfo*
feature_info) {
add 'bool cleared'?

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:140: true);
change this true to 'cleared'

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager_unittest.cc (right):

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager_unittest.cc:127:
TEST_F(TextureManagerTest, TextureUsageExt) {
Should there be a test that SetParameter returns false if the extension is not
enabled?

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager_unittest.cc:128:
TestHelper::SetupTextureManagerInitExpectations(gl_.get(),
"GL_ANGLE_texture_usage");
> 80 cols

[vangelis, 2011-12-03 00:19:53]

Thanks for the review, Gregg.  PTAL.

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/build_gle...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/build_gle...
gpu/command_buffer/build_gles2_cmd_buffer.py:1058: 'GL_RGBA8_OES',
On 2011/12/02 18:33:24, greggman wrote:
> these formats probably need to be added to
> src/gpu/command_buffer/common/gles2_cmd_utils.cc 
> in ElementsPerGroup and GetChannelsForFormat

These enums are only valid when supplying them to glTexStorage2D. They combine
the format and type parameter into one.  The actual internalformat of the
texture allocated is still one of the traditional GL formats. For example, using
GL_RGB565 when allocating with glTexStorage2D will result in a texture with
format = GL_RGB and type = GL_UNSIGNED_SHORT_5_6_6.

It looks like ElementsPerGroup and GetChannelsForFormat only get called with
valid texture formats. 

Am I reading this wrong?

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6740: if
(teximage2d_faster_than_texsubimage2d_ && xoffset == 0 && yoffset == 0) {
On 2011/12/02 18:33:24, greggman wrote:
> You'll need to check the texture is not immutable (see below)

Done.

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7696: {
On 2011/12/02 18:33:24, greggman wrote:
> This is not chromium style
> 
> should be
> 
> type function(args) {
>   switch (value) {
>     case CONST1:
>       return value;
>     case CONST2:
>       return value;
>    ...
>    }
> }

Done.

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7722: GLenum
ExtractTypeFromStorageFormat(GLenum internalformat)
On 2011/12/02 18:33:24, greggman wrote:
> style issues same as above. Should be static?

Done and Done

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7749: error::Error
GLES2DecoderImpl::DoTexStorage2DEXT(
On 2011/12/02 18:33:24, greggman wrote:
> There's no reason for this to return an error. should be void.
> 

Done.

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7756: feature_info_, target, 1,
width, height, 1) ||
On 2011/12/02 18:33:24, greggman wrote:
> the 1 after target should be 0
> 
> Also, you probably need to check that levels is correct for the given width
and
> height
> 
> There's a function, ComputeMipMapCount, in TextureManger.cc you could expose
as
> a static function on TextureManager

Done.

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7777:
texture_manager()->SetLevelInfo(
On 2011/12/02 18:33:24, greggman wrote:
> I think you're going to need to add a field 'immutable' to
> TextureManager::TextureInfo and set it here

Good point.  Done.  I also added checks in the handlers for TexImage,
CompressedTexImage and CopyTexImage which according to the spec must return
INVALID_OPERATION if the texture is marked as immutable.

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7783:
texture_manager()->MarkMipmapsGenerated(feature_info_, info);
On 2011/12/02 18:33:24, greggman wrote:
> This should be inside the if. 
> 
> You should also consider changing MarkMipmapsGenerated so it takes another
bool
> arg "cleared"
> 
> Pass that value as the last argument to SetLevelInfo (I'll put a comment over
> there)
> 
> Here you'd pass false. In DoGenerateMipmap you'd pass true.
> 
> 

Done.

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager.cc (right):

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:114: const FeatureInfo*
feature_info) {
On 2011/12/02 18:33:24, greggman wrote:
> add 'bool cleared'?

Done.

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:140: true);
On 2011/12/02 18:33:24, greggman wrote:
> change this true to 'cleared'

Done.

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager_unittest.cc (right):

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager_unittest.cc:127:
TEST_F(TextureManagerTest, TextureUsageExt) {
On 2011/12/02 18:33:24, greggman wrote:
> Should there be a test that SetParameter returns false if the extension is not
> enabled?

Added a GLES2DecoderTest for it.

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager_unittest.cc:128:
TestHelper::SetupTextureManagerInitExpectations(gl_.get(),
"GL_ANGLE_texture_usage");
On 2011/12/02 18:33:24, greggman wrote:
> > 80 cols

Done.

[greggman, 2011-12-03 01:41:01]

lgtm

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/build_gle...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/build_gle...
gpu/command_buffer/build_gles2_cmd_buffer.py:1058: 'GL_RGBA8_OES',
On 2011/12/03 00:19:53, vangelis wrote:
> On 2011/12/02 18:33:24, greggman wrote:
> > these formats probably need to be added to
> > src/gpu/command_buffer/common/gles2_cmd_utils.cc 
> > in ElementsPerGroup and GetChannelsForFormat
> 
> These enums are only valid when supplying them to glTexStorage2D. They combine
> the format and type parameter into one.  The actual internalformat of the
> texture allocated is still one of the traditional GL formats. For example,
using
> GL_RGB565 when allocating with glTexStorage2D will result in a texture with
> format = GL_RGB and type = GL_UNSIGNED_SHORT_5_6_6.
> 
> It looks like ElementsPerGroup and GetChannelsForFormat only get called with
> valid texture formats. 
> 
> Am I reading this wrong?

I see. Hmm, well, I guess it's fine. That seems to go against OpenGL ES 2.0 in
that there are no conversions and the formats have to be the same. I feel like
you should pass in type/format to TexStorage so you can know to use the same
type/format in TexSubImage2D but okay I guess.

http://codereview.chromium.org/8772033/diff/8001/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/8772033/diff/8001/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7713: switch (internalformat) {
this function and the next have too many spaces. delete 2 spaces from each line.

http://codereview.chromium.org/8772033/diff/8001/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager.h (right):

http://codereview.chromium.org/8772033/diff/8001/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.h:179: immutable_ = immutable;
DCHECK(!immutable_) ?

[vangelis, 2011-12-05 16:46:05]

On 2011/12/03 01:41:01, greggman wrote:
> lgtm
> 
>
http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/build_gle...
> File gpu/command_buffer/build_gles2_cmd_buffer.py (right):
> 
>
http://codereview.chromium.org/8772033/diff/3051/gpu/command_buffer/build_gle...
> gpu/command_buffer/build_gles2_cmd_buffer.py:1058: 'GL_RGBA8_OES',
> On 2011/12/03 00:19:53, vangelis wrote:
> > On 2011/12/02 18:33:24, greggman wrote:
> > > these formats probably need to be added to
> > > src/gpu/command_buffer/common/gles2_cmd_utils.cc 
> > > in ElementsPerGroup and GetChannelsForFormat
> > 
> > These enums are only valid when supplying them to glTexStorage2D. They
combine
> > the format and type parameter into one.  The actual internalformat of the
> > texture allocated is still one of the traditional GL formats. For example,
> using
> > GL_RGB565 when allocating with glTexStorage2D will result in a texture with
> > format = GL_RGB and type = GL_UNSIGNED_SHORT_5_6_6.
> > 
> > It looks like ElementsPerGroup and GetChannelsForFormat only get called with
> > valid texture formats. 
> > 
> > Am I reading this wrong?
> 
> I see. Hmm, well, I guess it's fine. That seems to go against OpenGL ES 2.0 in
> that there are no conversions and the formats have to be the same. I feel like
> you should pass in type/format to TexStorage so you can know to use the same
> type/format in TexSubImage2D but okay I guess.
> 

I agree with you.  Passing a standard type/format would have been much cleaner. 
But unfortunately that's how the extension was written.

[Vangelis Kokkevis, 2011-12-05 17:26:26]

Ken, Ben, can I have an OWNER's lgtm ?

[Ken Russell (switch to Gerrit), 2011-12-05 19:52:28]

Sorry for the delay -- content/gpu/ and webkit/gpu/ LGTM.