Correction of texture domain bounds.

The code that flips the orientation of the texture domain was not swizzling the bounds correctly.  The shader assumes that the bounds are passed as (t, l, b, r).  

Inverting or scaling the bounds can invalidate the l < r, t < b constraints.

This fix resolves chrome issue:  crbug.com/84010

Committed: http://code.google.com/p/skia/source/detail?r=1483

[twiz, 2011-06-01 20:54:32]

PTAL.

Justin, did you add a unit test for the texture domain clamping behaviour?  Can
the orientation path be exercised by that test?

[twiz, 2011-06-01 20:57:10]

Sending again, so that skia-review@googlegroups.com picks up the thread.

On 2011/06/01 20:54:32, twiz wrote:
> PTAL.
> 
> Justin, did you add a unit test for the texture domain clamping behaviour? 
Can
> the orientation path be exercised by that test?

[reed1, 2011-06-01 21:05:42]

http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:347: 
Should we check/sort the texDom here, before we store it in fProgramData? GrRect
(SkRect) has a method called sort() which ensures that L <= R and T <= B.

In that spirit, shouldn't we just assert that we're sorted here, and perform
this sort/check up where the textureDomain was assigned to fSamplerStates[]? I
just hate to pass around bogus data that might throw off our caches.

http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:374: 
If we don't perform the check earlier, lets at least add a comment here

1. what invariant we're enforcing
2. why it might have been wrong

[junov1, 2011-06-01 21:23:05]

http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:367: values[3] = 1.0f - values[3];
I think it would be enough to swap values[1] and values[3] unconditionally right
here.

[reed1, 2011-06-01 21:25:39]

On 2011/06/01 21:23:05, junov1 wrote:
> http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp
> File gpu/src/GrGpuGLShaders.cpp (right):
> 
>
http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
> gpu/src/GrGpuGLShaders.cpp:367: values[3] = 1.0f - values[3];
> I think it would be enough to swap values[1] and values[3] unconditionally
right
> here.


Ah, are you saying the only inversion comes from the 1-x when the orientation is
reversed?

[junov1, 2011-06-01 21:25:42]

On 2011/06/01 20:54:32, twiz wrote:
> PTAL.
> 
> Justin, did you add a unit test for the texture domain clamping behaviour? 
Can
> the orientation path be exercised by that test?

I added a sample in Sample app, but it clearly does not cover this case.  Feel
free to augment the test in SampleTextureDomain.cpp

[twiz, 2011-06-01 21:43:39]

http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:347: 
On 2011/06/01 21:05:42, reed1 wrote:
> Should we check/sort the texDom here, before we store it in fProgramData?
GrRect
> (SkRect) has a method called sort() which ensures that L <= R and T <= B.
> 
> In that spirit, shouldn't we just assert that we're sorted here, and perform
> this sort/check up where the textureDomain was assigned to fSamplerStates[]? I
> just hate to pass around bogus data that might throw off our caches.

texDom should always be sorted.  We could add an assert here.  The only
inversion could come from the orientation flip, and if the contentScale* factors
can be negative.

http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:367: values[3] = 1.0f - values[3];
On 2011/06/01 21:23:05, junov1 wrote:
> I think it would be enough to swap values[1] and values[3] unconditionally
right
> here.  

I thought about doing the swap here, but I was uncertain if the scale factors
below could swap the coordinates.

[twiz, 2011-06-01 21:47:59]

On 2011/06/01 21:43:39, twiz wrote:
> http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp
> File gpu/src/GrGpuGLShaders.cpp (right):
> 
>
http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
> gpu/src/GrGpuGLShaders.cpp:347: 
> On 2011/06/01 21:05:42, reed1 wrote:
> > Should we check/sort the texDom here, before we store it in fProgramData?
> GrRect
> > (SkRect) has a method called sort() which ensures that L <= R and T <= B.
> > 
> > In that spirit, shouldn't we just assert that we're sorted here, and perform
> > this sort/check up where the textureDomain was assigned to fSamplerStates[]?
I
> > just hate to pass around bogus data that might throw off our caches.
> 
> texDom should always be sorted.  We could add an assert here.  The only
> inversion could come from the orientation flip, and if the contentScale*
factors
> can be negative.

Reviewing the code, it seems that the content factors should always be positive.
 

I uploaded a new patch that swaps according to Justin's comment.

Will also modify the test to cover this case.

> 
>
http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
> gpu/src/GrGpuGLShaders.cpp:367: values[3] = 1.0f - values[3];
> On 2011/06/01 21:23:05, junov1 wrote:
> > I think it would be enough to swap values[1] and values[3] unconditionally
> right
> > here.  
> 
> I thought about doing the swap here, but I was uncertain if the scale factors
> below could swap the coordinates.

[junov1, 2011-06-01 23:18:34]

LGTM

[reed1, 2011-06-02 11:53:30]

short, sweet, with comment

LGTM

[twiz, 2011-06-02 20:12:43]

On 2011/06/01 21:47:59, twiz wrote:
> On 2011/06/01 21:43:39, twiz wrote:
> > http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp
> > File gpu/src/GrGpuGLShaders.cpp (right):
> > 
> >
>
http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
> > gpu/src/GrGpuGLShaders.cpp:347: 
> > On 2011/06/01 21:05:42, reed1 wrote:
> > > Should we check/sort the texDom here, before we store it in fProgramData?
> > GrRect
> > > (SkRect) has a method called sort() which ensures that L <= R and T <= B.
> > > 
> > > In that spirit, shouldn't we just assert that we're sorted here, and
perform
> > > this sort/check up where the textureDomain was assigned to
fSamplerStates[]?
> I
> > > just hate to pass around bogus data that might throw off our caches.
> > 
> > texDom should always be sorted.  We could add an assert here.  The only
> > inversion could come from the orientation flip, and if the contentScale*
> factors
> > can be negative.
> 
> Reviewing the code, it seems that the content factors should always be
positive.
>  
> 
> I uploaded a new patch that swaps according to Justin's comment.
> 
> Will also modify the test to cover this case.

New patch uploaded that tests the bottom-up texture orientation in
SampleTextureDomain.cpp.

Please take a last look.

> 
> > 
> >
>
http://codereview.appspot.com/4517126/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
> > gpu/src/GrGpuGLShaders.cpp:367: values[3] = 1.0f - values[3];
> > On 2011/06/01 21:23:05, junov1 wrote:
> > > I think it would be enough to swap values[1] and values[3] unconditionally
> > right
> > > here.  
> > 
> > I thought about doing the swap here, but I was uncertain if the scale
factors
> > below could swap the coordinates.

[reed1, 2011-06-02 20:18:21]

LGTM

[junov, 2011-06-03 13:27:17]

On 2011/06/02 20:18:21, reed1 wrote:
> LGTM

LGTM