Fix incorrect binding spec for multisampled_render_to_texture APIs

Fix incorrect binding spec for multisampled_render_to_texture APIs

The extensions with implicit vs. explicit resolve are already
treated differently at the cmdbuffer level. The incorrect
specification of glRenderbufferStorageMultisampleEXT
(multisampled_render_to_texture) with
glRenderbufferStorageMultisample (core MSAA with explicit resolve)
causes an incorrect binding (see removed TODO which says the
same thing).

Also remove glRenderbufferStorageMultisampleANGLE alternative with
the core name.

BUG=454816, 438742, 325668
Committed: https://crrev.com/9f26c23299dcd1dcd816162af7068afca1bb8ef0
Cr-Commit-Position: refs/heads/master@{#314422}

[no sievers, 2015-02-03 21:38:16]

sievers@chromium.org changed reviewers:
+ kbr@chromium.org

[no sievers, 2015-02-03 21:39:41]

ptal

https://codereview.chromium.org/887113003/diff/1/ui/gl/gl_bindings_autogen_gl.cc
File ui/gl/gl_bindings_autogen_gl.cc (left):

https://codereview.chromium.org/887113003/diff/1/ui/gl/gl_bindings_autogen_gl...
ui/gl/gl_bindings_autogen_gl.cc:1712:
DCHECK(fn.glRenderbufferStorageMultisampleEXTFn);
The problem was caused by us falling into here on ARM (ES3 +
multisampled_render_to_texture). Then drawing buffer creation failed (was
incomplete when we do CheckFramebufferStatus).

[Ken Russell (switch to Gerrit), 2015-02-03 21:42:50]

Very nice debugging work. LGTM

[no sievers, 2015-02-03 21:43:55]

On 2015/02/03 21:42:50, Ken Russell wrote:
> Very nice debugging work. LGTM

And now we know that splitting up the entry points was actually correct. They
don't seem to be compatible on this driver :)

[Ken Russell (switch to Gerrit), 2015-02-03 21:48:16]

On 2015/02/03 21:43:55, sievers wrote:
> On 2015/02/03 21:42:50, Ken Russell wrote:
> > Very nice debugging work. LGTM
> 
> And now we know that splitting up the entry points was actually correct. They
> don't seem to be compatible on this driver :)

Right. The explicit vs. implicit resolve distinction between some of the
extensions and the core ES 3.0 APIs is confusing.

[no sievers, 2015-02-03 21:59:07]

Can you double-check one thing that confused me a bit, just to make sure I'm not
breaking anything:

See
https://codereview.chromium.org/72173002/diff/340001/gpu/command_buffer/servi...
and GLES2DecoderImpl::RenderbufferStorageMultisampleHelper().

We actually seem to use glRenderbufferStorageMultisampleEXT() for the explicit
resolve also.
So that would be for GL_EXT_framebuffer_multisample.

So that means we use the same binding for render_to_texture and
framebuffer_multisample (but a different one for Angle and core msaa support). I
guess we thought that's fine since we wouldn't expect them to be advertised
simultaneously.

Also, I can probably address the TODO now and alias
GL_EXT_framebuffer_multisample:glRenderbufferStorageMultisampleEXT to
glRenderbufferStorageMultisampleFn (instead of EXT), and similar for ANGLE,
since the bindings are not so brute force anymore), see my comment:


  5023   // TODO(sievers): This could be resolved at the GL binding level, but
the
  5024   // binding process is currently a bit too 'brute force'.
  5025   if (feature_info->feature_flags().is_angle) {


Does that make sense? wdyt? Should I do this now or separately?

[Ken Russell (switch to Gerrit), 2015-02-03 22:21:07]

On 2015/02/03 21:59:07, sievers wrote:
> Can you double-check one thing that confused me a bit, just to make sure I'm
not
> breaking anything:
> 
> See
>
https://codereview.chromium.org/72173002/diff/340001/gpu/command_buffer/servi...
> and GLES2DecoderImpl::RenderbufferStorageMultisampleHelper().
> 
> We actually seem to use glRenderbufferStorageMultisampleEXT() for the explicit
> resolve also.
> So that would be for GL_EXT_framebuffer_multisample.
> 
> So that means we use the same binding for render_to_texture and
> framebuffer_multisample (but a different one for Angle and core msaa support).
I
> guess we thought that's fine since we wouldn't expect them to be advertised
> simultaneously.

You're right, this identically named entry point is defined by two different
extensions. However, the implicit-resolve behavior of
EXT_multisampled_render_to_texture only affects the color buffer, which is
allocated via FramebufferTexture2DMultisampleEXT. Also,
EXT_framebuffer_multisample is desktop-only and
EXT_multisampled_render_to_texture is ES-only, so I don't think there's any
collision in practice.


> Also, I can probably address the TODO now and alias
> GL_EXT_framebuffer_multisample:glRenderbufferStorageMultisampleEXT to
> glRenderbufferStorageMultisampleFn (instead of EXT), and similar for ANGLE,
> since the bindings are not so brute force anymore), see my comment:
> 
> 
>   5023   // TODO(sievers): This could be resolved at the GL binding level, but
> the
>   5024   // binding process is currently a bit too 'brute force'.
>   5025   if (feature_info->feature_flags().is_angle) {
> 
> 
> Does that make sense? wdyt? Should I do this now or separately?

Let's not do that in this patch. I think it's better to be explicit about this,
and the precedence order you've chosen seems fine here.

[no sievers, 2015-02-03 22:21:41]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2015-02-03 22:22:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/887113003/1

[commit-bot: I haz the power, 2015-02-03 22:32:31]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-02-03 22:34:30]

Patchset 1 (id:??) landed as
https://crrev.com/9f26c23299dcd1dcd816162af7068afca1bb8ef0
Cr-Commit-Position: refs/heads/master@{#314422}