Key shader on whether frag pos read is relative to top-left or bottom-left

Key shader on whether frag pos read is relative to top-left or bottom-left

R=robertphillips@google.com

Committed: https://code.google.com/p/skia/source/detail?r=9113

[bsalomon, 2013-05-10 13:38:04]

I tested this by making GM do saveLayer() before each GM and hacking SkGpuDevice
to make all layers top-left. I then compared with a normal GM run. The
differences were all trivial edge/interp differences.

[Stephen White, 2013-05-10 13:42:05]

On 2013/05/10 13:38:04, bsalomon wrote:
> I tested this by making GM do saveLayer() before each GM and hacking
SkGpuDevice
> to make all layers top-left. I then compared with a normal GM run. The
> differences were all trivial edge/interp differences.

Will this affect Chrome layout test results?  (in particular
css3/filters/effect-reference-hw.html).  We're already using TopDown textures
for filters in Chrome. I'm fine with suppressing these failures on the skia roll
if so, but otherwise we should probably do the #ifdef dance.

[robertphillips, 2013-05-10 13:53:58]

lgtm + nits & suggestions

https://codereview.chromium.org/14633007/diff/1/src/gpu/gl/GrGLProgramDesc.cpp
File src/gpu/gl/GrGLProgramDesc.cpp (right):

https://codereview.chromium.org/14633007/diff/1/src/gpu/gl/GrGLProgramDesc.cp...
src/gpu/gl/GrGLProgramDesc.cpp:117: } else {
Use 'kNoFragPosRead_FragPosKey' here?

https://codereview.chromium.org/14633007/diff/1/src/gpu/gl/GrGLProgramDesc.h
File src/gpu/gl/GrGLProgramDesc.h (right):

https://codereview.chromium.org/14633007/diff/1/src/gpu/gl/GrGLProgramDesc.h#...
src/gpu/gl/GrGLProgramDesc.h:110: // Otherwise, 0.
fFragPosKey?

https://codereview.chromium.org/14633007/diff/1/src/gpu/gl/GrGLShaderBuilder.cpp
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/14633007/diff/1/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.cpp:354: 
// Note: this method only be called if the fragment position is required

https://codereview.chromium.org/14633007/diff/1/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.cpp:357: return kTopLeft_GrSurfaceOrigin ==
dst->origin() ?
rm space?

[bsalomon, 2013-05-10 13:56:46]

On 2013/05/10 13:42:05, Stephen White wrote:
> On 2013/05/10 13:38:04, bsalomon wrote:
> > I tested this by making GM do saveLayer() before each GM and hacking
> SkGpuDevice
> > to make all layers top-left. I then compared with a normal GM run. The
> > differences were all trivial edge/interp differences.
> 
> Will this affect Chrome layout test results?  (in particular
> css3/filters/effect-reference-hw.html).  We're already using TopDown textures
> for filters in Chrome. I'm fine with suppressing these failures on the skia
roll
> if so, but otherwise we should probably do the #ifdef dance.

I don't think so. I haven't changed which textures are TL vs BL, just fixed the
rendering issues with TL RTs and effects that use frag-pos. But I can give that
test a try. But maybe I'm missing something. Does blink/chrome do some kind of
workaround for our bugs? I'll try running that test.

[bsalomon, 2013-05-13 15:16:49]

I tested the css3/filters/effect-reference-hw.html test with this change and it
passes.

https://codereview.chromium.org/14633007/diff/1/src/gpu/gl/GrGLProgramDesc.cpp
File src/gpu/gl/GrGLProgramDesc.cpp (right):

https://codereview.chromium.org/14633007/diff/1/src/gpu/gl/GrGLProgramDesc.cp...
src/gpu/gl/GrGLProgramDesc.cpp:117: } else {
On 2013/05/10 13:53:58, robertphillips wrote:
> Use 'kNoFragPosRead_FragPosKey' here?

It's private. I think in the long run the desc should be a nested class of the
builder and this enum could be visible to both.

https://codereview.chromium.org/14633007/diff/1/src/gpu/gl/GrGLProgramDesc.h
File src/gpu/gl/GrGLProgramDesc.h (right):

https://codereview.chromium.org/14633007/diff/1/src/gpu/gl/GrGLProgramDesc.h#...
src/gpu/gl/GrGLProgramDesc.h:110: // Otherwise, 0.
On 2013/05/10 13:53:58, robertphillips wrote:
> fFragPosKey?

Done, and changed fDstRead->fDstReadKey

https://codereview.chromium.org/14633007/diff/1/src/gpu/gl/GrGLShaderBuilder.cpp
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/14633007/diff/1/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.cpp:354: 
On 2013/05/10 13:53:58, robertphillips wrote:
> // Note: this method only be called if the fragment position is required

done (in the header)

https://codereview.chromium.org/14633007/diff/1/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.cpp:357: return kTopLeft_GrSurfaceOrigin ==
dst->origin() ?
On 2013/05/10 13:53:58, robertphillips wrote:
> rm space?

Changed to if/else.

[bsalomon, 2013-05-13 18:50:45]

Committed patchset #2 manually as r9113 (presubmit successful).