Implement support for rendering to 32-bit float textures on ES3

Implement support for rendering to 32-bit float textures on ES3

On ES2 with OES_texture_float/OES_texture_half_float support, it is only possible to render to 16-bit half float textures using EXT_color_buffer_half_float. There's no support for rendering to 32-bit float textures in ES2 extensions. On ES3, rendering to some 32-bit float texture formats is exposed with EXT_color_buffer_float, but one must use the sized internal formats specified in ES3 core to do that. To expose this, a new command buffer extension is added which enables clients to directly use the sized internal format GL_RGBA32F.

A similar extension is also added to expose GL_RGB32F on desktop GL platforms for the sake of consistency.

These extensions are available whenever rendering to float textures is available. To support the current version of ANGLE, format conversions back to unsized internal formats are added to ui/gl.

Tests are added to cover this functionality. The new tests also add coverage for the handling of 32-bit float formats on ES2 and on desktop GL that existed before this patch.

BUG=329605
TEST=gpu_unittests, WebGL conformance tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251329

[oetuaho-nv, 2014-01-20 17:49:39]

Most relevant WebGL conformance test:
https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/oes-t...

I've tested this on Tegra K1, where the conformance test reported that rendering
to float texture succeeded. Also verified that it works with some content out in
the wild.

[no sievers, 2014-01-21 22:21:12]

https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_gl_api_implementati...
File ui/gl/gl_gl_api_implementation.cc (right):

https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_gl_api_implementati...
ui/gl/gl_gl_api_implementation.cc:49: // Use sized internal formats from core
ES3 to get support for rendering
Why is the format being renderable relevant in this function?

https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_gl_api_implementati...
ui/gl/gl_gl_api_implementation.cc:65: // GL_HALF_FLOAT_OES textures can be
renderable with
same here

https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_gl_api_implementati...
ui/gl/gl_gl_api_implementation.cc:68: DCHECK(type != GL_HALF_FLOAT);
Does this compile? Isn't it GL_HALF_FLOAT_OES?

https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_gl_api_implementati...
ui/gl/gl_gl_api_implementation.cc:68: DCHECK(type != GL_HALF_FLOAT);
Don't we need to allow this? Looking at feature_info.cc we seem to allow and
expose GL_OES_texture_half_float on GLES.

Wouldn't we just want to pass it through and it would choose its own internal
format?

Note that the validators would take care of failing TexImage2D() if the
extension was *not* supported.

https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_gl_api_implementati...
ui/gl/gl_gl_api_implementation.cc:232:
base::AtExitManager::RegisterCallback(CleanupVersionInfo, NULL);
Hmm, are you doing this because we are actually not calling ClearGLBindingsGL()?
Maybe we should rather fix that then (separately). Seems like we'd already be
leaking other stuff.

[piman, 2014-01-21 22:37:12]

https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_gl_api_implementati...
File ui/gl/gl_gl_api_implementation.cc (right):

