Complete disable_gl_rgb_format workaround for Mali-400 GPU

Complete disable_gl_rgb_format workaround for Mali-400 GPU

GL_RGB format is not supported on Mali-400; this change made a start
on a workaround for it:
https://codereview.chromium.org/996163003

However, a number of things were not handled correctly, for example
the alpha channel could be cleared to 0, making content not show up
on screen.

BUG=449150
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

[halliwell, 2016-02-12 20:39:33]

Description was changed from

==========
Complete disable_gl_rgb_format workaround for Mali-400 GPU

GL_RGB format is not supported on Mali-400; this change made a start
on a workaround for it:
https://codereview.chromium.org/996163003

However, a number of things were not handled correctly, for example
the alpha channel could be cleared to 0, making content not show up
on screen.

BUG=449150
==========

to

==========
Complete disable_gl_rgb_format workaround for Mali-400 GPU

GL_RGB format is not supported on Mali-400; this change made a start
on a workaround for it:
https://codereview.chromium.org/996163003

However, a number of things were not handled correctly, for example
the alpha channel could be cleared to 0, making content not show up
on screen.

BUG=449150
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[halliwell, 2016-02-12 20:41:15]

Description was changed from

==========
Complete disable_gl_rgb_format workaround for Mali-400 GPU

GL_RGB format is not supported on Mali-400; this change made a start
on a workaround for it:
https://codereview.chromium.org/996163003

However, a number of things were not handled correctly, for example
the alpha channel could be cleared to 0, making content not show up
on screen.

BUG=449150
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Complete disable_gl_rgb_format workaround for Mali-400 GPU

GL_RGB format is not supported on Mali-400; this change made a start
on a workaround for it:
https://codereview.chromium.org/996163003

However, a number of things were not handled correctly, for example
the alpha channel could be cleared to 0, making content not show up
on screen.

BUG=449150
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[halliwell, 2016-02-12 20:41:15]

halliwell@chromium.org changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, zmo@chromium.org

[halliwell, 2016-02-12 20:43:12]

This is a first pass based on discussion with Brandon this morning.

I'm super tight on time, so if you could do a quick first-pass sanity check to
answer "can I submit this to our downstream branch to work around the bug", that
would be awesome :)

I can spend more time next week on finishing it up properly for actual Chromium
submission.

