Fix MSAA on windows

Fix MSAA on windows

MSAA would brake on windows because skia creates a BGRA8 internal format render
buffer, which we didn't support on windows.

We were turning on BGRA8_EXT for render buffers when we had
https://www.opengl.org/registry/specs/EXT/bgra.txt, this is a desktop spec that doesn't mention BGRA8_EXT
for render buffers, but we're assuming that it should support it.

I've added the condition that BGRA8 is supported for ANGLE as well as GL_EXT_bgra, and a couple test
to confirm this.

BUG=460567
Committed: https://crrev.com/02a2e2d381ed5089d7ab613aec9960616739dfe8
Cr-Commit-Position: refs/heads/master@{#317958}

[hendrikw, 2015-02-21 00:15:00]

hendrikw@chromium.org changed reviewers:
+ kbr@chromium.org, zmo@chromium.org

[hendrikw, 2015-02-21 00:15:00]

PTAL, thanks!

[Ken Russell (switch to Gerrit), 2015-02-21 01:20:41]

kbr@chromium.org changed reviewers:
+ bsalomon@chromium.org, geofflang@chromium.org, jmadill@chromium.org,
shannonwoods@chromium.org

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

I've added several other reviewers who can help, but something looks not correct
about this change.

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

https://codereview.chromium.org/947653002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/feature_info.cc:474: if
(extensions.Contains("GL_EXT_texture_format_BGRA8888")) {
Something looks wrong with this change. GL_EXT_texure_format_BGRA8888 is an
OpenGL ES-only extension, meaning that it will probably be exposed on Android
devices and via ANGLE, but not on Linux or Mac OS. This means that this code
path will not be taken on Linux or Mac any more.

I think you need more careful review of this and the dependent code paths.
Sorry, but I'm Blink gardener today and can't study this in depth until Monday.

[hendrikw, 2015-02-21 01:38:02]

On 2015/02/21 01:22:28, Ken Russell wrote:
> I've added several other reviewers who can help, but something looks not
correct
> about this change.
> 
>
https://codereview.chromium.org/947653002/diff/40001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/feature_info.cc (right):
> 
>
https://codereview.chromium.org/947653002/diff/40001/gpu/command_buffer/servi...
> gpu/command_buffer/service/feature_info.cc:474: if
> (extensions.Contains("GL_EXT_texture_format_BGRA8888")) {
> Something looks wrong with this change. GL_EXT_texure_format_BGRA8888 is an
> OpenGL ES-only extension, meaning that it will probably be exposed on Android
> devices and via ANGLE, but not on Linux or Mac OS. This means that this code
> path will not be taken on Linux or Mac any more.
> 
> I think you need more careful review of this and the dependent code paths.
> Sorry, but I'm Blink gardener today and can't study this in depth until
Monday.

That's a good point about Linux and Mac, but I don't yet understand how this
could have worked, https://www.opengl.org/registry/specs/EXT/bgra.txt doesn't
mention BGRA8, but that's what it was enabling... :/   Seems a bit sketchy.

bgra.txt does mention BGRA_EXT, but certainly not for render buffers.  My guess
is that Linux and Mac are using RGBA (I'm inferring from Shannon's comment on my
other review that BGRA may have been used by skia for DX9 support)

That said, I do have some failing bots to sort out before this review can move
forward.

[hendrikw, 2015-02-21 02:00:53]

On 2015/02/21 01:38:02, hendrikw wrote:
> On 2015/02/21 01:22:28, Ken Russell wrote:
> > I've added several other reviewers who can help, but something looks not
> correct
> > about this change.
> > 
> >
>
https://codereview.chromium.org/947653002/diff/40001/gpu/command_buffer/servi...
> > File gpu/command_buffer/service/feature_info.cc (right):
> > 
> >
>
https://codereview.chromium.org/947653002/diff/40001/gpu/command_buffer/servi...
> > gpu/command_buffer/service/feature_info.cc:474: if
> > (extensions.Contains("GL_EXT_texture_format_BGRA8888")) {
> > Something looks wrong with this change. GL_EXT_texure_format_BGRA8888 is an
> > OpenGL ES-only extension, meaning that it will probably be exposed on
Android
> > devices and via ANGLE, but not on Linux or Mac OS. This means that this code
> > path will not be taken on Linux or Mac any more.
> > 
> > I think you need more careful review of this and the dependent code paths.
> > Sorry, but I'm Blink gardener today and can't study this in depth until
> Monday.
> 
> That's a good point about Linux and Mac, but I don't yet understand how this
> could have worked, https://www.opengl.org/registry/specs/EXT/bgra.txt doesn't
> mention BGRA8, but that's what it was enabling... :/   Seems a bit sketchy.
> 
> bgra.txt does mention BGRA_EXT, but certainly not for render buffers.  My
guess
> is that Linux and Mac are using RGBA (I'm inferring from Shannon's comment on
my
> other review that BGRA may have been used by skia for DX9 support)
> 
> That said, I do have some failing bots to sort out before this review can move
> forward.

Go unittests!  Our unit test tests explicitly that
GL_EXT_texture_format_BGRA8888 does not enable GL_BGRA8_EXT on render buffers,
which is weird, given when I read, so I went back to the docs.

https://github.com/ChromiumWebApps/chromium/blob/master/gpu/GLES2/extensions/...
"EXT_texture_format_BGRA8888 adds a BGRA8_EXT as a legal internal texture
format"

ok.

Lets look at EXT_texture_format_BGRA8888, ... no mention of BGRA8_EXT, well,
where does BGRA8_EXT come from,

It's in
https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt. 
So likely CHROMIUM_renderbuffer_format_BGRA8888 is wrong, and should read:
"EXT_texture_storage adds a BGRA8_EXT as a legal internal texture format"

So, following that, I'm changing it to enable when "GL_EXT_texture_storage" is
there.

[Stephen White, 2015-02-24 18:04:22]

Ken: does the new fix make sense to you? IWBN to get this not-crashing on
Windows.

[Jamie Madill, 2015-02-24 18:08:09]

Hendrik: do you need to update the tests? It looks as though some bots are
failing in related gpu_unittests.

[hendrikw, 2015-02-24 18:11:57]

On 2015/02/24 18:08:09, Jamie Madill wrote:
> Hendrik: do you need to update the tests? It looks as though some bots are
> failing in related gpu_unittests.

(was on vacation yesterday)

Yeah, I'll need to update the tests, I'm in the process of doing this, as well
as tweaking the code a bit.

Ken, hold off for a bit :)  Shannon and I spoke a bit on this this morning and
there's a new update coming soon.

[hendrikw, 2015-02-24 18:53:33]

PTAL and make sure I'm deciphering the specs correctly.

This is questionable:
Only EXT_texture_format_BGRA8888 and APPLE_texture_format_BGRA8888 are mentioned
in GL_EXT_texture_storage as adding the BGRA8_EXT format, but we also add this
format for GL_EXT_bgra (enable_texture_format_bgra8888 = true), and I'm
following this same logic for enable_render_buffer_bgra, which might be wrong.

Let me know, thanks!

[hendrikw, 2015-02-24 21:59:38]

Ken pointed out that my last change may result in BGRA8 being turned on for
android devices, I've changed it to that it's now on for EXT_bgra or if we're
ANGLE, and nothing else.

Also added a couple of tests.

PTAL, thanks!

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

Thanks for persevering with this patch and clarifying the behavior. LGTM.

[hendrikw, 2015-02-24 23:11:43]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-02-24 23:13:34]

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

[hendrikw, 2015-02-25 00:21:17]

On 2015/02/24 23:13:34, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/947653002/140001

I still need an owner review :)  (kbr, how are you not an owner! :)

[hendrikw, 2015-02-25 00:21:22]

The CQ bit was unchecked by hendrikw@chromium.org

[Ken Russell (switch to Gerrit), 2015-02-25 00:38:19]

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

[Ken Russell (switch to Gerrit), 2015-02-25 00:38:20]

+vmiura for OWNERS

[vmiura, 2015-02-25 00:44:13]

LGTM with nit.

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

https://codereview.chromium.org/947653002/diff/140001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:474: // Only desktop GL and ANGLE
guarantee that we can allocate a renderbuffer
nit: Could you clarify the comment to say "Only desktop GL extension GL_EXT_bgra
or ANGLE guarantee..."

[hendrikw, 2015-02-25 00:45:51]

done

[hendrikw, 2015-02-25 00:46:07]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-02-25 00:46:08]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
vmiura@chromium.org
Link to the patchset: https://codereview.chromium.org/947653002/#ps160001
(title: "update comment for v")

[commit-bot: I haz the power, 2015-02-25 00:47:07]

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

[commit-bot: I haz the power, 2015-02-25 02:56:21]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-02-25 02:57:27]

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

[Stephen White, 2015-02-25 22:38:42]

w00t! Thank you for pushing this through, Hendrik!