cc: Use highp precision for texture coords if available and needed

cc: Use highp precision for texture coords if available and needed

This fixes an earlier version of this patch that had a bug in
FragmentShaderRGBATexAlphaMask.

High precision is only used when the source is greater than N
pixels in any direction to prevent performance regressions. N is
determined based on the actual precision of mediump reported by
the driver.

BUG=183581
BUG=173747
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191692

[brianderson, 2013-03-20 18:55:22]

Please take a look.

This version is based on https://codereview.chromium.org/12314003, which had to
be reverted due to https://code.google.com/p/chromium/issues/detail?id=183581.

This version fixes the bug that caused it to be reverted. It also only uses
highp only if absolutely needed to alleviate performance concerns. For example,
tiles are generally small enough not to need highp texture coordinates.

[brianderson, 2013-03-20 18:58:10]

https://codereview.chromium.org/12665005/diff/21001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/12665005/diff/21001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:718:
quad->shared_quad_state->visible_content_rect.bottom_right());
enne: Is this a good way to determine the maximum texture coordinate that will
be used?

[vangelis, 2013-03-20 19:20:44]

I like the new direction.  In addition to the comments, please watch out for
80col limits in a couple of files.

https://codereview.chromium.org/12665005/diff/21001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/12665005/diff/21001/cc/output/gl_renderer.h#n...
cc/output/gl_renderer.h:232: typedef ProgramBinding<VertexShaderPosTexTransform,
I don't believe the highp typedefs are needed. They are identical to the normalp
and they're not used anyway.