[Zhenyao Mo, 2016-02-12 21:06:06]

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/framebuffer_manager.cc:475: GLenum
Framebuffer::GetColorAttachmentFormat() const {
Name this GetColorAttachment0Format.

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2761:
state_.emulating_rgb_with_rgba = true;
You should only set this if attrib_parser.alpha_size == 0.  Otherwise
BackBufferHasAlpha() will return false even if we request RGBA.

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2788:
state_.emulating_rgb_with_rgba = true;
Same here.

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6154: glClearColor(0.0f, 0.0f,
0.0f,
This is problematic, because we could have some attachments as RGBA, and others
as RGB but emulated by RGBA.  To do it right, we will need to clear differently.

If you are time limited, for a quick hack, you can set it to 1 if the workaround
is active and the following condition is true. but please add a TODO here for
following up.

[bajones, 2016-02-12 21:07:41]

Looks good, but needs two fixes.

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2759: if
(workarounds().disable_gl_rgb_format) {
This branch and the one below are wrong, because they'll trigger even when an
RGBA buffer was requested, incorrectly marking it as emulated RGB. The condition
should be:

if (offscreen_target_color_format_ == GL_RGB &&
workarounds().disable_gl_rgb_format) { ... }

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2786: if
(workarounds().disable_gl_rgb_format) {
Ditto

[halliwell, 2016-02-12 22:52:56]

Thank you both for the rapid feedback.

Planning to submit PS2 in our local branch for release.

I will follow up next week on the clearing logic for multiple render targets and
also probably want to talk about adding tests for this.

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/framebuffer_manager.cc:475: GLenum
Framebuffer::GetColorAttachmentFormat() const {
On 2016/02/12 21:06:06, Zhenyao Mo wrote:
> Name this GetColorAttachment0Format.

Done.

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2759: if
(workarounds().disable_gl_rgb_format) {
On 2016/02/12 21:07:41, bajones wrote:
> This branch and the one below are wrong, because they'll trigger even when an
> RGBA buffer was requested, incorrectly marking it as emulated RGB. The
condition
> should be:
> 
> if (offscreen_target_color_format_ == GL_RGB &&
> workarounds().disable_gl_rgb_format) { ... }

Doh.  Thank you.

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2761:
state_.emulating_rgb_with_rgba = true;
On 2016/02/12 21:06:06, Zhenyao Mo wrote:
> You should only set this if attrib_parser.alpha_size == 0.  Otherwise
> BackBufferHasAlpha() will return false even if we request RGBA.

And thank you :)

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2786: if
(workarounds().disable_gl_rgb_format) {
On 2016/02/12 21:07:41, bajones wrote:
> Ditto

Done.

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2788:
state_.emulating_rgb_with_rgba = true;
On 2016/02/12 21:06:06, Zhenyao Mo wrote:
> Same here.

Done.

https://codereview.chromium.org/1700433002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6154: glClearColor(0.0f, 0.0f,
0.0f,
On 2016/02/12 21:06:06, Zhenyao Mo wrote:
> This is problematic, because we could have some attachments as RGBA, and
others
> as RGB but emulated by RGBA.  To do it right, we will need to clear
differently.
> 
> If you are time limited, for a quick hack, you can set it to 1 if the
workaround
> is active and the following condition is true. but please add a TODO here for
> following up.

Agreed.  I updated this to check for the workaround.  I will submit in this form
in our downstream branch.

However, there is no time pressure to submit to Chromium, so I'll come back to
this next week and change how clearing is done to handle this case properly.

[Ken Russell (switch to Gerrit), 2016-02-17 19:59:09]

I don't understand how this fix works, since it mainly seems to affect the
behavior of the command buffer's "default" back buffer, which is no longer used
by WebGL. Instead, WebGL's back buffer is managed in Blink's DrawingBuffer
class. Perhaps it's still getting some of its parameters from the default back
buffer via GL queries.

If it works, then the current code basically LGTM, though I defer review to
Brandon and Mo.

[Zhenyao Mo, 2016-02-17 20:51:49]

On 2016/02/17 19:59:09, Ken Russell wrote:
> I don't understand how this fix works, since it mainly seems to affect the
> behavior of the command buffer's "default" back buffer, which is no longer
used
> by WebGL. Instead, WebGL's back buffer is managed in Blink's DrawingBuffer
> class. Perhaps it's still getting some of its parameters from the default back
> buffer via GL queries.
> 
> If it works, then the current code basically LGTM, though I defer review to
> Brandon and Mo.

Yes, most code deals with back buffer.  It is still nice to have, to make this
emulation complete.

The part that fixed the bug is line 6156 - 6163 in gles2_cmd_decoder.cc

[Ken Russell (switch to Gerrit), 2016-02-17 22:33:33]

On 2016/02/17 20:51:49, Zhenyao Mo wrote:
> On 2016/02/17 19:59:09, Ken Russell wrote:
> > I don't understand how this fix works, since it mainly seems to affect the
> > behavior of the command buffer's "default" back buffer, which is no longer
> used
> > by WebGL. Instead, WebGL's back buffer is managed in Blink's DrawingBuffer
> > class. Perhaps it's still getting some of its parameters from the default
back
> > buffer via GL queries.
> > 
> > If it works, then the current code basically LGTM, though I defer review to
> > Brandon and Mo.
> 
> Yes, most code deals with back buffer.  It is still nice to have, to make this
> emulation complete.
> 
> The part that fixed the bug is line 6156 - 6163 in gles2_cmd_decoder.cc

Got it. Thanks.

[halliwell, 2016-03-02 23:49:51]

On 2016/02/17 22:33:33, Ken Russell wrote:
> On 2016/02/17 20:51:49, Zhenyao Mo wrote:
> > On 2016/02/17 19:59:09, Ken Russell wrote:
> > > I don't understand how this fix works, since it mainly seems to affect the
> > > behavior of the command buffer's "default" back buffer, which is no longer
> > used
> > > by WebGL. Instead, WebGL's back buffer is managed in Blink's DrawingBuffer
> > > class. Perhaps it's still getting some of its parameters from the default
> back
> > > buffer via GL queries.
> > > 
> > > If it works, then the current code basically LGTM, though I defer review
to
> > > Brandon and Mo.
> > 
> > Yes, most code deals with back buffer.  It is still nice to have, to make
this
> > emulation complete.
> > 
> > The part that fixed the bug is line 6156 - 6163 in gles2_cmd_decoder.cc
> 
> Got it. Thanks.

I was trying to write a test for this - I had hoped we could force-enable the
workaround on any GPU to test.  But now I realised the behaviour is specific to
this GPU.  I've established that the driver allows you to create RGB textures,
but actually backs them with RGBA storage.  If you bind the texture to a
framebuffer, you can render into the alpha channel, and then you can read that
alpha channel back in a shader later.  I wrote a standalone test program to show
this happening in isolation.

That's what was happening here: DrawingBuffer allocates RGB texture, it's bound
to a framebuffer, ClearUnclearedAttachments cleared alpha to 0, and later the
compositor read the alpha channel in its shader.

Based on this behaviour, I'd love to hear your thoughts on what else may be
needed for correctness of the workaround.  It seems like it will be hard to test
on a regular GPU, unless we go further and implement the 'back with RGBA
texture' part on the Chromium side instead of relying on whatever the driver is
doing here.

Thoughts?

[Zhenyao Mo, 2016-03-03 23:48:21]

On 2016/03/02 23:49:51, halliwell wrote:
> On 2016/02/17 22:33:33, Ken Russell wrote:
> > On 2016/02/17 20:51:49, Zhenyao Mo wrote:
> > > On 2016/02/17 19:59:09, Ken Russell wrote:
> > > > I don't understand how this fix works, since it mainly seems to affect
the
> > > > behavior of the command buffer's "default" back buffer, which is no
longer
> > > used
> > > > by WebGL. Instead, WebGL's back buffer is managed in Blink's
DrawingBuffer
> > > > class. Perhaps it's still getting some of its parameters from the
default
> > back
> > > > buffer via GL queries.
> > > > 
> > > > If it works, then the current code basically LGTM, though I defer review
> to
> > > > Brandon and Mo.
> > > 
> > > Yes, most code deals with back buffer.  It is still nice to have, to make
> this
> > > emulation complete.
> > > 
> > > The part that fixed the bug is line 6156 - 6163 in gles2_cmd_decoder.cc
> > 
> > Got it. Thanks.
> 
> I was trying to write a test for this - I had hoped we could force-enable the
> workaround on any GPU to test.  But now I realised the behaviour is specific
to
> this GPU.  I've established that the driver allows you to create RGB textures,
> but actually backs them with RGBA storage.  If you bind the texture to a
> framebuffer, you can render into the alpha channel, and then you can read that
> alpha channel back in a shader later.  I wrote a standalone test program to
show
> this happening in isolation.
> 
> That's what was happening here: DrawingBuffer allocates RGB texture, it's
bound
> to a framebuffer, ClearUnclearedAttachments cleared alpha to 0, and later the
> compositor read the alpha channel in its shader.
> 
> Based on this behaviour, I'd love to hear your thoughts on what else may be
> needed for correctness of the workaround.  It seems like it will be hard to
test
> on a regular GPU, unless we go further and implement the 'back with RGBA
> texture' part on the Chromium side instead of relying on whatever the driver
is
> doing here.
> 
> Thoughts?

I agree it's not easy to test on a GPU without this issue. The easiest is to run
WebGL conformance on the affected hardware.

[Ken Russell (switch to Gerrit), 2016-03-03 23:57:06]

On 2016/03/03 23:48:21, Zhenyao Mo wrote:
> On 2016/03/02 23:49:51, halliwell wrote:
> > On 2016/02/17 22:33:33, Ken Russell wrote:
> > > On 2016/02/17 20:51:49, Zhenyao Mo wrote:
> > > > On 2016/02/17 19:59:09, Ken Russell wrote:
> > > > > I don't understand how this fix works, since it mainly seems to affect
> the
> > > > > behavior of the command buffer's "default" back buffer, which is no
> longer
> > > > used
> > > > > by WebGL. Instead, WebGL's back buffer is managed in Blink's
> DrawingBuffer
> > > > > class. Perhaps it's still getting some of its parameters from the
> default
> > > back
> > > > > buffer via GL queries.
> > > > > 
> > > > > If it works, then the current code basically LGTM, though I defer
review
> > to
> > > > > Brandon and Mo.
> > > > 
> > > > Yes, most code deals with back buffer.  It is still nice to have, to
make
> > this
> > > > emulation complete.
> > > > 
> > > > The part that fixed the bug is line 6156 - 6163 in gles2_cmd_decoder.cc
> > > 
> > > Got it. Thanks.
> > 
> > I was trying to write a test for this - I had hoped we could force-enable
the
> > workaround on any GPU to test.  But now I realised the behaviour is specific
> to
> > this GPU.  I've established that the driver allows you to create RGB
textures,
> > but actually backs them with RGBA storage.  If you bind the texture to a
> > framebuffer, you can render into the alpha channel, and then you can read
that
> > alpha channel back in a shader later.  I wrote a standalone test program to
> show
> > this happening in isolation.
> > 
> > That's what was happening here: DrawingBuffer allocates RGB texture, it's
> bound
> > to a framebuffer, ClearUnclearedAttachments cleared alpha to 0, and later
the
> > compositor read the alpha channel in its shader.
> > 
> > Based on this behaviour, I'd love to hear your thoughts on what else may be
> > needed for correctness of the workaround.  It seems like it will be hard to
> test
> > on a regular GPU, unless we go further and implement the 'back with RGBA
> > texture' part on the Chromium side instead of relying on whatever the driver
> is
> > doing here.
> > 
> > Thoughts?
> 
> I agree it's not easy to test on a GPU without this issue. The easiest is to
run
> WebGL conformance on the affected hardware.

I'm not 100% sure, but I think the best solution to this problem would be to
teach the DrawingBuffer.cpp code in Blink about the need to stop using the
GL_RGB format in DrawingBuffer::defaultTextureParameters. Not sure how to
communicate that from the embedder -- maybe via WebGraphicsInfo in
WebGraphicsContext3D.h. Then DrawingBuffer can ensure that the alpha channel is
cleared to 1 before handing it off to the compositor. To be honest I don't
understand why this patch works.

[halliwell, 2016-03-04 03:17:18]

On 2016/03/03 23:57:06, Ken Russell wrote:
> On 2016/03/03 23:48:21, Zhenyao Mo wrote:
> > On 2016/03/02 23:49:51, halliwell wrote:
> > > On 2016/02/17 22:33:33, Ken Russell wrote:
> > > > On 2016/02/17 20:51:49, Zhenyao Mo wrote:
> > > > > On 2016/02/17 19:59:09, Ken Russell wrote:
> > > > > > I don't understand how this fix works, since it mainly seems to
affect
> > the
> > > > > > behavior of the command buffer's "default" back buffer, which is no
> > longer
> > > > > used
> > > > > > by WebGL. Instead, WebGL's back buffer is managed in Blink's
> > DrawingBuffer
> > > > > > class. Perhaps it's still getting some of its parameters from the
> > default
> > > > back
> > > > > > buffer via GL queries.
> > > > > > 
> > > > > > If it works, then the current code basically LGTM, though I defer
> review
> > > to
> > > > > > Brandon and Mo.
> > > > > 
> > > > > Yes, most code deals with back buffer.  It is still nice to have, to
> make
> > > this
> > > > > emulation complete.
> > > > > 
> > > > > The part that fixed the bug is line 6156 - 6163 in
gles2_cmd_decoder.cc
> > > > 
> > > > Got it. Thanks.
> > > 
> > > I was trying to write a test for this - I had hoped we could force-enable
> the
> > > workaround on any GPU to test.  But now I realised the behaviour is
specific
> > to
> > > this GPU.  I've established that the driver allows you to create RGB
> textures,
> > > but actually backs them with RGBA storage.  If you bind the texture to a
> > > framebuffer, you can render into the alpha channel, and then you can read
> that
> > > alpha channel back in a shader later.  I wrote a standalone test program
to
> > show
> > > this happening in isolation.
> > > 
> > > That's what was happening here: DrawingBuffer allocates RGB texture, it's
> > bound
> > > to a framebuffer, ClearUnclearedAttachments cleared alpha to 0, and later
> the
> > > compositor read the alpha channel in its shader.
> > > 
> > > Based on this behaviour, I'd love to hear your thoughts on what else may
be
> > > needed for correctness of the workaround.  It seems like it will be hard
to
> > test
> > > on a regular GPU, unless we go further and implement the 'back with RGBA
> > > texture' part on the Chromium side instead of relying on whatever the
driver
> > is
> > > doing here.
> > > 
> > > Thoughts?
> > 
> > I agree it's not easy to test on a GPU without this issue. The easiest is to
> run
> > WebGL conformance on the affected hardware.
> 
> I'm not 100% sure, but I think the best solution to this problem would be to
> teach the DrawingBuffer.cpp code in Blink about the need to stop using the
> GL_RGB format in DrawingBuffer::defaultTextureParameters. Not sure how to
> communicate that from the embedder -- maybe via WebGraphicsInfo in
> WebGraphicsContext3D.h. Then DrawingBuffer can ensure that the alpha channel
is
> cleared to 1 before handing it off to the compositor. To be honest I don't
> understand why this patch works.

I put something on your calendars for next week so we could talk the rest of
this through :)