gpu: support immutable texture on Linux Mesa driver and GLES3.

gpu: support immutable texture on Linux Mesa driver and GLES3.

Mesa GL driver supports GL_ARB_texture_storage extensions, not
GL_EXT_texture_storage. GLES3 supports glTexStroage2D by default.

GLES3 doesn't support BGRA format for immutable texture without
GL_APPLE_texture_format_bgra8888 extension. We prefer BGRA support
to immutable texture support, so GLES3 enables immutable texture
only if it supports GL_APPLE_texture_format_bgra8888.

In addition, currently the compositor blindly uses BGRA on glTexStorage2D.
It can cause a potential bug so this CL checks BGRA format support
to use BGRA on glTexStorage2D.

This CL makes Intel GPU (e.g. Intel Chromebook) and GLES3 devices with
GL_APPLE_texture_format_bgra8888 extension take advantage of immutable
texture.

BUG=407034
Committed: https://crrev.com/42485ac75b825f1eb30132fa5341985ade5b1eb5
Cr-Commit-Position: refs/heads/master@{#292175}

[dshwang, 2014-08-25 10:00:27]

Hi I'm try to enable immutable texture again.

https://codereview.chromium.org/471603003/ caused black rect on link_freon
because resource_provider uses glTexStorage2D with GL_BGRA8_EXT format. GLES3
doesn't support BGRA format for immutable texture. So compositor doesn't use
BGRA immutable texture on GLES3. It's why this CL passes is_es3 to compositor.
I'm a bit nervous to make the compositor know gles3. Is it acceptable? WDYT?

[jamesr, 2014-08-25 17:23:13]

The command buffer interface that the compositor uses is GLES2, not 3.  It
sounds like you want to expose some functionality that is GLES3 based on the
service side, no?  Is there a gpu capability bit you can use or create to
represent this?

[dshwang, 2014-08-25 18:07:27]

On 2014/08/25 17:23:13, jamesr wrote:
> The command buffer interface that the compositor uses is GLES2, not 3.  It
> sounds like you want to expose some functionality that is GLES3 based on the
> service side, no?  Is there a gpu capability bit you can use or create to
> represent this?

Currently, chromium compositor uses glTexStorage2D if GL_EXT_texture_storage is
supported.
I want to support glTexStorage2D if gpu driver support GLES3, which gives a
benefit to Android Nexus and ARM chromebook.
However, glTexStorage2D on GLES3 doesn't support GL_BRGA8 as internal format, so
I want to prevent the compositor from using GL_BRGA8 if GLES3.

After reading your comment, it's not good to introduce is_es3 in capabilities.h
I'll introduce a flag hinting only RGBA glTexStorage2D support, instead of
is_es3

Which is better among two options?
1. Change Capabilities.texture_storage to enum { TextureStorageNotSupported,
TextureStorageSupported, TextureStorageSupportedIncludingBGRA }
2. Add new boolean flag; Capabilities.texture_storage_BGRA_internal_format

[piman, 2014-08-25 22:58:31]

On 2014/08/25 18:07:27, dshwang wrote:
> On 2014/08/25 17:23:13, jamesr wrote:
> > The command buffer interface that the compositor uses is GLES2, not 3.  It
> > sounds like you want to expose some functionality that is GLES3 based on the
> > service side, no?  Is there a gpu capability bit you can use or create to
> > represent this?
> 
> Currently, chromium compositor uses glTexStorage2D if GL_EXT_texture_storage
is
> supported.
> I want to support glTexStorage2D if gpu driver support GLES3, which gives a
> benefit to Android Nexus and ARM chromebook.
> However, glTexStorage2D on GLES3 doesn't support GL_BRGA8 as internal format,
so
> I want to prevent the compositor from using GL_BRGA8 if GLES3.
> 
> After reading your comment, it's not good to introduce is_es3 in
capabilities.h
> I'll introduce a flag hinting only RGBA glTexStorage2D support, instead of
> is_es3
> 
> Which is better among two options?
> 1. Change Capabilities.texture_storage to enum { TextureStorageNotSupported,
> TextureStorageSupported, TextureStorageSupportedIncludingBGRA }
> 2. Add new boolean flag; Capabilities.texture_storage_BGRA_internal_format

NAK, see bug.

[dshwang, 2014-08-26 15:17:41]

Now only APPLE_texture_format_BGRA8888 extension allows BGRA8_EXT in ES3's
glTexStorage2D as piman mentioned in
https://code.google.com/p/chromium/issues/detail?id=407034#c5
So only GLSE3 with APPLE_texture_format_BGRA8888 extension can take advantage of
immutable texture.

In addition, there are not GL_EXT_texture_storage unittests, so I added them.
Could you review again?

[dshwang, 2014-08-26 15:45:17]

Patchset #2 (id:20001) has been deleted

[piman, 2014-08-26 19:06:29]

https://codereview.chromium.org/499283002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/499283002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/feature_info.cc:698:
enable_immutable_texture_format_bgra &= is_es3;
nit: modifying this variable is hard to follow. How about simply testing
(enable_immutable_texture_format_bgra && is_es3) in the if below?

https://codereview.chromium.org/499283002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/feature_info.cc:700:
extensions.Contains("GL_ARB_texture_storage") ||
So, GL_ARB_texture_storage doesn't by itself allow GL_BGRA8 to be passed, at
least in GLES. I'm not fully sure about desktop GL, where the rules on internal
formats are different than the ES rules, but GL_BGRA8 isn't anywhere in OpenGL
2.0 spec (even though it does have GL_EXT_bgra folded in).

[dshwang, 2014-08-26 19:24:04]

https://codereview.chromium.org/499283002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/499283002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/feature_info.cc:700:
extensions.Contains("GL_ARB_texture_storage") ||
On 2014/08/26 19:06:29, piman (OOO) wrote:
> So, GL_ARB_texture_storage doesn't by itself allow GL_BGRA8 to be passed, at
> least in GLES. I'm not fully sure about desktop GL, where the rules on
internal
> formats are different than the ES rules, but GL_BGRA8 isn't anywhere in OpenGL
> 2.0 spec (even though it does have GL_EXT_bgra folded in).

Linux mesa GL driver on both desktop and chromebook supports
GL_ARB_texture_storage and works very well with GL_BGRA8 by GL_EXT_bgra

If GL_ARB_texture_storage is in trouble on GLES, GL_EXT_texture_storage is
supposed to be in trouble also although there are not any GLES1/2 devices
supporting GL_EXT_texture_storage yet, because currently chromium is fine.

I'll block all ES devices unless supporting GL_APPLE_texture_format_BGRA8888

[dshwang, 2014-08-26 19:48:47]

Now GLES isn't allowed if GL_APPLE_texture_format_BGRA8888 is not supported.
WDYT?

[piman, 2014-08-26 20:54:31]

https://codereview.chromium.org/499283002/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/feature_info_unittest.cc (right):

https://codereview.chromium.org/499283002/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/feature_info_unittest.cc:438:
EXPECT_THAT(info_->extensions(), Not(HasSubstr("GL_EXT_texture_storage")));
That seems wrong. If we have GL_EXT_texture_storage on the underlying driver, we
can always expose GL_EXT_texture_storage to the client, even if
GL_EXT_texture_format_bgra8888 is not supported.

It's only in the es3 case (*without* GL_EXT_texture_storage) that we can't
expose GL_EXT_texture_storage to the client unless
GL_APPLE_texture_format_bgra8888 is available.

[dshwang, 2014-08-26 21:01:44]

On 2014/08/26 20:54:31, piman (OOO) wrote:
>
https://codereview.chromium.org/499283002/diff/60001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/feature_info_unittest.cc (right):
> 
>
https://codereview.chromium.org/499283002/diff/60001/gpu/command_buffer/servi...
> gpu/command_buffer/service/feature_info_unittest.cc:438:
> EXPECT_THAT(info_->extensions(), Not(HasSubstr("GL_EXT_texture_storage")));
> That seems wrong. If we have GL_EXT_texture_storage on the underlying driver,
we
> can always expose GL_EXT_texture_storage to the client, even if
> GL_EXT_texture_format_bgra8888 is not supported.
> 
> It's only in the es3 case (*without* GL_EXT_texture_storage) that we can't
> expose GL_EXT_texture_storage to the client unless
> GL_APPLE_texture_format_bgra8888 is available.

In theory, desktop is also possible to support glTexStorage2D without BGRA
support.
Although mobile exposes glTexStorage2D only if BGRA is supported, dose desktop
always expose glTexStorage2D, no matter what BGRA?
As I understand, we want to handle GLES like this in order to make the
compositor blindly use glTexStorage2D, no matter what RGBA or BGRA, right?

[piman, 2014-08-26 23:32:57]

On Tue, Aug 26, 2014 at 2:01 PM, <dongseong.hwang@intel.com> wrote:

> On 2014/08/26 20:54:31, piman (OOO) wrote:
>
> https://codereview.chromium.org/499283002/diff/60001/gpu/
> command_buffer/service/feature_info_unittest.cc
>
>> File gpu/command_buffer/service/feature_info_unittest.cc (right):
>>
>
>
> https://codereview.chromium.org/499283002/diff/60001/gpu/
> command_buffer/service/feature_info_unittest.cc#newcode438
>
>> gpu/command_buffer/service/feature_info_unittest.cc:438:
>> EXPECT_THAT(info_->extensions(), Not(HasSubstr("GL_EXT_texture_
>> storage")));
>> That seems wrong. If we have GL_EXT_texture_storage on the underlying
>> driver,
>>
> we
>
>> can always expose GL_EXT_texture_storage to the client, even if
>> GL_EXT_texture_format_bgra8888 is not supported.
>>
>
>  It's only in the es3 case (*without* GL_EXT_texture_storage) that we can't
>> expose GL_EXT_texture_storage to the client unless
>> GL_APPLE_texture_format_bgra8888 is available.
>>
>
> In theory, desktop is also possible to support glTexStorage2D without BGRA
> support.

Although mobile exposes glTexStorage2D only if BGRA is supported, dose
> desktop
> always expose glTexStorage2D, no matter what BGRA?
>

It's not about glTexStorage2D, it's about GL_EXT_texture_storage.

GL_EXT_texture_storage says "*if* GL_EXT_texture_format_bgra8888 is
supported, then glTexStorage2DEXT *MUST* allow GL_BGRA8". We can't
expose GL_EXT_texture_storage to the client if we're not conformant to that
requirement. However, we can expose GL_EXT_texture_storage if we don't
expose GL_EXT_texture_format_bgra8888.

If the driver exposes GL_EXT_texture_storage and no
GL_EXT_texture_format_bgra8888, then it's still ok to
expose GL_EXT_texture_storage to the client, provided we don't
expose GL_EXT_texture_format_bgra8888.

Here's a table that explains all the valid possibilities.
driver -> client
1- ES2 + GL_EXT_texture_storage + (GL_EXT_texture_format_bgra8888 or
GL_APPLE_texture_format_bgra8888) -> GL_EXT_texture_storage +
GL_EXT_texture_format_bgra8888
2- ES2 + GL_EXT_texture_storage (no GL_EXT_texture_format_bgra8888 nor
GL_APPLE_texture_format_bgra8888) -> GL_EXT_texture_storage (and no
GL_EXT_texture_format_bgra8888 - we don't claim to handle GL_BGRA8 in
glTexStorage2DEXT)
3- ES2 + GL_EXT_texture_format_bgra8888 or
GL_APPLE_texture_format_bgra8888 (no GL_EXT_texture_storage)
-> GL_EXT_texture_format_bgra8888
4- ES2 (no GL_EXT_texture_storage, no GL_EXT_texture_format_bgra8888, no
GL_APPLE_texture_format_bgra8888) -> nothing
5- ES3 + GL_EXT_texture_format_bgra8888 -> GL_EXT_texture_format_bgra8888
 (we can't expose GL_EXT_texture_storage because we fail the GL_BGRA8
requirement)
6- ES3 + GL_EXT_texture_storage + GL_EXT_texture_format_bgra8888 ->
GL_EXT_texture_storage + GL_EXT_texture_format_bgra8888  (driver promises
to handle GL_BGRA8 by exposing GL_EXT_texture_storage)
7- ES3 + GL_APPLE_texture_format_bgra8888 -> GL_EXT_texture_storage +
GL_EXT_texture_format_bgra8888 (driver promises to handle GL_BGRA8 by
exposing GL_APPLE_texture_format_bgra8888)
8- ES3 + none of the above -> GL_EXT_texture_storage (and no
GL_EXT_texture_format_bgra8888 - we don't claim to handle GL_BGRA8)

Does it make sense?

As I understand, we want to handle GLES like this in order to make the
> compositor blindly use glTexStorage2D, no matter what RGBA or BGRA, right?
>

There's 2 constraints:
A- we can't expose GL_EXT_texture_storage and
GL_EXT_texture_format_bgra8888 together if we can't handle GL_BGRA8 - per
spec.
B- if because of (A) we have to decide between
between GL_EXT_texture_storage or GL_EXT_texture_format_bgra8888 (case #5
above), then we prefer to expose GL_EXT_texture_format_bgra8888 because the
performance loss of not being able to use BGRA is way more than the
performance loss of not being able to use glTexStorage2DEXT.


>
> https://codereview.chromium.org/499283002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dshwang, 2014-08-27 13:18:04]

I apply your awesome table to feature_info. And I prevent compositor from using
glTexStorage2D with BGRA if GL_EXT_texture_format_bgra8888 is not supported.
Could you review again?

On 2014/08/26 23:32:57, piman (OOO) wrote:
> It's not about glTexStorage2D, it's about GL_EXT_texture_storage.
> 
> GL_EXT_texture_storage says "*if* GL_EXT_texture_format_bgra8888 is
> supported, then glTexStorage2DEXT *MUST* allow GL_BGRA8". We can't
> expose GL_EXT_texture_storage to the client if we're not conformant to that
> requirement. However, we can expose GL_EXT_texture_storage if we don't
> expose GL_EXT_texture_format_bgra8888.
> 
> If the driver exposes GL_EXT_texture_storage and no
> GL_EXT_texture_format_bgra8888, then it's still ok to
> expose GL_EXT_texture_storage to the client, provided we don't
> expose GL_EXT_texture_format_bgra8888.
> 
> Here's a table that explains all the valid possibilities.
> driver -> client
> 1- ES2 + GL_EXT_texture_storage + (GL_EXT_texture_format_bgra8888 or
> GL_APPLE_texture_format_bgra8888) -> GL_EXT_texture_storage +
> GL_EXT_texture_format_bgra8888
> 2- ES2 + GL_EXT_texture_storage (no GL_EXT_texture_format_bgra8888 nor
> GL_APPLE_texture_format_bgra8888) -> GL_EXT_texture_storage (and no
> GL_EXT_texture_format_bgra8888 - we don't claim to handle GL_BGRA8 in
> glTexStorage2DEXT)
> 3- ES2 + GL_EXT_texture_format_bgra8888 or
> GL_APPLE_texture_format_bgra8888 (no GL_EXT_texture_storage)
> -> GL_EXT_texture_format_bgra8888
> 4- ES2 (no GL_EXT_texture_storage, no GL_EXT_texture_format_bgra8888, no
> GL_APPLE_texture_format_bgra8888) -> nothing
> 5- ES3 + GL_EXT_texture_format_bgra8888 -> GL_EXT_texture_format_bgra8888
>  (we can't expose GL_EXT_texture_storage because we fail the GL_BGRA8
> requirement)
> 6- ES3 + GL_EXT_texture_storage + GL_EXT_texture_format_bgra8888 ->
> GL_EXT_texture_storage + GL_EXT_texture_format_bgra8888  (driver promises
> to handle GL_BGRA8 by exposing GL_EXT_texture_storage)
> 7- ES3 + GL_APPLE_texture_format_bgra8888 -> GL_EXT_texture_storage +
> GL_EXT_texture_format_bgra8888 (driver promises to handle GL_BGRA8 by
> exposing GL_APPLE_texture_format_bgra8888)
> 8- ES3 + none of the above -> GL_EXT_texture_storage (and no
> GL_EXT_texture_format_bgra8888 - we don't claim to handle GL_BGRA8)
> 
> Does it make sense?

Thank you for your patient to explain. super make sense!
I applied them and add unittests to check them all.

> 
> As I understand, we want to handle GLES like this in order to make the
> > compositor blindly use glTexStorage2D, no matter what RGBA or BGRA, right?
> >
> 
> There's 2 constraints:
> A- we can't expose GL_EXT_texture_storage and
> GL_EXT_texture_format_bgra8888 together if we can't handle GL_BGRA8 - per
> spec.
> B- if because of (A) we have to decide between
> between GL_EXT_texture_storage or GL_EXT_texture_format_bgra8888 (case #5
> above), then we prefer to expose GL_EXT_texture_format_bgra8888 because the
> performance loss of not being able to use BGRA is way more than the
> performance loss of not being able to use glTexStorage2DEXT.
> 

All right.

[piman, 2014-08-27 16:35:13]

LGTM! Thanks for working through this.

[dshwang, 2014-08-27 16:37:46]

On 2014/08/27 16:35:13, piman (OOO) wrote:
> LGTM! Thanks for working through this.

Thank you for great review!

[dshwang, 2014-08-27 16:37:50]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-08-27 16:38:29]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/499283002/80001

[commit-bot: I haz the power, 2014-08-27 17:18:35]

Committed patchset #4 (id:80001) as 65c832d73cb2270044960948d0b7a2231bc4f5cf

[commit-bot: I haz the power, 2014-09-10 02:52:28]

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