https://codereview.chromium.org/12665005/diff/21001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/12665005/diff/21001/cc/output/shader.cc#newco...
cc/output/shader.cc:40: if (requestedPrecision == TexCoordPrecisionHigh) {
The context is no longer needed here, is it?

https://codereview.chromium.org/12665005/diff/21001/cc/output/shader.cc#newco...
cc/output/shader.cc:56: return "#define TexCoordPrecision highp\n" +
Not sure I understand why for the vertex shaders we blindly use highp but for
fragment shader we're passing the precision in as an argument.

[aelias_OOO_until_Jul13, 2013-03-20 19:55:01]

https://codereview.chromium.org/12665005/diff/21001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/12665005/diff/21001/cc/output/shader.cc#newco...
cc/output/shader.cc:64: if (max_coordinate.x() > 1024 || max_coordinate.y() >
1024)
1280 is a common height on mid-end phones like the Galaxy Nexus so this
threshold would activate highp on their fbo copy.  My understanding is really
want highp for very high resolutions like 2560, so could we bump it up to 2048?

[brianderson, 2013-03-20 20:57:15]

I will likely rebase this on top of https://codereview.chromium.org/12844006/ to
address the 80 column limit and other formatting issues.

https://codereview.chromium.org/12665005/diff/21001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/12665005/diff/21001/cc/output/gl_renderer.h#n...
cc/output/gl_renderer.h:232: typedef ProgramBinding<VertexShaderPosTexTransform,
On 2013/03/20 19:20:44, vangelis wrote:
> I don't believe the highp typedefs are needed. They are identical to the
normalp
> and they're not used anyway.

Ah yes, these are not needed and are not used. I used to have the precision as a
template parameter, but decided not to take that route.

https://codereview.chromium.org/12665005/diff/21001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/12665005/diff/21001/cc/output/shader.cc#newco...
cc/output/shader.cc:40: if (requestedPrecision == TexCoordPrecisionHigh) {
On 2013/03/20 19:20:44, vangelis wrote:
> The context is no longer needed here, is it?
Context is no longer needed. Will remove it.

https://codereview.chromium.org/12665005/diff/21001/cc/output/shader.cc#newco...
cc/output/shader.cc:56: return "#define TexCoordPrecision highp\n" +
On 2013/03/20 19:20:44, vangelis wrote:
> Not sure I understand why for the vertex shaders we blindly use highp but for
> fragment shader we're passing the precision in as an argument.

My thinking was that we're not likely to be vertex shader bound and should just
use highp blindly to avoid any unexpected corner cases. Some of the vertex
shaders use math that can reduce the final effective precision.

https://codereview.chromium.org/12665005/diff/21001/cc/output/shader.cc#newco...
cc/output/shader.cc:64: if (max_coordinate.x() > 1024 || max_coordinate.y() >
1024)
On 2013/03/20 19:55:01, aelias wrote:
> 1280 is a common height on mid-end phones like the Galaxy Nexus so this
> threshold would activate highp on their fbo copy.  My understanding is really
> want highp for very high resolutions like 2560, so could we bump it up to
2048?

mediump is only guaranteed to provide 10 bits of precision, so we can represent
exact integer coordinates up to 2^10 = 1024.  Beyond that, an FBO copy will not
be perfect with mediump precision.

Are we even doing large FBO copies in the common case at ToT? We were not able
to reproduce the same mediump corruption at ToT as we were with the M18 branch,
(presumably because a large copy had been removed). Also, at ToT, the
SurfaceFlinger dump seems to indicate that the Renderer's output gets its own
overlay.

[aelias_OOO_until_Jul13, 2013-03-20 22:37:18]

We're drawing from a near-screen-size FBO to a hardware overlay as one big
TextureDrawQuad.  The input texture coordinates for that draw span the
width/height of the screen.

Hmm, 1024 does seem like the principled threshold... could we put that number in
a cc Setting so that Android can at least hack it to something different if we
see a performance problem?

[brianderson, 2013-03-21 00:50:00]

Talked with aelias and we decided to make the highp threshold easily adjustable
from LayerTreeSettings.

Most recent version will likely need to be rabased on many pending style
patches.

[Sami, 2013-03-21 12:07:57]

I like this solution. Plumbing the highp threshold everywhere seems like a pain,
though. It sounds like we want a different threshold for Android, but if we
didn't, it could be a static constant or getter instead.

> Are we even doing large FBO copies in the common case at ToT? We were
> not able to reproduce the same mediump corruption at ToT as we were with
> the M18 branch, (presumably because a large copy had been removed).
> Also, at ToT, the SurfaceFlinger dump seems to indicate that the
> Renderer's output gets its own overlay.

There's still a blit from the renderer compositor's output texture to the window
in the browser compositor. Has anyone checked what Galaxy Nexus says about
mediump precision? Maybe it's more than 10 bits and that's why the FBO blit
doesn't show any error. Still, we should see the error on Nexus 10.

[brianderson, 2013-03-21 18:20:21]

On 2013/03/21 12:07:57, Sami wrote:
> I like this solution. Plumbing the highp threshold everywhere seems like a
pain,
> though. It sounds like we want a different threshold for Android, but if we
> didn't, it could be a static constant or getter instead.
> 
> > Are we even doing large FBO copies in the common case at ToT? We were
> > not able to reproduce the same mediump corruption at ToT as we were with
> > the M18 branch, (presumably because a large copy had been removed).
> > Also, at ToT, the SurfaceFlinger dump seems to indicate that the
> > Renderer's output gets its own overlay.
> 
> There's still a blit from the renderer compositor's output texture to the
window
> in the browser compositor. Has anyone checked what Galaxy Nexus says about
> mediump precision? Maybe it's more than 10 bits and that's why the FBO blit
> doesn't show any error. Still, we should see the error on Nexus 10.

Many devices actually use the same precision for mediump and highp. For the
devices where mediump and highp are different, the compiler can promote mediump
to highp. Different compiler versions do not necessarily promote in the same
manner, possible exposing corruption as the compiler gets better at optimizing.

So it's possible that 1) mediump actually has the precision needed or 2) the
compiler is silently promoting to highp and we are just getting lucky on devices
other than the N10.

Instead of hard-coding the threshold, I will query for the actual mediump
precision to determine the threshold for the device we are running on. That
would also allow me to get rid of the tedious plumbing.

If the compiler is silently promoting anyway, we won't have a performance
regression. And if the compiler doesn't silently promote, we protect ourselves
from using a lower precision than we need.

[brianderson, 2013-03-21 22:28:11]

Latest version sets the highp threshold based on the actual precision of
mediump.  Alex, is that acceptable?

Note: This should not land before https://codereview.chromium.org/13004003,
which caches the precision queries client-side to avoid many blocking round
trips to the server.

[aelias_OOO_until_Jul13, 2013-03-22 01:42:25]

Seems good provided the query works as it should in reality.  What values does
the precision query come up with in practice for the Galaxy Nexus and Nexus 7?

[Sami, 2013-03-22 11:14:34]

I think this looks much better now, lgtm.

https://codereview.chromium.org/12665005/diff/47001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/12665005/diff/47001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2018: debug_border_program_ = make_scoped_ptr(new
DebugBorderProgram(context_, TexCoordPrecisionNA));
Nit: line length

https://codereview.chromium.org/12665005/diff/47001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/12665005/diff/47001/cc/output/shader.cc#newco...
cc/output/shader.cc:46: } else {
Just an idea, feel free to ignore: if we're given TexCoordPrecisionNA (in debug
builds), #define TexCoordPrecision to something that will cause the shader
compilation to fail. This way we'll catch people adding shaders without thinking
about the precision they need.