https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_gl_api_implementati...
ui/gl/gl_gl_api_implementation.cc:48: if (gfx::g_version_info->is_es3) {
I think we want to limit the amount of magic that happens at this level.

We expose GLES2 to the clients. This function is just here to share the code
when using desktop GL vs GLES on the service side (since they have different
requirements wrt internal_format), but I don't think we want to go beyond that.

I think what we want is expose extensions to the client so that it can choose to
allocate textures with (say) a RGBA32F internal_format if the extension is
present, and pass it through here (modulo differences between GLES vs GL).
GLES2Decoder would do the validation.

Otherwise, the same code (that'd do render-to-float-texture) might work on one
implementation and not another one, without being able to tell them apart.

[no sievers, 2014-01-21 22:59:11]

Actually, I'm getting a bit more confused now since your patch seems to mostly
handle the case where 'is_es3 AND OES_texture_float is available'.

The v3.0.3 spec seems to suggest that since a combination of type == GL_FLOAT
and internal format being specific by the app as a base format (GL_RGBA/GL_RGB)
is not listed in table 3.12, one is probably better off in specifying a sized
internal_format of RGBA32F. Which is I guess what your patch does.

However, wouldn't the availability of the texture_float extension suggest that
you could still use an unsized internal_format of GL_RGBA?

Maybe we need to agree on what should happen for the given permutaions of
extensions availability and is_es2, is_es3 respectively. Also how is half_float
different from float?

Also, I'm wondering if we should advertise OES_texture_float on es3 drivers so
that it can be used from WebGL (getting converted to a sized internal float
format). wdyt?

[oetuaho-nv, 2014-01-22 09:45:43]

On 2014/01/21 22:59:11, sievers wrote:
> Actually, I'm getting a bit more confused now since your patch seems to mostly
> handle the case where 'is_es3 AND OES_texture_float is available'.
> 
> The v3.0.3 spec seems to suggest that since a combination of type == GL_FLOAT
> and internal format being specific by the app as a base format
(GL_RGBA/GL_RGB)
> is not listed in table 3.12, one is probably better off in specifying a sized
> internal_format of RGBA32F. Which is I guess what your patch does.
> 
> However, wouldn't the availability of the texture_float extension suggest that
> you could still use an unsized internal_format of GL_RGBA?
> 
> Maybe we need to agree on what should happen for the given permutaions of
> extensions availability and is_es2, is_es3 respectively. Also how is
half_float
> different from float?
> 
> Also, I'm wondering if we should advertise OES_texture_float on es3 drivers so
> that it can be used from WebGL (getting converted to a sized internal float
> format). wdyt?

I agree that this is confusing. Let's go through the interesting ES2/ES3
permutations with respect to 32-bit float textures:

ES2 with OES_texture_float: We should use the unsized internal formats defined
in OES_texture_float. Float textures are not renderable.

ES2 with OES_texture_float and EXT_color_buffer_float: Not possible, since
EXT_color_buffer_float requires ES3.

ES3: A native app can use the sized internal formats defined in ES3 core for
creating float textures. But WebGL can't expose OES_texture_float, since ES3
core doesn't contain float LUMINANCE/ALPHA formats. If we'd like to support
RGB/RGBA float textures on this configuration, we'd need to create a new WebGL
extension that drops the LUMINANCE/ALPHA formats.

ES3 with OES_texture_float: WebGL can expose OES_texture_float and can use the
unsized internal formats for float textures. Using sized internal formats for
RGB/RGBA float textures is possible, but doesn't have any benefit. Float
textures are not renderable.

ES3 with OES_texture_float and EXT_color_buffer_float: WebGL can use the same
internal formats as in the previous option, but using sized internal formats for
RGB/RGBA float textures has the benefit of them being renderable.

As for half-float, EXT_color_buffer_half_float is supported starting from ES2
(unlike EXT_color_buffer_float), so we can simply use that and the unsized
internal formats no matter which ES version we have.

[oetuaho-nv, 2014-01-22 09:46:53]

https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_gl_api_implementati...
File ui/gl/gl_gl_api_implementation.cc (right):

https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_gl_api_implementati...
ui/gl/gl_gl_api_implementation.cc:48: if (gfx::g_version_info->is_es3) {
On 2014/01/21 22:37:12, piman wrote:
> I think we want to limit the amount of magic that happens at this level.
> 
> We expose GLES2 to the clients. This function is just here to share the code
> when using desktop GL vs GLES on the service side (since they have different
> requirements wrt internal_format), but I don't think we want to go beyond
that.
> 
> I think what we want is expose extensions to the client so that it can choose
to
> allocate textures with (say) a RGBA32F internal_format if the extension is
> present, and pass it through here (modulo differences between GLES vs GL).
> GLES2Decoder would do the validation.
> 
> Otherwise, the same code (that'd do render-to-float-texture) might work on one
> implementation and not another one, without being able to tell them apart.

Traditionally in OpenGL, the mechanism for the client to determine whether a
format is renderable is to check for framebuffer completeness. And whether a
given framebuffer is complete is subject to implementation-dependent set of
restrictions, see GLES2 spec section 4.4.5. I don't think that we should change
this to an extension mechanism in WebGL. It seems to go against the design
philosophy that developers can transfer knowledge of OpenGL to WebGL with as few
difficulties as possible, and the way the spec is worded, we might need a lot of
extensions for different formats, not just for float textures.

Also, having to use different enums based on extension support sounds like
needlessly exposing complexity to the client. I think we should treat mobile
APIs equally to desktop GL and shouldn't make supporting them an extra hassle
for content developers.

https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_gl_api_implementati...
ui/gl/gl_gl_api_implementation.cc:49: // Use sized internal formats from core
ES3 to get support for rendering
On 2014/01/21 22:21:13, sievers wrote:
> Why is the format being renderable relevant in this function?

When creating textures, it's better to choose a renderable format in case the
client later decides to attach it to a framebuffer. I don't see any reasonable
alternatives, changing the format when the framebuffer attachment happens would
require costly copying. I can add a comment about this.

https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_gl_api_implementati...
ui/gl/gl_gl_api_implementation.cc:68: DCHECK(type != GL_HALF_FLOAT);
On 2014/01/21 22:21:13, sievers wrote:
> Does this compile? Isn't it GL_HALF_FLOAT_OES?

Both of these enums exist, and have different values. Right now the higher-level
code is exclusively using GL_HALF_FLOAT_OES for textures. This check is here to
ensure that the higher-level code doesn't switch to the other enum for whatever
reason without making the requisite changes here as well. I'll clarify the
comment.

https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_gl_api_implementati...
ui/gl/gl_gl_api_implementation.cc:232:
base::AtExitManager::RegisterCallback(CleanupVersionInfo, NULL);
On 2014/01/21 22:21:13, sievers wrote:
> Hmm, are you doing this because we are actually not calling
ClearGLBindingsGL()?
> Maybe we should rather fix that then (separately). Seems like we'd already be
> leaking other stuff.

Ah, I should have realized from the code that putting this to ClearGLBindingsGL
should be sufficient. I'll remove this. I'm not aware of any bugs related to
this.

[piman, 2014-01-22 21:17:57]

On Wed, Jan 22, 2014 at 1:46 AM, <oetuaho@nvidia.com> wrote:

>
> https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_
> gl_api_implementation.cc
> File ui/gl/gl_gl_api_implementation.cc (right):
>
> https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_
> gl_api_implementation.cc#newcode48
> ui/gl/gl_gl_api_implementation.cc:48: if (gfx::g_version_info->is_es3) {
> On 2014/01/21 22:37:12, piman wrote:
>
>> I think we want to limit the amount of magic that happens at this
>>
> level.
>
>  We expose GLES2 to the clients. This function is just here to share
>>
> the code
>
>> when using desktop GL vs GLES on the service side (since they have
>>
> different
>
>> requirements wrt internal_format), but I don't think we want to go
>>
> beyond that.
>
>  I think what we want is expose extensions to the client so that it can
>>
> choose to
>
>> allocate textures with (say) a RGBA32F internal_format if the
>>
> extension is
>
>> present, and pass it through here (modulo differences between GLES vs
>>
> GL).
>
>> GLES2Decoder would do the validation.
>>
>
>  Otherwise, the same code (that'd do render-to-float-texture) might
>>
> work on one
>
>> implementation and not another one, without being able to tell them
>>
> apart.
>
> Traditionally in OpenGL, the mechanism for the client to determine
> whether a format is renderable is to check for framebuffer completeness.
> And whether a given framebuffer is complete is subject to
> implementation-dependent set of restrictions, see GLES2 spec section
> 4.4.5. I don't think that we should change this to an extension
> mechanism in WebGL. It seems to go against the design philosophy that
> developers can transfer knowledge of OpenGL to WebGL with as few
> difficulties as possible, and the way the spec is worded, we might need
> a lot of extensions for different formats, not just for float textures.
>

I disagree. Round trips to the service side (to check for FBO completeness)
are very expensive. So we don't want to force the client side (whether
webgl or pepper or native code in the renderer be it skia or compositor) to
make those.
We always try hard to not have code on the service side fail without a way
to tell ahead of time. It's not GL-y.


> Also, having to use different enums based on extension support sounds
> like needlessly exposing complexity to the client. I think we should
> treat mobile APIs equally to desktop GL and shouldn't make supporting
> them an extra hassle for content developers.


It's not about desktop vs mobile. On the contrary. What we expose on the
client side is the ES2 API + extensions (eventually ES3+extensions). In
this case, we're talking about adding support for OES_texture_float +
EXT_color_buffer_float
on the client side. We should do that when it's available, i.e. either on
ES3 backend with the extension present, or with a desktop GL backend.

https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_
> gl_api_implementation.cc#newcode49
> ui/gl/gl_gl_api_implementation.cc:49: // Use sized internal formats from
> core ES3 to get support for rendering
> On 2014/01/21 22:21:13, sievers wrote:
>
>> Why is the format being renderable relevant in this function?
>>
>
> When creating textures, it's better to choose a renderable format in
> case the client later decides to attach it to a framebuffer. I don't see
> any reasonable alternatives, changing the format when the framebuffer
> attachment happens would require costly copying. I can add a comment
> about this.
>
>
> https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_
> gl_api_implementation.cc#newcode68
> ui/gl/gl_gl_api_implementation.cc:68: DCHECK(type != GL_HALF_FLOAT);
> On 2014/01/21 22:21:13, sievers wrote:
>
>> Does this compile? Isn't it GL_HALF_FLOAT_OES?
>>
>
> Both of these enums exist, and have different values. Right now the
> higher-level code is exclusively using GL_HALF_FLOAT_OES for textures.
> This check is here to ensure that the higher-level code doesn't switch
> to the other enum for whatever reason without making the requisite
> changes here as well. I'll clarify the comment.
>
>
> https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_
> gl_api_implementation.cc#newcode232
> ui/gl/gl_gl_api_implementation.cc:232:
> base::AtExitManager::RegisterCallback(CleanupVersionInfo, NULL);
> On 2014/01/21 22:21:13, sievers wrote:
>
>> Hmm, are you doing this because we are actually not calling
>>
> ClearGLBindingsGL()?
>
>> Maybe we should rather fix that then (separately). Seems like we'd
>>
> already be
>
>> leaking other stuff.
>>
>
> Ah, I should have realized from the code that putting this to
> ClearGLBindingsGL should be sufficient. I'll remove this. I'm not aware
> of any bugs related to this.
>
> https://codereview.chromium.org/139013008/
>

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

[oetuaho-nv, 2014-01-23 13:19:58]

On 2014/01/22 21:17:57, piman wrote:
> On Wed, Jan 22, 2014 at 1:46 AM, <mailto:oetuaho@nvidia.com> wrote:
> 
> >
> > https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_
> > gl_api_implementation.cc
> > File ui/gl/gl_gl_api_implementation.cc (right):
> >
> > https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_
> > gl_api_implementation.cc#newcode48
> > ui/gl/gl_gl_api_implementation.cc:48: if (gfx::g_version_info->is_es3) {
> > On 2014/01/21 22:37:12, piman wrote:
> >
> >> I think we want to limit the amount of magic that happens at this
> >>
> > level.
> >
> >  We expose GLES2 to the clients. This function is just here to share
> >>
> > the code
> >
> >> when using desktop GL vs GLES on the service side (since they have
> >>
> > different
> >
> >> requirements wrt internal_format), but I don't think we want to go
> >>
> > beyond that.
> >
> >  I think what we want is expose extensions to the client so that it can
> >>
> > choose to
> >
> >> allocate textures with (say) a RGBA32F internal_format if the
> >>
> > extension is
> >
> >> present, and pass it through here (modulo differences between GLES vs
> >>
> > GL).
> >
> >> GLES2Decoder would do the validation.
> >>
> >
> >  Otherwise, the same code (that'd do render-to-float-texture) might
> >>
> > work on one
> >
> >> implementation and not another one, without being able to tell them
> >>
> > apart.
> >
> > Traditionally in OpenGL, the mechanism for the client to determine
> > whether a format is renderable is to check for framebuffer completeness.
> > And whether a given framebuffer is complete is subject to
> > implementation-dependent set of restrictions, see GLES2 spec section
> > 4.4.5. I don't think that we should change this to an extension
> > mechanism in WebGL. It seems to go against the design philosophy that
> > developers can transfer knowledge of OpenGL to WebGL with as few
> > difficulties as possible, and the way the spec is worded, we might need
> > a lot of extensions for different formats, not just for float textures.
> >
> 
> I disagree. Round trips to the service side (to check for FBO completeness)
> are very expensive. So we don't want to force the client side (whether
> webgl or pepper or native code in the renderer be it skia or compositor) to
> make those.
> We always try hard to not have code on the service side fail without a way
> to tell ahead of time. It's not GL-y.
> 
> > Also, having to use different enums based on extension support sounds
> > like needlessly exposing complexity to the client. I think we should
> > treat mobile APIs equally to desktop GL and shouldn't make supporting
> > them an extra hassle for content developers.
> 
> It's not about desktop vs mobile. On the contrary. What we expose on the
> client side is the ES2 API + extensions (eventually ES3+extensions). In
> this case, we're talking about adding support for OES_texture_float +
> EXT_color_buffer_float
> on the client side. We should do that when it's available, i.e. either on
> ES3 backend with the extension present, or with a desktop GL backend.

I think I might have misunderstood you a little, can you clarify this? Is the
core issue here that you'd rather implement the suggestion in WebGL
OES_texture_float that "new implementations should not implicitly support float
rendering and applications should be modified to explicitly enable
WEBGL_color_buffer_float", and do this across platforms? If this is the case,
I'd be happy to move this forward as well. The transition will be painful before
EXT_color_buffer_float will be universally adopted, with lots of content not
working with specific configurations, but the end result will be better.

Still, I think it's a bit odd to say that something that's part of the GL specs
is not GL-y. Framebuffer format support is subject to an
implementation-dependent set of restrictions in both GLES and desktop OpenGL.
Support is required for most of the formats from OpenGL 3.0 and GLES 3.0
onwards, but the earlier ARB extension and GLES2 only mandate support for one
implementation-chosen format.

So the only universal mechanism in OpenGL to check if a given format is
supported is to create a framebuffer and check its completeness. A client needs
to check the support only once per format it uses, so this won't create a
performance issue in a well-written app. Requiring support for specific formats
in a given configuration is obviously good design, and we can do that in this
case if we go with EXT_color_buffer_float, but I don't think that we need to go
out of our way to make it this way.

[piman, 2014-01-23 21:59:09]

On Thu, Jan 23, 2014 at 5:19 AM, <oetuaho@nvidia.com> wrote:

> On 2014/01/22 21:17:57, piman wrote:
>
>> On Wed, Jan 22, 2014 at 1:46 AM, <mailto:oetuaho@nvidia.com> wrote:
>>
>
>  >
>> > https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_
>> > gl_api_implementation.cc
>> > File ui/gl/gl_gl_api_implementation.cc (right):
>> >
>> > https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_
>>
>> > gl_api_implementation.cc#newcode48
>> > ui/gl/gl_gl_api_implementation.cc:48: if (gfx::g_version_info->is_es3)
>> {
>> > On 2014/01/21 22:37:12, piman wrote:
>> >
>> >> I think we want to limit the amount of magic that happens at this
>> >>
>> > level.
>> >
>> >  We expose GLES2 to the clients. This function is just here to share
>> >>
>> > the code
>> >
>> >> when using desktop GL vs GLES on the service side (since they have
>> >>
>> > different
>> >
>> >> requirements wrt internal_format), but I don't think we want to go
>> >>
>> > beyond that.
>> >
>> >  I think what we want is expose extensions to the client so that it can
>> >>
>> > choose to
>> >
>> >> allocate textures with (say) a RGBA32F internal_format if the
>> >>
>> > extension is
>> >
>> >> present, and pass it through here (modulo differences between GLES vs
>> >>
>> > GL).
>> >
>> >> GLES2Decoder would do the validation.
>> >>
>> >
>> >  Otherwise, the same code (that'd do render-to-float-texture) might
>> >>
>> > work on one
>> >
>> >> implementation and not another one, without being able to tell them
>> >>
>> > apart.
>> >
>> > Traditionally in OpenGL, the mechanism for the client to determine
>> > whether a format is renderable is to check for framebuffer completeness.
>> > And whether a given framebuffer is complete is subject to
>> > implementation-dependent set of restrictions, see GLES2 spec section
>> > 4.4.5. I don't think that we should change this to an extension
>> > mechanism in WebGL. It seems to go against the design philosophy that
>> > developers can transfer knowledge of OpenGL to WebGL with as few
>> > difficulties as possible, and the way the spec is worded, we might need
>> > a lot of extensions for different formats, not just for float textures.
>> >
>>
>
>  I disagree. Round trips to the service side (to check for FBO
>> completeness)
>> are very expensive. So we don't want to force the client side (whether
>> webgl or pepper or native code in the renderer be it skia or compositor)
>> to
>> make those.
>> We always try hard to not have code on the service side fail without a way
>> to tell ahead of time. It's not GL-y.
>>
>
>  > Also, having to use different enums based on extension support sounds
>> > like needlessly exposing complexity to the client. I think we should
>> > treat mobile APIs equally to desktop GL and shouldn't make supporting
>> > them an extra hassle for content developers.
>>
>
>  It's not about desktop vs mobile. On the contrary. What we expose on the
>> client side is the ES2 API + extensions (eventually ES3+extensions). In
>> this case, we're talking about adding support for OES_texture_float +
>> EXT_color_buffer_float
>> on the client side. We should do that when it's available, i.e. either on
>> ES3 backend with the extension present, or with a desktop GL backend.
>>
>
> I think I might have misunderstood you a little, can you clarify this? Is
> the
> core issue here that you'd rather implement the suggestion in WebGL
> OES_texture_float that "new implementations should not implicitly support
> float
> rendering and applications should be modified to explicitly enable
> WEBGL_color_buffer_float", and do this across platforms?


I'm mostly concerned about generically the client-side of the command
buffers (webgl is one, but so is pepper, skia, compositor, etc.).
Talking about webgl specifically, I think it would be a good change to have
this exposed as an extension if it won't work everywhere. Ken may be a
better person to give a definite answer. But it is an option for WebGL to
do something different from ES3, hide this and leave it up to the app to
CheckFramebufferStatus - it just means that this logic (magical renaming of
internal_format) would move to the WebGL implementation
(WebGLRenderingContext) instead of the service side.


> If this is the case,
> I'd be happy to move this forward as well. The transition will be painful
> before
> EXT_color_buffer_float will be universally adopted, with lots of content
> not
> working with specific configurations, but the end result will be better.
>
> Still, I think it's a bit odd to say that something that's part of the GL
> specs
> is not GL-y. Framebuffer format support is subject to an
> implementation-dependent set of restrictions in both GLES and desktop
> OpenGL.
> Support is required for most of the formats from OpenGL 3.0 and GLES 3.0
> onwards, but the earlier ARB extension and GLES2 only mandate support for
> one
> implementation-chosen format.
>
> So the only universal mechanism in OpenGL to check if a given format is
> supported is to create a framebuffer and check its completeness. A client
> needs
> to check the support only once per format it uses, so this won't create a
> performance issue in a well-written app.


Technically, you need to check it for every combination of textures/RB
formats, as well as their origins (EGLImages, etc.).
This is the part that is not GL-y in the spec. It has a very high cost for
deferred architectures such as Chrome, because the cost of checking is
extremely high. The typical GL way is to check support for features ahead
of time based on the extension list (and static values such as max texture
size), and determine the paths you'll take based on that.
So that's why I would like it to be implemented this way for the client
side.


> Requiring support for specific formats
> in a given configuration is obviously good design, and we can do that in
> this
> case if we go with EXT_color_buffer_float, but I don't think that we need
> to go
> out of our way to make it this way.
>

I think we do. This is the common pattern for optional features in Chrome.


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

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

[oetuaho-nv, 2014-01-28 15:27:09]

On 2014/01/23 21:59:09, piman wrote:
> On Thu, Jan 23, 2014 at 5:19 AM, <mailto:oetuaho@nvidia.com> wrote:
> 
> > On 2014/01/22 21:17:57, piman wrote:
> >
> >> On Wed, Jan 22, 2014 at 1:46 AM, <mailto:oetuaho@nvidia.com> wrote:
> >>
> >
> >  >
> >> > https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_
> >> > gl_api_implementation.cc
> >> > File ui/gl/gl_gl_api_implementation.cc (right):
> >> >
> >> > https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_
> >>
> >> > gl_api_implementation.cc#newcode48
> >> > ui/gl/gl_gl_api_implementation.cc:48: if (gfx::g_version_info->is_es3)
> >> {
> >> > On 2014/01/21 22:37:12, piman wrote:
> >> >
> >> >> I think we want to limit the amount of magic that happens at this
> >> >>
> >> > level.
> >> >
> >> >  We expose GLES2 to the clients. This function is just here to share
> >> >>
> >> > the code
> >> >
> >> >> when using desktop GL vs GLES on the service side (since they have
> >> >>
> >> > different
> >> >
> >> >> requirements wrt internal_format), but I don't think we want to go
> >> >>
> >> > beyond that.
> >> >
> >> >  I think what we want is expose extensions to the client so that it can
> >> >>
> >> > choose to
> >> >
> >> >> allocate textures with (say) a RGBA32F internal_format if the
> >> >>
> >> > extension is
> >> >
> >> >> present, and pass it through here (modulo differences between GLES vs
> >> >>
> >> > GL).
> >> >
> >> >> GLES2Decoder would do the validation.
> >> >>
> >> >
> >> >  Otherwise, the same code (that'd do render-to-float-texture) might
> >> >>
> >> > work on one
> >> >
> >> >> implementation and not another one, without being able to tell them
> >> >>
> >> > apart.
> >> >
> >> > Traditionally in OpenGL, the mechanism for the client to determine
> >> > whether a format is renderable is to check for framebuffer completeness.
> >> > And whether a given framebuffer is complete is subject to
> >> > implementation-dependent set of restrictions, see GLES2 spec section
> >> > 4.4.5. I don't think that we should change this to an extension
> >> > mechanism in WebGL. It seems to go against the design philosophy that
> >> > developers can transfer knowledge of OpenGL to WebGL with as few
> >> > difficulties as possible, and the way the spec is worded, we might need
> >> > a lot of extensions for different formats, not just for float textures.
> >> >
> >>
> >
> >  I disagree. Round trips to the service side (to check for FBO
> >> completeness)
> >> are very expensive. So we don't want to force the client side (whether
> >> webgl or pepper or native code in the renderer be it skia or compositor)
> >> to
> >> make those.
> >> We always try hard to not have code on the service side fail without a way
> >> to tell ahead of time. It's not GL-y.
> >>
> >
> >  > Also, having to use different enums based on extension support sounds
> >> > like needlessly exposing complexity to the client. I think we should
> >> > treat mobile APIs equally to desktop GL and shouldn't make supporting
> >> > them an extra hassle for content developers.
> >>
> >
> >  It's not about desktop vs mobile. On the contrary. What we expose on the
> >> client side is the ES2 API + extensions (eventually ES3+extensions). In
> >> this case, we're talking about adding support for OES_texture_float +
> >> EXT_color_buffer_float
> >> on the client side. We should do that when it's available, i.e. either on
> >> ES3 backend with the extension present, or with a desktop GL backend.
> >>
> >
> > I think I might have misunderstood you a little, can you clarify this? Is
> > the
> > core issue here that you'd rather implement the suggestion in WebGL
> > OES_texture_float that "new implementations should not implicitly support
> > float
> > rendering and applications should be modified to explicitly enable
> > WEBGL_color_buffer_float", and do this across platforms?
> 
> 
> I'm mostly concerned about generically the client-side of the command
> buffers (webgl is one, but so is pepper, skia, compositor, etc.).
> Talking about webgl specifically, I think it would be a good change to have
> this exposed as an extension if it won't work everywhere. Ken may be a
> better person to give a definite answer. But it is an option for WebGL to
> do something different from ES3, hide this and leave it up to the app to
> CheckFramebufferStatus - it just means that this logic (magical renaming of
> internal_format) would move to the WebGL implementation
> (WebGLRenderingContext) instead of the service side.

Okay, I understand the concern with the other command buffer clients. Is the
Pepper API also designed to be as close to regular GLES2 as possible (vs. as
close to WebGL as possible to make porting applications between these two
easier)? Either way, it seems we need to put the support behind an extension at
the command buffer level, and the existing implicit support for rendering to
float textures on desktop would also be behind the extension. The extension
could be named CHROMIUM_color_buffer_float, and it could mandate that its
clients use the sized internal formats. Thankfully the sized internal format
enums with different postfixes are defined as the same numbers, so this doesn't
get more messy than that.

As for the WebGL side of the equation, I discussed this with Ken at last week's
WebGL WG meeting, and we agreed that there the rendering support needs to stay
implicit until WebGL 2. So the new command buffer extension would be implicitly
enabled by WebGL when OES_texture_float is enabled, and then WebGL could do the
conversions in case the extension is supported. The conversions behind the
command buffer interface for desktop GL would need stay in place to support
desktop configurations where float textures are supported but rendering to them
is not, and to support LUMINANCE/ALPHA float textures.

> 
> > If this is the case,
> > I'd be happy to move this forward as well. The transition will be painful
> > before
> > EXT_color_buffer_float will be universally adopted, with lots of content
> > not
> > working with specific configurations, but the end result will be better.
> >
> > Still, I think it's a bit odd to say that something that's part of the GL
> > specs
> > is not GL-y. Framebuffer format support is subject to an
> > implementation-dependent set of restrictions in both GLES and desktop
> > OpenGL.
> > Support is required for most of the formats from OpenGL 3.0 and GLES 3.0
> > onwards, but the earlier ARB extension and GLES2 only mandate support for
> > one
> > implementation-chosen format.
> >
> > So the only universal mechanism in OpenGL to check if a given format is
> > supported is to create a framebuffer and check its completeness. A client
> > needs
> > to check the support only once per format it uses, so this won't create a
> > performance issue in a well-written app.
> 
> 
> Technically, you need to check it for every combination of textures/RB
> formats, as well as their origins (EGLImages, etc.).
> This is the part that is not GL-y in the spec. It has a very high cost for
> deferred architectures such as Chrome, because the cost of checking is
> extremely high. The typical GL way is to check support for features ahead
> of time based on the extension list (and static values such as max texture
> size), and determine the paths you'll take based on that.
> So that's why I would like it to be implemented this way for the client
> side.
> 

Right, it is true that the total number of formats is high. We will have to add
the checks for all the formats we put into the spec of the proposed command
buffer extension, but of course they will run much faster since it can all
happen behind the command buffer interface.

> 
> > Requiring support for specific formats
> > in a given configuration is obviously good design, and we can do that in
> > this
> > case if we go with EXT_color_buffer_float, but I don't think that we need
> > to go
> > out of our way to make it this way.
> >
> 
> I think we do. This is the common pattern for optional features in Chrome.
> 
> 
> >
> >
> > https://codereview.chromium.org/139013008/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[oetuaho-nv, 2014-02-03 17:13:34]

Haven't heard back from you for a while, should I proceed with the new
implementation plan outlined in my latest comment?

[piman, 2014-02-04 00:52:03]

On Tue, Jan 28, 2014 at 7:27 AM, <oetuaho@nvidia.com> wrote:

> On 2014/01/23 21:59:09, piman wrote:
>
>  On Thu, Jan 23, 2014 at 5:19 AM, <mailto:oetuaho@nvidia.com> wrote:
>>
>
>  > On 2014/01/22 21:17:57, piman wrote:
>> >
>> >> On Wed, Jan 22, 2014 at 1:46 AM, <mailto:oetuaho@nvidia.com> wrote:
>> >>
>> >
>> >  >
>> >> > https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_
>> >> > gl_api_implementation.cc
>> >> > File ui/gl/gl_gl_api_implementation.cc (right):
>> >> >
>> >> > https://codereview.chromium.org/139013008/diff/1/ui/gl/gl_
>> >>
>> >> > gl_api_implementation.cc#newcode48
>> >> > ui/gl/gl_gl_api_implementation.cc:48: if
>> (gfx::g_version_info->is_es3)
>> >> {
>> >> > On 2014/01/21 22:37:12, piman wrote:
>> >> >
>> >> >> I think we want to limit the amount of magic that happens at this
>> >> >>
>> >> > level.
>> >> >
>> >> >  We expose GLES2 to the clients. This function is just here to share
>> >> >>
>> >> > the code
>> >> >
>> >> >> when using desktop GL vs GLES on the service side (since they have
>> >> >>
>> >> > different
>> >> >
>> >> >> requirements wrt internal_format), but I don't think we want to go
>> >> >>
>> >> > beyond that.
>> >> >
>> >> >  I think what we want is expose extensions to the client so that it
>> can
>> >> >>
>> >> > choose to
>> >> >
>> >> >> allocate textures with (say) a RGBA32F internal_format if the
>> >> >>
>> >> > extension is
>> >> >
>> >> >> present, and pass it through here (modulo differences between GLES
>> vs
>> >> >>
>> >> > GL).
>> >> >
>> >> >> GLES2Decoder would do the validation.
>> >> >>
>> >> >
>> >> >  Otherwise, the same code (that'd do render-to-float-texture) might
>> >> >>
>> >> > work on one
>> >> >
>> >> >> implementation and not another one, without being able to tell them
>> >> >>
>> >> > apart.
>> >> >
>> >> > Traditionally in OpenGL, the mechanism for the client to determine
>> >> > whether a format is renderable is to check for framebuffer
>> completeness.
>> >> > And whether a given framebuffer is complete is subject to
>> >> > implementation-dependent set of restrictions, see GLES2 spec section
>> >> > 4.4.5. I don't think that we should change this to an extension
>> >> > mechanism in WebGL. It seems to go against the design philosophy that
>> >> > developers can transfer knowledge of OpenGL to WebGL with as few
>> >> > difficulties as possible, and the way the spec is worded, we might
>> need
>> >> > a lot of extensions for different formats, not just for float
>> textures.
>> >> >
>> >>
>> >
>> >  I disagree. Round trips to the service side (to check for FBO
>> >> completeness)
>> >> are very expensive. So we don't want to force the client side (whether
>> >> webgl or pepper or native code in the renderer be it skia or
>> compositor)
>> >> to
>> >> make those.
>> >> We always try hard to not have code on the service side fail without a
>> way
>> >> to tell ahead of time. It's not GL-y.
>> >>
>> >
>> >  > Also, having to use different enums based on extension support sounds
>> >> > like needlessly exposing complexity to the client. I think we should
>> >> > treat mobile APIs equally to desktop GL and shouldn't make supporting
>> >> > them an extra hassle for content developers.
>> >>
>> >
>> >  It's not about desktop vs mobile. On the contrary. What we expose on
>> the
>> >> client side is the ES2 API + extensions (eventually ES3+extensions). In
>> >> this case, we're talking about adding support for OES_texture_float +
>> >> EXT_color_buffer_float
>> >> on the client side. We should do that when it's available, i.e. either
>> on
>> >> ES3 backend with the extension present, or with a desktop GL backend.
>> >>
>> >
>> > I think I might have misunderstood you a little, can you clarify this?
>> Is
>> > the
>> > core issue here that you'd rather implement the suggestion in WebGL
>> > OES_texture_float that "new implementations should not implicitly
>> support
>> > float
>> > rendering and applications should be modified to explicitly enable
>> > WEBGL_color_buffer_float", and do this across platforms?
>>
>
>
>  I'm mostly concerned about generically the client-side of the command
>> buffers (webgl is one, but so is pepper, skia, compositor, etc.).
>> Talking about webgl specifically, I think it would be a good change to
>> have
>> this exposed as an extension if it won't work everywhere. Ken may be a
>> better person to give a definite answer. But it is an option for WebGL to
>> do something different from ES3, hide this and leave it up to the app to
>> CheckFramebufferStatus - it just means that this logic (magical renaming
>> of
>> internal_format) would move to the WebGL implementation
>> (WebGLRenderingContext) instead of the service side.
>>
>
> Okay, I understand the concern with the other command buffer clients. Is
> the
> Pepper API also designed to be as close to regular GLES2 as possible (vs.
> as
> close to WebGL as possible to make porting applications between these two
> easier)?


Correct, the goal of Pepper is to be able to port native applications as
easily as possible, hence compatibility with GLES2 is more important than
compatibility with WebGL. We actually can run conformance tests on the
Pepper implementation.
Same is true for e.g. Skia.


> Either way, it seems we need to put the support behind an extension at
> the command buffer level, and the existing implicit support for rendering
> to
> float textures on desktop would also be behind the extension. The extension
> could be named CHROMIUM_color_buffer_float, and it could mandate that its
> clients use the sized internal formats. Thankfully the sized internal
> format
> enums with different postfixes are defined as the same numbers, so this
> doesn't
> get more messy than that.
>

Ok.


> As for the WebGL side of the equation, I discussed this with Ken at last
> week's
> WebGL WG meeting, and we agreed that there the rendering support needs to
> stay
> implicit until WebGL 2. So the new command buffer extension would be
> implicitly
> enabled by WebGL when OES_texture_float is enabled, and then WebGL could
> do the
> conversions in case the extension is supported. The conversions behind the
> command buffer interface for desktop GL would need stay in place to support
> desktop configurations where float textures are supported but rendering to
> them
> is not, and to support LUMINANCE/ALPHA float textures.


You mean the existing code (prior to your CL), right? Or did you think of
something additional to that?


>
>
>
>  > If this is the case,
>> > I'd be happy to move this forward as well. The transition will be
>> painful
>> > before
>> > EXT_color_buffer_float will be universally adopted, with lots of content
>> > not
>> > working with specific configurations, but the end result will be better.
>> >
>> > Still, I think it's a bit odd to say that something that's part of the
>> GL
>> > specs
>> > is not GL-y. Framebuffer format support is subject to an
>> > implementation-dependent set of restrictions in both GLES and desktop
>> > OpenGL.
>> > Support is required for most of the formats from OpenGL 3.0 and GLES 3.0
>> > onwards, but the earlier ARB extension and GLES2 only mandate support
>> for
>> > one
>> > implementation-chosen format.
>> >
>> > So the only universal mechanism in OpenGL to check if a given format is
>> > supported is to create a framebuffer and check its completeness. A
>> client
>> > needs
>> > to check the support only once per format it uses, so this won't create
>> a
>> > performance issue in a well-written app.
>>
>
>
>  Technically, you need to check it for every combination of textures/RB
>> formats, as well as their origins (EGLImages, etc.).
>> This is the part that is not GL-y in the spec. It has a very high cost for
>> deferred architectures such as Chrome, because the cost of checking is
>> extremely high. The typical GL way is to check support for features ahead
>> of time based on the extension list (and static values such as max texture
>> size), and determine the paths you'll take based on that.
>> So that's why I would like it to be implemented this way for the client
>> side.
>>
>
>
> Right, it is true that the total number of formats is high. We will have
> to add
> the checks for all the formats we put into the spec of the proposed command
> buffer extension, but of course they will run much faster since it can all
> happen behind the command buffer interface.
>
>
>
>  > Requiring support for specific formats
>> > in a given configuration is obviously good design, and we can do that in
>> > this
>> > case if we go with EXT_color_buffer_float, but I don't think that we
>> need
>> > to go
>> > out of our way to make it this way.
>> >
>>
>
>  I think we do. This is the common pattern for optional features in Chrome.
>>
>
>
>  >
>> >
>> > https://codereview.chromium.org/139013008/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/139013008/
>

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

[oetuaho-nv, 2014-02-04 16:11:04]

On 2014/02/04 00:52:03, piman wrote:
> 
> Correct, the goal of Pepper is to be able to port native applications as
> easily as possible, hence compatibility with GLES2 is more important than
> compatibility with WebGL. We actually can run conformance tests on the
> Pepper implementation.
> Same is true for e.g. Skia.
> 

Ok, thanks for explaining this.

> 
> > Either way, it seems we need to put the support behind an extension at
> > the command buffer level, and the existing implicit support for rendering
> > to
> > float textures on desktop would also be behind the extension. The extension
> > could be named CHROMIUM_color_buffer_float, and it could mandate that its
> > clients use the sized internal formats. Thankfully the sized internal
> > format
> > enums with different postfixes are defined as the same numbers, so this
> > doesn't
> > get more messy than that.
> >
> 
> Ok.
> 
> 
> > As for the WebGL side of the equation, I discussed this with Ken at last
> > week's
> > WebGL WG meeting, and we agreed that there the rendering support needs to
> > stay
> > implicit until WebGL 2. So the new command buffer extension would be
> > implicitly
> > enabled by WebGL when OES_texture_float is enabled, and then WebGL could
> > do the
> > conversions in case the extension is supported. The conversions behind the
> > command buffer interface for desktop GL would need stay in place to support
> > desktop configurations where float textures are supported but rendering to
> > them
> > is not, and to support LUMINANCE/ALPHA float textures.
> 
> 
> You mean the existing code (prior to your CL), right? Or did you think of
> something additional to that?
> 

Yes, only the conversions that are there prior to my CL would remain on the
service side. The new conversions for WebGL would be added to client side.

[piman, 2014-02-04 19:49:06]

On Tue, Feb 4, 2014 at 8:11 AM, <oetuaho@nvidia.com> wrote:

> On 2014/02/04 00:52:03, piman wrote:
>
>  Correct, the goal of Pepper is to be able to port native applications as
>> easily as possible, hence compatibility with GLES2 is more important than
>> compatibility with WebGL. We actually can run conformance tests on the
>> Pepper implementation.
>> Same is true for e.g. Skia.
>>
>
>
> Ok, thanks for explaining this.
>
>
>
>  > Either way, it seems we need to put the support behind an extension at
>> > the command buffer level, and the existing implicit support for
>> rendering
>> > to
>> > float textures on desktop would also be behind the extension. The
>> extension
>> > could be named CHROMIUM_color_buffer_float, and it could mandate that
>> its
>> > clients use the sized internal formats. Thankfully the sized internal
>> > format
>> > enums with different postfixes are defined as the same numbers, so this
>> > doesn't
>> > get more messy than that.
>> >
>>
>
>  Ok.
>>
>
>
>  > As for the WebGL side of the equation, I discussed this with Ken at last
>> > week's
>> > WebGL WG meeting, and we agreed that there the rendering support needs
>> to
>> > stay
>> > implicit until WebGL 2. So the new command buffer extension would be
>> > implicitly
>> > enabled by WebGL when OES_texture_float is enabled, and then WebGL could
>> > do the
>> > conversions in case the extension is supported. The conversions behind
>> the
>> > command buffer interface for desktop GL would need stay in place to
>> support
>> > desktop configurations where float textures are supported but rendering
>> to
>> > them
>> > is not, and to support LUMINANCE/ALPHA float textures.
>>
>
>
>  You mean the existing code (prior to your CL), right? Or did you think of
>> something additional to that?
>>
>
>
> Yes, only the conversions that are there prior to my CL would remain on the
> service side. The new conversions for WebGL would be added to client side.
>

Great, this sounds good then.


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

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

[Ken Russell (switch to Gerrit), 2014-02-05 01:58:06]

Sorry I'm so late replying to this CL. Apologies if I'm re-hashing anything.

 - It would be ideal if, for the current WebGL 1.0 spec, it could be possible to
render to floating-point textures when running on an ES3 implementation which
also supports EXT_color_buffer_float / half_float.

 - In this case, it would be fine if it were the WebGL implementation's
responsibility to do any enum conversion.

 - In "WebGL 2.0", ES3 semantics will be exposed, and apps will be required to
query for the EXT_color_buffer_float/half_float extensions in order to enable
rendering to FP textures. In this case, no magic enum conversion will be needed,
because it will be legal to specify an internalformat for the texture which is
different from the format.

I haven't had a chance to look at these code changes yet -- will try to do so
tomorrow.

[oetuaho-nv, 2014-02-05 16:26:19]

Uploaded a work-in-progress version of the service side of the new approach now.
The Blink side is going to be fairly simple.

Things still missing:
* Tests
* Ensuring that framebuffers with float texture attachments are complete before
making the extension available (affects OpenGL before version 3.0)

[oetuaho-nv, 2014-02-06 15:52:16]

The new patch set contains fixes and fills in missing functionality from the
previous one.

One issue is still unresolved design-wise: framebuffers are shared between
contexts through the ContextGroup, but extensions enabled through
RequestExtensionCHROMIUM are now context-specific. This means that a framebuffer
that is complete in one context might be incomplete in another, and this
interacts badly with caching framebuffer completeness too. Do I simply move the
extension enabling information for CHROMIUM_color_buffer_float to the context
group to address this, or is there a better alternative? Should
RequestExtensionCHROMIUM spec be changed to mention that enabling the extension
can apply to the whole context group in this case?

[piman, 2014-02-07 00:43:09]

https://codereview.chromium.org/139013008/diff/280001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/139013008/diff/280001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:497: GL_RGB32F_ARB == GL_RGB32F &&
GL_RGB32F_EXT == GL_RGB32F);
nit: you can use COMPILE_ASSERT

https://codereview.chromium.org/139013008/diff/280001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:512: GLfloat* data = new
GLfloat[width * width * 4];
Why not passing NULL to the glTexImage2D below?

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

https://codereview.chromium.org/139013008/diff/280001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1673: bool
chromium_color_buffer_float_explicitly_enabled_;
I don't think we need this. Since we decided that WebGL assumes the extension is
there (but allows it to fail), and the other clients would test for the
extension and only use it if it's there, we shouldn't need to explicitly
EnableFeatureCHROMIUM to use it.

EnableFeatureCHROMIUM is there essentially to change the validation, and really
only for glsl. This doesn't have any glsl impact, so we don't need to do this
dance.

[piman, 2014-02-07 00:43:48]

On 2014/02/06 15:52:16, oetuaho-nv wrote:
> The new patch set contains fixes and fills in missing functionality from the
> previous one.
> 
> One issue is still unresolved design-wise: framebuffers are shared between
> contexts through the ContextGroup, but extensions enabled through
> RequestExtensionCHROMIUM are now context-specific. This means that a
framebuffer
> that is complete in one context might be incomplete in another, and this
> interacts badly with caching framebuffer completeness too. Do I simply move
the
> extension enabling information for CHROMIUM_color_buffer_float to the context
> group to address this, or is there a better alternative? Should
> RequestExtensionCHROMIUM spec be changed to mention that enabling the
extension
> can apply to the whole context group in this case?

As mentioned, we don't need to do the RequestExtensionCHROMIUM dance for this
extension, so it shouldn't be a problem.

[oetuaho-nv, 2014-02-07 09:26:58]

https://codereview.chromium.org/139013008/diff/280001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/139013008/diff/280001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:497: GL_RGB32F_ARB == GL_RGB32F &&
GL_RGB32F_EXT == GL_RGB32F);
On 2014/02/07 00:43:10, piman wrote:
> nit: you can use COMPILE_ASSERT

Done.

https://codereview.chromium.org/139013008/diff/280001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:512: GLfloat* data = new
GLfloat[width * width * 4];
On 2014/02/07 00:43:10, piman wrote:
> Why not passing NULL to the glTexImage2D below?

True, NULL is fine.

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

https://codereview.chromium.org/139013008/diff/280001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1673: bool
chromium_color_buffer_float_explicitly_enabled_;
On 2014/02/07 00:43:10, piman wrote:
> I don't think we need this. Since we decided that WebGL assumes the extension
is
> there (but allows it to fail), and the other clients would test for the
> extension and only use it if it's there, we shouldn't need to explicitly
> EnableFeatureCHROMIUM to use it.
> 
> EnableFeatureCHROMIUM is there essentially to change the validation, and
really
> only for glsl. This doesn't have any glsl impact, so we don't need to do this
> dance.

Okay. I just got the idea from earlier reviews that this should not be enabled
by default, but making it so will make things easier.

[oetuaho-nv, 2014-02-07 15:04:50]

The latest patch set fixes the things pointed out in the last review and adds
some testing.

There was one bump on the road: On ES3, it's actually only possible to expose
rendering to RGBA targets, and not RGB. Contrary to what I thought earlier,
EXT_color_buffer_float doesn't have RGB32F in its list of new color-renderable
formats. So I split the extension into two, one for RGBA and one for RGB.
However, this would have been a good thing to do regardless, since it gives us
compatibility with desktop OpenGL < 3.0 drivers which might expose only one of
these formats as well.

I also uploaded a pull request to the WebGL conformance suite to get a bit more
test coverage for this there: https://github.com/KhronosGroup/WebGL/pull/464

I'll put up the Blink patch early next week.

[piman, 2014-02-07 19:03:24]

LGTM, 1 nit.

https://codereview.chromium.org/139013008/diff/370001/gpu/command_buffer/serv...
File gpu/command_buffer/service/framebuffer_manager.h (right):

https://codereview.chromium.org/139013008/diff/370001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.h:121: bool
allow_float_rgb_color_attachment) const;
nit: Would it make sense to pass the FeatureInfo here? Wondering about future
extensions.

[oetuaho-nv, 2014-02-10 15:12:54]

I added passing const FeatureInfo* to IsPossiblyComplete instead of two
booleans.

Blink change is now here: https://codereview.chromium.org/150083005 . The Blink
change should preferably go in first to avoid disabling rendering to float
before both of the changes are in.

There's still also a bug I just found - since CHROMIUM_color_buffer_float_rgb is
a substring of CHROMIUM_color_buffer_float_rgba, it does not get added to the
extension string in FeatureInfo. I will fix this.

[oetuaho-nv, 2014-02-10 16:46:08]

And the issue in FeatureInfo is now fixed. There were some substantial changes
to the tests in the last few patch sets, so I would appreciate it if you could
take one more look.

[piman, 2014-02-10 23:09:02]

LGTM+1 nit

https://codereview.chromium.org/139013008/diff/780001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/139013008/diff/780001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:782: pos + str.length() <
extensions_.length() &&
nit: indenting looks wrong, continuation should align ('pos' under 'pos' above).
Try 'git cl format'?

https://codereview.chromium.org/139013008/diff/780001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:789: }
/me thinks we should make extension_ a list (or hash_set?) of strings while
we're building it, and finalize it in the end. Not for this CL, though...

[Ken Russell (switch to Gerrit), 2014-02-11 00:37:23]

https://codereview.chromium.org/139013008/diff/780001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/139013008/diff/780001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:523: GL_FLOAT, NULL);
Do you need to also set the GL_TEXTURE_MIN_FILTER to GL_NEAREST? We've found
that some drivers will report an incomplete framebuffer if a texture with the
default min filter is used as the color attachment.

https://codereview.chromium.org/139013008/diff/780001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:789: }
On 2014/02/10 23:09:03, piman wrote:
> /me thinks we should make extension_ a list (or hash_set?) of strings while
> we're building it, and finalize it in the end. Not for this CL, though...

Agreed on both points.

https://codereview.chromium.org/139013008/diff/780001/gpu/command_buffer/serv...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/139013008/diff/780001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.cc:245: return false;
I'm pretty sure this is going to break ANGLE's ability to render to
floating-point textures, since it currently advertises itself as an ES2
implementation but "happens" (deliberately, and in a way that violates specs,
sorry about that) to support rendering to GL_FLOAT/GL_RGBA textures.

Can you confirm this?

Can you think of a way to relax these checks slightly to avoid breaking FP
texture rendering on Windows? In other words, if
GL_CHROMIUM_color_buffer_float_rgb / GL_CHROMIUM_color_buffer_float_rgba are
advertised, then be strict about using a sized internal format when rendering to
FP textures?

[piman, 2014-02-11 00:46:12]

On Mon, Feb 10, 2014 at 4:37 PM, <kbr@chromium.org> wrote:

>
> https://codereview.chromium.org/139013008/diff/780001/gpu/
> command_buffer/service/feature_info.cc
> File gpu/command_buffer/service/feature_info.cc (right):
>
> https://codereview.chromium.org/139013008/diff/780001/gpu/
> command_buffer/service/feature_info.cc#newcode523
> gpu/command_buffer/service/feature_info.cc:523: GL_FLOAT, NULL);
> Do you need to also set the GL_TEXTURE_MIN_FILTER to GL_NEAREST? We've
> found that some drivers will report an incomplete framebuffer if a
> texture with the default min filter is used as the color attachment.
>
> https://codereview.chromium.org/139013008/diff/780001/gpu/
> command_buffer/service/feature_info.cc#newcode789
> gpu/command_buffer/service/feature_info.cc:789: }
>
> On 2014/02/10 23:09:03, piman wrote:
>
>> /me thinks we should make extension_ a list (or hash_set?) of strings
>>
> while
>
>> we're building it, and finalize it in the end. Not for this CL,
>>
> though...
>
> Agreed on both points.
>
> https://codereview.chromium.org/139013008/diff/780001/gpu/
> command_buffer/service/framebuffer_manager.cc
> File gpu/command_buffer/service/framebuffer_manager.cc (right):
>
> https://codereview.chromium.org/139013008/diff/780001/gpu/
> command_buffer/service/framebuffer_manager.cc#newcode245
> gpu/command_buffer/service/framebuffer_manager.cc:245: return false;
> I'm pretty sure this is going to break ANGLE's ability to render to
> floating-point textures, since it currently advertises itself as an ES2
> implementation but "happens" (deliberately, and in a way that violates
> specs, sorry about that) to support rendering to GL_FLOAT/GL_RGBA
> textures.
>
> Can you confirm this?
>
> Can you think of a way to relax these checks slightly to avoid breaking
> FP texture rendering on Windows? In other words, if
> GL_CHROMIUM_color_buffer_float_rgb / GL_CHROMIUM_color_buffer_float_rgba
> are advertised, then be strict about using a sized internal format when
> rendering to FP textures?
>

If ANGLE supports float through a different path (really, it sounds like,
through a different extension that isn't advertised), then we should:
- add a path to support that
- treat ANGLE-has-an-unadvertised-extension-that-we-care-about as a
workaround for a driver bug


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

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

[Ken Russell (switch to Gerrit), 2014-02-11 02:19:48]

On 2014/02/11 00:46:12, piman wrote:
> 
> If ANGLE supports float through a different path (really, it sounds like,
> through a different extension that isn't advertised), then we should:
> - add a path to support that
> - treat ANGLE-has-an-unadvertised-extension-that-we-care-about as a
> workaround for a driver bug

It would be better to have an explicit code path for ANGLE's current FP render
target support. However, a couple of points:
 - Actually, there is ES 2.0 hardware (from NVIDIA) which supports rendering to
FP textures (half-float, at least)
 - As ANGLE is upgraded to ES 3.0, it will fall under Olli's new code path

I'm not sure how much code it's worth adding at this point in order to make the
current behavior explicit.

Olli, would you be willing/able to generalize your patch? I can help if you are
too busy.

[oetuaho-nv, 2014-02-11 12:42:16]

Half-float rendering should not be affected by the patch in any way. It only
concerns 32-bit float rendering.

https://codereview.chromium.org/139013008/diff/780001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/139013008/diff/780001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:523: GL_FLOAT, NULL);
On 2014/02/11 00:37:23, Ken Russell wrote:
> Do you need to also set the GL_TEXTURE_MIN_FILTER to GL_NEAREST? We've found
> that some drivers will report an incomplete framebuffer if a texture with the
> default min filter is used as the color attachment.

Done.

https://codereview.chromium.org/139013008/diff/780001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:782: pos + str.length() <
extensions_.length() &&
On 2014/02/10 23:09:03, piman wrote:
> nit: indenting looks wrong, continuation should align ('pos' under 'pos'
above).
> Try 'git cl format'?

Done.

https://codereview.chromium.org/139013008/diff/780001/gpu/command_buffer/serv...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/139013008/diff/780001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.cc:245: return false;
On 2014/02/11 00:37:23, Ken Russell wrote:
> I'm pretty sure this is going to break ANGLE's ability to render to
> floating-point textures, since it currently advertises itself as an ES2
> implementation but "happens" (deliberately, and in a way that violates specs,
> sorry about that) to support rendering to GL_FLOAT/GL_RGBA textures.
> 
> Can you confirm this?
> 
> Can you think of a way to relax these checks slightly to avoid breaking FP
> texture rendering on Windows? In other words, if
> GL_CHROMIUM_color_buffer_float_rgb / GL_CHROMIUM_color_buffer_float_rgba are
> advertised, then be strict about using a sized internal format when rendering
to
> FP textures?

Right, I should have realized that ANGLE doesn't follow GL specs in any way as
it comes to rendering to float textures. I can change the patch so that when
is_angle in FeatureInfo is true, the unsized internalformats are still accepted
here. To fix this better, support for the sized internalformats could be added
to ANGLE and it could expose an ANGLE_color_buffer_float extension, but I won't
have time for implementing that.

[piman, 2014-02-11 21:28:37]

https://codereview.chromium.org/139013008/diff/1070001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/139013008/diff/1070001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager.cc:242: // by
CHROMIUM_color_buffer_float_rgb(a).
I don't think this is right.
When using ANGLE, we'd still get GL_RGBA32F from the client, and we need to
translate that into GL_RGBA on the service side.

[Ken Russell (switch to Gerrit), 2014-02-12 00:11:00]

Olli, thank you for pushing this through and very sorry again for how long it
took me to get to and finish this review.

Looks great at this point. LGTM.

https://codereview.chromium.org/139013008/diff/1070001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/139013008/diff/1070001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager.cc:242: // by
CHROMIUM_color_buffer_float_rgb(a).
On 2014/02/11 21:28:38, piman wrote:
> I don't think this is right.
> When using ANGLE, we'd still get GL_RGBA32F from the client, and we need to
> translate that into GL_RGBA on the service side.

When running on ANGLE, the command buffer won't expose the
GL_CHROMIUM_color_buffer_float_rgb{a} extension, so the client side won't send
down the sized internal format.

https://codereview.chromium.org/139013008/diff/1070001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_manager_unittest.cc (right):

https://codereview.chromium.org/139013008/diff/1070001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager_unittest.cc:727:
framebuffer_->IsPossiblyComplete(feature_info_.get()));
The code duplication between these tests is unfortunate, but since a lot of
pre-existing tests in this file duplicate code too, I won't hold up this review
on refactoring these tests.

https://codereview.chromium.org/139013008/diff/1070001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.h (right):

https://codereview.chromium.org/139013008/diff/1070001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.h:248: GLenum target,
GLint level, GLenum call_internal_format,
The name "call_internal_format" is a little confusing; it sounds like a boolean
("should I call internal format"?). Maybe "requested_internal_format"?

https://codereview.chromium.org/139013008/diff/1070001/gpu/command_buffer/ser...
File gpu/command_buffer/service/test_helper.cc (right):

https://codereview.chromium.org/139013008/diff/1070001/gpu/command_buffer/ser...
gpu/command_buffer/service/test_helper.cc:306: bool is_es3 =
(l_version.substr(0, 12) == "opengl es 3.");
This will be fragile if ES 4 ever comes out. Would be better to use
gfx::GLVersionInfo, and further, to add queries to it like "is_es3_or_later".

[piman, 2014-02-12 00:13:46]

On Tue, Feb 11, 2014 at 4:11 PM, <kbr@chromium.org> wrote:

> Olli, thank you for pushing this through and very sorry again for how long
> it
> took me to get to and finish this review.
>
> Looks great at this point. LGTM.
>
>
>
>
> https://codereview.chromium.org/139013008/diff/1070001/
> gpu/command_buffer/service/framebuffer_manager.cc
> File gpu/command_buffer/service/framebuffer_manager.cc (right):
>
> https://codereview.chromium.org/139013008/diff/1070001/
> gpu/command_buffer/service/framebuffer_manager.cc#newcode242
> gpu/command_buffer/service/framebuffer_manager.cc:242: // by
> CHROMIUM_color_buffer_float_rgb(a).
> On 2014/02/11 21:28:38, piman wrote:
>
>> I don't think this is right.
>> When using ANGLE, we'd still get GL_RGBA32F from the client, and we
>>
> need to
>
>> translate that into GL_RGBA on the service side.
>>
>
> When running on ANGLE, the command buffer won't expose the
> GL_CHROMIUM_color_buffer_float_rgb{a} extension, so the client side
> won't send down the sized internal format.
>

What about non-WebGL code?


>
> https://codereview.chromium.org/139013008/diff/1070001/
> gpu/command_buffer/service/framebuffer_manager_unittest.cc
> File gpu/command_buffer/service/framebuffer_manager_unittest.cc (right):
>
> https://codereview.chromium.org/139013008/diff/1070001/
> gpu/command_buffer/service/framebuffer_manager_unittest.cc#newcode727
> gpu/command_buffer/service/framebuffer_manager_unittest.cc:727:
> framebuffer_->IsPossiblyComplete(feature_info_.get()));
> The code duplication between these tests is unfortunate, but since a lot
> of pre-existing tests in this file duplicate code too, I won't hold up
> this review on refactoring these tests.
>
> https://codereview.chromium.org/139013008/diff/1070001/
> gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.h
> File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.h
> (right):
>
> https://codereview.chromium.org/139013008/diff/1070001/
> gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.h#newcode248
> gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.h:248: GLenum
> target, GLint level, GLenum call_internal_format,
> The name "call_internal_format" is a little confusing; it sounds like a
> boolean ("should I call internal format"?). Maybe
> "requested_internal_format"?
>
> https://codereview.chromium.org/139013008/diff/1070001/
> gpu/command_buffer/service/test_helper.cc
> File gpu/command_buffer/service/test_helper.cc (right):
>
> https://codereview.chromium.org/139013008/diff/1070001/
> gpu/command_buffer/service/test_helper.cc#newcode306
> gpu/command_buffer/service/test_helper.cc:306: bool is_es3 =
> (l_version.substr(0, 12) == "opengl es 3.");
> This will be fragile if ES 4 ever comes out. Would be better to use
> gfx::GLVersionInfo, and further, to add queries to it like
> "is_es3_or_later".
>
> https://codereview.chromium.org/139013008/
>

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

[Ken Russell (switch to Gerrit), 2014-02-12 01:48:21]

On 2014/02/12 00:13:46, piman wrote:
> On Tue, Feb 11, 2014 at 4:11 PM, <mailto:kbr@chromium.org> wrote:
> > When running on ANGLE, the command buffer won't expose the
> > GL_CHROMIUM_color_buffer_float_rgb{a} extension, so the client side
> > won't send down the sized internal format.
> >
> 
> What about non-WebGL code?

Yes, you're right, there's a possibility of regression there. Sorry I didn't
think about that.

If Pepper 3D code is relying on rendering to FP textures on desktop platforms,
this CL will break it, since it won't be able to render to GL_RGBA/GL_FLOAT
textures any more.

Olli, could you please make your checks more permissive? A few possibilities:

1. Only expose GL_CHROMIUM_color_buffer_float_rgb{a} on ES3, not on desktop
platforms.
2. Only be strict about the internalformat checks on ES3, but treat
GL_RGB{A}/GL_FLOAT textures as renderable when OES_texture_float/half_float are
exposed on non-ES3 backends.
3. Other ideas?

[piman, 2014-02-12 02:53:20]

On Tue, Feb 11, 2014 at 5:48 PM, <kbr@chromium.org> wrote:

> On 2014/02/12 00:13:46, piman wrote:
>
>  On Tue, Feb 11, 2014 at 4:11 PM, <mailto:kbr@chromium.org> wrote:
>> > When running on ANGLE, the command buffer won't expose the
>> > GL_CHROMIUM_color_buffer_float_rgb{a} extension, so the client side
>> > won't send down the sized internal format.
>> >
>>
>
>  What about non-WebGL code?
>>
>
> Yes, you're right, there's a possibility of regression there. Sorry I
> didn't
> think about that.
>
> If Pepper 3D code is relying on rendering to FP textures on desktop
> platforms,
> this CL will break it, since it won't be able to render to GL_RGBA/GL_FLOAT
> textures any more.
>
> Olli, could you please make your checks more permissive? A few
> possibilities:
>
> 1. Only expose GL_CHROMIUM_color_buffer_float_rgb{a} on ES3, not on
> desktop
> platforms.
> 2. Only be strict about the internalformat checks on ES3, but treat
> GL_RGB{A}/GL_FLOAT textures as renderable when
> OES_texture_float/half_float are
> exposed on non-ES3 backends.
> 3. Other ideas?
>

If we need to provide some backwards compat, what we should do is:
- expose GL_CHROMIUM_color_buffer_float_rgb{a} any time it can be supported
(GLES3+extensions, GL2+extensions, angle). Do the RGBA32F->RGBA translation
on desktop and angle. It's the only well-defined & portable path we support
from the client side
- allow GL_RGBA as an internal format, but it will only work if backend
supports it. Only way to test is through CheckFramebufferStatus


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

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

[oetuaho-nv, 2014-02-12 10:51:16]

On 2014/02/12 02:53:20, piman wrote:
> On Tue, Feb 11, 2014 at 5:48 PM, <mailto:kbr@chromium.org> wrote:
> 
> > On 2014/02/12 00:13:46, piman wrote:
> >
> >  On Tue, Feb 11, 2014 at 4:11 PM, <mailto:kbr@chromium.org> wrote:
> >> > When running on ANGLE, the command buffer won't expose the
> >> > GL_CHROMIUM_color_buffer_float_rgb{a} extension, so the client side
> >> > won't send down the sized internal format.
> >> >
> >>
> >
> >  What about non-WebGL code?
> >>
> >
> > Yes, you're right, there's a possibility of regression there. Sorry I
> > didn't
> > think about that.
> >
> > If Pepper 3D code is relying on rendering to FP textures on desktop
> > platforms,
> > this CL will break it, since it won't be able to render to GL_RGBA/GL_FLOAT
> > textures any more.
> >
> > Olli, could you please make your checks more permissive? A few
> > possibilities:
> >
> > 1. Only expose GL_CHROMIUM_color_buffer_float_rgb{a} on ES3, not on
> > desktop
> > platforms.
> > 2. Only be strict about the internalformat checks on ES3, but treat
> > GL_RGB{A}/GL_FLOAT textures as renderable when
> > OES_texture_float/half_float are
> > exposed on non-ES3 backends.
> > 3. Other ideas?
> >
> 
> If we need to provide some backwards compat, what we should do is:
> - expose GL_CHROMIUM_color_buffer_float_rgb{a} any time it can be supported
> (GLES3+extensions, GL2+extensions, angle). Do the RGBA32F->RGBA translation
> on desktop and angle. It's the only well-defined & portable path we support
> from the client side
> - allow GL_RGBA as an internal format, but it will only work if backend
> supports it. Only way to test is through CheckFramebufferStatus
> 
> 
> >
> > https://codereview.chromium.org/139013008/
> >

I made the change, but it would be better if we could make ANGLE directly handle
the RGBA32F/RGB32F formats. Avoiding magic conversions on the command buffer
side was the main reason why this became a 600-line patch instead of a 150-line
patch in the first place. It's a bit of a confusing mess now.

It would be good to add the conversions to the sized formats to the Pepper API
as well, so that the sized formats could be consistently used at command buffer
level, and that rendering to float textures would work consistently between ES3
and desktop for Pepper, but we can leave this kind of improvements as potential
follow-ups.

[oetuaho-nv, 2014-02-12 10:51:43]

https://codereview.chromium.org/139013008/diff/1070001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.h (right):

https://codereview.chromium.org/139013008/diff/1070001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.h:248: GLenum target,
GLint level, GLenum call_internal_format,
On 2014/02/12 00:11:01, Ken Russell wrote:
> The name "call_internal_format" is a little confusing; it sounds like a
boolean
> ("should I call internal format"?). Maybe "requested_internal_format"?

Done.

https://codereview.chromium.org/139013008/diff/1070001/gpu/command_buffer/ser...
File gpu/command_buffer/service/test_helper.cc (right):

https://codereview.chromium.org/139013008/diff/1070001/gpu/command_buffer/ser...
gpu/command_buffer/service/test_helper.cc:306: bool is_es3 =
(l_version.substr(0, 12) == "opengl es 3.");
On 2014/02/12 00:11:01, Ken Russell wrote:
> This will be fragile if ES 4 ever comes out. Would be better to use
> gfx::GLVersionInfo, and further, to add queries to it like "is_es3_or_later".

This matches the ES3 check in feature_info.cc that was there prior to this
patch. But I agree that it would be good to change both.

[piman, 2014-02-12 21:04:07]

https://codereview.chromium.org/139013008/diff/1350001/ui/gl/gl_gl_api_implem...
File ui/gl/gl_gl_api_implementation.cc (right):

https://codereview.chromium.org/139013008/diff/1350001/ui/gl/gl_gl_api_implem...
ui/gl/gl_gl_api_implementation.cc:51: // ANGLE exposing GLES2 API doesn't
support those.
nit: file a bug and reference here, so that we can remove this in the future.

[oetuaho-nv, 2014-02-13 14:03:57]

Added a link to the ANGLE bug. There was also still a fairly significant bug
with TexSubImage2D in patch set #10, which I hadn't caught since I hadn't run
the full WebGL conformance suite on the latest version locally, I unified some
of the validation code between different texture commands to make it work
correctly.

https://codereview.chromium.org/139013008/diff/1690001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/139013008/diff/1690001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8141: target, level, xoffset,
yoffset, width, height, type)) {
Comparing with the texture's format here was redundant, so this change doesn't
break things.

https://codereview.chromium.org/139013008/diff/1690001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8212: GL_UNSIGNED_BYTE)) {
Here the check for the depth/stencil format was even more clearly redundant.

[piman, 2014-02-14 05:22:43]

lgtm

[oetuaho-nv, 2014-02-14 08:21:32]

The CQ bit was checked by oetuaho@nvidia.com

[commit-bot: I haz the power, 2014-02-14 08:22:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oetuaho@nvidia.com/139013008/1690001

[commit-bot: I haz the power, 2014-02-14 15:20:59]

Change committed as 251329

[Ken Russell (switch to Gerrit), 2014-02-15 01:37:42]

Sorry for the delay again, but LGTM too, for what it's worth.

Thank you for making this contribution, and in particular for being so careful
with it.