cc: Use highp precision for texture coordinates if available

cc: Use highp precision for texture coordinates if available

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

[brianderson, 2013-02-21 02:25:56]

Right now, this arbitrarily uses highp for all calculations involved with
texture coordinates if highp is available.  Otherwise, it uses mediump.

If there are performance regressions, we will want to use highp more
selectively; but given that mediump shows corruption even in simple cases
(https://code.google.com/p/chromium/issues/detail?id=173804), it'll be tricky to
determine all the places where we can get away with just mediump.

[Vangelis Kokkevis, 2013-02-21 07:47:35]

https://codereview.chromium.org/12314003/diff/3/cc/shader.cc
File cc/shader.cc (right):

https://codereview.chromium.org/12314003/diff/3/cc/shader.cc#newcode33
cc/shader.cc:33: static std::string fixShader(const char* shaderString) {
nit: please use a more descriptive name for this function (e.g.
setCoordPrecision or something similar). "fix" is a bit generic.

https://codereview.chromium.org/12314003/diff/3/cc/shader.cc#newcode66
cc/shader.cc:66: attribute TexCoordPrecision vec2 a_texCoord;
I wonder if we should leave the vertex shaders alone.  The default float
precision is highp . There's not reason to force them down to mediump, is there?

[Sami, 2013-02-21 11:47:38]

I like the approach of doing this with a macro at the top of the shader and only
changing the needed variables.

Before landing I think we should run some tests -- especially on the Galaxy
Nexus -- to see the performance impact. If it turns out there's a big effect,
then we could provide two variants of the problematic shaders and pick one at
runtime -- maybe based on the same LINEAR/NEAREST filtering logic.

https://codereview.chromium.org/12314003/diff/3/cc/shader.cc
File cc/shader.cc (right):

https://codereview.chromium.org/12314003/diff/3/cc/shader.cc#newcode66
cc/shader.cc:66: attribute TexCoordPrecision vec2 a_texCoord;
On 2013/02/21 07:47:35, Vangelis Kokkevis wrote:
> I wonder if we should leave the vertex shaders alone.  The default float
> precision is highp . There's not reason to force them down to mediump, is
there?
> 

Right. We could add a VERTEX_SHADER and FRAGMENT_SHADER macro and only set the
precision in the latter one.

[brianderson, 2013-02-21 23:05:54]

After further investigation, it looks like GL_FRAGMENT_PRECISION_HIGH is not
defined on N4, N7, or N10, even though highp is supported.

Is it possible we are evaluating GL_FRAGMENT_PRECISION_HIGH before our shaders
get to the driver?

https://codereview.chromium.org/12314003/diff/3/cc/shader.cc
File cc/shader.cc (right):

https://codereview.chromium.org/12314003/diff/3/cc/shader.cc#newcode66
cc/shader.cc:66: attribute TexCoordPrecision vec2 a_texCoord;
On 2013/02/21 11:47:38, Sami wrote:
> On 2013/02/21 07:47:35, Vangelis Kokkevis wrote:
> > I wonder if we should leave the vertex shaders alone.  The default float
> > precision is highp . There's not reason to force them down to mediump, is
> there?
> > 
> 
> Right. We could add a VERTEX_SHADER and FRAGMENT_SHADER macro and only set the
> precision in the latter one.

Hmm, I had VERTEX_SHADER and FRAGMENT_SHADER macros in a local version of this
patch, but was getting shader compiler errors and took it out.  I'm not getting
those errors anymore, so I must have done something wrong before.

I will add it back, especially since I'm noticing cracks with forced mediump
precision in the vertex shader.

Note: using different precisions for texture coordinates between the vertex
shader and fragment shader work fine on N4, N7, and N10.

[brianderson, 2013-02-22 00:50:23]

Looks like GL_FRAGMENT_PRECISION_HIGH being undefined might be a bug in ANGLE. 
I have opened a bug for that seperately here:
https://code.google.com/p/angleproject/issues/detail?id=413

In the mean time, I will change this patch to call glGetShaderPrecisionFormat to
determine the available precision.

[Vangelis Kokkevis, 2013-02-22 07:13:19]

On 2013/02/22 00:50:23, Brian Anderson wrote:
> Looks like GL_FRAGMENT_PRECISION_HIGH being undefined might be a bug in ANGLE.

> I have opened a bug for that seperately here:
> https://code.google.com/p/angleproject/issues/detail?id=413

Alok may know something about this!

> 
> In the mean time, I will change this patch to call glGetShaderPrecisionFormat
to
> determine the available precision.

[aelias_OOO_until_Jul13, 2013-02-22 07:17:28]

Note that highp is optional on OpenGL ES and may be interpreted as mediump by
low-end devices.  So the places where we see performance degradation might be
surprising (e.g. we may not see degradation on a low-end device because it just
keeps using mediump, but we might see degradation on a slightly more powerful
one that does support it).  So we should test a good range.

[Sami, 2013-02-22 11:52:19]

Good point Alex. Implementations that don't support highp should refuse shaders
that require it, but I suspect some of them may just fall silently back to
mediump and hope that nothing breaks. I'm not sure if this is covered by the
conformance test.

https://codereview.chromium.org/12314003/diff/9001/cc/shader.cc
File cc/shader.cc (right):

https://codereview.chromium.org/12314003/diff/9001/cc/shader.cc#newcode44
cc/shader.cc:44: std::string texCoordPrecision = (precision != 0) ? "highp" :
"mediump";
For extra paranoia let's add DCHECKs here that makes sure the highp precision
and range match the spec requirements (precision >= 16 and range >= {62, 62} I
think).

https://codereview.chromium.org/12314003/diff/9001/cc/shader.cc#newcode53
cc/shader.cc:53: + std::string(shaderString);
Nit: move the + to the previous line for consistency.

[brianderson, 2013-02-26 00:32:48]

Patch Set 4 depends on:
https://codereview.chromium.org/12328092

I've also introduced the concepts of TexCoordPrecisionHigh (highp if available)
and TexCoordPrecisionLow (always mediump).

TexCoordPrecisionHigh is only used for texture coordinates that are passed
straight into texture2DRect().  This reduces the risk that we regress
performance anywhere.  We should only reduce performance if the extra register
space used prevents us from running more fragment shaders in parallel.

TexCoordPrecisionLow is used for all cases that perform any sort of calculation
on the texture coordinate, which includes clamping, masking, and
checkerboarding.  We should convert these to TexCoordPrecisionHigh more
carefully in the future.

Note: This also does not address the inevitable corruption we will see on
devices that simply do not support highp.  It should automatically fall back to
mediump precision however.

[Vangelis Kokkevis, 2013-02-26 01:35:04]

lgtm with comments addressed

https://codereview.chromium.org/12314003/diff/10003/cc/shader.cc
File cc/shader.cc (right):

https://codereview.chromium.org/12314003/diff/10003/cc/shader.cc#newcode37
cc/shader.cc:37: const char* shaderString) {
Open bracket goes in its own line

https://codereview.chromium.org/12314003/diff/10003/cc/shader.cc#newcode38
cc/shader.cc:38: // texCoordPrecisionHigh is used for cases where we absolutely
need high precision or
80 cols. Here and below.

https://codereview.chromium.org/12314003/diff/10003/cc/shader.cc#newcode61
cc/shader.cc:61: else {
else needs to go to the previous line

[Sami, 2013-02-26 11:56:40]

Looks good, just a question about the naming.

https://codereview.chromium.org/12314003/diff/10003/cc/shader.cc
File cc/shader.cc (right):

https://codereview.chromium.org/12314003/diff/10003/cc/shader.cc#newcode11
cc/shader.cc:11: #include "third_party/khronos/GLES2/gl2.h"
Nit: alpha order.

https://codereview.chromium.org/12314003/diff/10003/cc/shader.cc#newcode43
cc/shader.cc:43: // texCoordPrecisionLow is used for cases where we don't need
the high precision
Should we call this texCoordPrecisionMedium instead? Otherwise this could get
confused with lowp.

[brianderson, 2013-02-26 19:08:21]

https://codereview.chromium.org/12314003/diff/10003/cc/shader.cc
File cc/shader.cc (right):

https://codereview.chromium.org/12314003/diff/10003/cc/shader.cc#newcode11
cc/shader.cc:11: #include "third_party/khronos/GLES2/gl2.h"
On 2013/02/26 11:56:40, Sami wrote:
> Nit: alpha order.

Presubmit complained when I put khronos before WebKit, which is why it's like
this.

It appears presubmit's alpha order is case sensitive. A glance at the Google
style guide doesn't say anything about whether the alpha order should be case
sensitive or not.

I plan to leave it like this to keep presubmit happy.

https://codereview.chromium.org/12314003/diff/10003/cc/shader.cc#newcode43
cc/shader.cc:43: // texCoordPrecisionLow is used for cases where we don't need
the high precision
On 2013/02/26 11:56:40, Sami wrote:
> Should we call this texCoordPrecisionMedium instead? Otherwise this could get
> confused with lowp.

That's a good idea.  I'll change it to texCoordPrecisionMedium instead.

[Sami, 2013-02-27 11:31:10]

On 2013/02/26 19:08:21, Brian Anderson wrote:
> It appears presubmit's alpha order is case sensitive. A glance at the Google
> style guide doesn't say anything about whether the alpha order should be case
> sensitive or not.

Oh I see, I guess we're still following some parts of WebKit style here.
 
> That's a good idea.  I'll change it to texCoordPrecisionMedium instead.

Thank you, lgtm.

[brianderson, 2013-03-08 00:50:42]

CQing this since the other dependencies have landed.

This patch ends up being a no-op in most cases since the N10 appears to keep the
high precision of texture coordinates if they aren't mutated in the fragment
shader.  I'm guessing other platforms do something similar.

A follow-up patch will only use highp when absolutely needed (large source
textures), even in shaders that mutate the texture coordinate. This patch
intentionally avoids those cases to conservatively avoid performance
regressions.

[commit-bot: I haz the power, 2013-03-08 00:51:02]

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

[commit-bot: I haz the power, 2013-03-08 02:27:27]

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

[brianderson, 2013-03-11 20:28:05]

There's an unexpected snag with this patch: determining the supported precision
is a synchronous gpu service query and some of the shaders are initialized on
startup.

Synchronous calls are not allowed on startup.

I am currently investigating how we might be able to work around this.

[commit-bot: I haz the power, 2013-03-12 00:10:13]

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

[commit-bot: I haz the power, 2013-03-12 03:57:39]

Change committed as 187481