https://codereview.chromium.org/12665005/diff/47001/cc/output/shader.cc#newco...
cc/output/shader.cc:74: const gfx::Point &max_coordinate) {
Nit: "Point&" instead of "Point &"

[brianderson, 2013-03-22 22:04:44]

Note:
N4 is the only device I tested that reports 24 bits of precision for mediump.

The following devices all report 10 bits of precision for mediump: Nexus S,
Galaxy S II, Galaxy Nexus, N10

Still need to get my hands on an N7.

[brianderson, 2013-03-22 22:42:10]

N7 reports that mediump has 13 bits of precision.

I talked with Alex again and, since so many devices will have a threshold of
1024, it's risky that we might still regress performance with the current patch.

Since there isn't a reliable repro of the original bug anymore and this fix is
mostly theoretical, I am willing to increase the threshold until we have
concrete proof that we need to be strict. However, I definitely want to get this
patch in since the precision issue is bound to show up again at some point.

[brianderson, 2013-03-22 22:57:07]

https://codereview.chromium.org/12665005/diff/70001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/12665005/diff/70001/cc/output/shader.cc#newco...
cc/output/shader.cc:84: return std::max(1 << precision, 2048);
Alex, what do you think of this way of being conservative?

[brianderson, 2013-03-23 02:30:25]

Rebased on pending chromification patch:
https://codereview.chromium.org/12844006

[aelias_OOO_until_Jul13, 2013-03-24 00:52:02]

https://codereview.chromium.org/12665005/diff/74001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/12665005/diff/74001/cc/output/shader.cc#newco...
cc/output/shader.cc:88: #if defined(OS_ANDROID)
Fine functionality-wise, but there shouldn't be any #ifdefs in cc.  Can this be
moved to a setting?

[danakj, 2013-03-24 17:39:58]

https://codereview.chromium.org/12665005/diff/74001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/12665005/diff/74001/cc/output/shader.cc#newco...
cc/output/shader.cc:88: #if defined(OS_ANDROID)
On 2013/03/24 00:52:02, aelias wrote:
> Fine functionality-wise, but there shouldn't be any #ifdefs in cc.  Can this
be
> moved to a setting?

And then running the shader compilation unit tests with/without the setting
would be bonus cool.

[brianderson, 2013-03-25 22:58:48]

The settings have been added back in and I've updated the
gl_renderer_unittest.cc and added a shader_unittest.cc.

[aelias_OOO_until_Jul13, 2013-03-25 23:01:52]

Please also add the setting in
content/browser/renderer_host/compositor_impl_android.cc

[commit-bot: I haz the power, 2013-03-25 23:48:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/12665005/87001

[commit-bot: I haz the power, 2013-03-25 23:49:06]

Presubmit check for 12665005-87001 failed and returned exit status 1.

INFO:root:Found 30 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/cc/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    content/renderer/gpu/render_widget_compositor.cc
    content/browser/renderer_host/compositor_impl_android.cc

Presubmit checks took 5.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[aelias_OOO_until_Jul13, 2013-03-25 23:50:45]

lgtm for content/browser/renderer_host/compositor_impl_android.cc

[brianderson, 2013-03-25 23:52:22]

Al, can you review content/renderer/gpu/render_widget_compositor.cc ?

I need an owner's lgtm.

[jamesr, 2013-03-25 23:54:27]

lgtm for content/renderer/gpu/

[commit-bot: I haz the power, 2013-03-26 00:00:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/12665005/87001

[commit-bot: I haz the power, 2013-03-26 00:38:35]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-26 01:01:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/12665005/113002

[commit-bot: I haz the power, 2013-03-26 01:39:48]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-26 02:18:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/12665005/118003

[commit-bot: I haz the power, 2013-03-26 03:29:56]

Retried try job too often on linux_aura for step(s) aura_unittests,
compositor_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-03-26 18:02:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/12665005/118003

[commit-bot: I haz the power, 2013-03-26 18:24:00]

Retried try job too often on win_x64_rel for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2013-03-27 00:20:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/12665005/133001

[commit-bot: I haz the power, 2013-03-27 00:21:03]

Presubmit check for 12665005-133001 failed and returned exit status 1.

INFO:root:Found 31 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/cc/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    gpu/command_buffer/service/gles2_cmd_decoder.cc

Presubmit checks took 2.7s to calculate.

[commit-bot: I haz the power, 2013-03-27 00:26:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/12665005/137001

[commit-bot: I haz the power, 2013-03-27 00:40:39]

Failed to trigger a try job on win7_aura
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-03-27 00:40:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/12665005/142001

[commit-bot: I haz the power, 2013-03-27 01:34:04]

Retried try job too often on linux_aura for step(s) compositor_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[brianderson, 2013-03-27 02:37:35]

Tests are failing because the in-process implementation of glGetShaderPrecision
format is missing some workarounds.  This patch should be good to go after the
following patch lands:
https://codereview.chromium.org/13111002/

[commit-bot: I haz the power, 2013-04-01 18:34:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/12665005/159001

[commit-bot: I haz the power, 2013-04-01 23:03:12]

Change committed as 191692