Make GrGLConfigConversionEffect work for Imagination and some other GPUs.

Make GrGLConfigConversionEffect work for Imagination and some other GPUs.

BUG=372341
Committed: https://skia.googlesource.com/skia/+/93cf46f7d687128d2ad05762bd65fea6c4539553

Committed: https://skia.googlesource.com/skia/+/cecc91c4446c9c2f9b95736e206bfd1d507a75a9

[changjun.yang, 2014-05-12 11:19:53]

As mentioned in the bug entry, I think deviation up to 1 should be accepted for
GPU shader PreservingPMConversion test.
PTAL.

[bsalomon, 2014-05-13 08:09:00]

On 2014/05/12 11:19:53, changjun.yang wrote:
> As mentioned in the bug entry, I think deviation up to 1 should be accepted
for
> GPU shader PreservingPMConversion test.
> PTAL.

There is a canvas2d layout test in blink that verifies that read/write/read
produces the same results for both reads. IIRC other browsers have this
property. Perhaps there is an alternate version of the upm<->pm converting
shaders that would have this property for IMG?

[changjun.yang, 2014-05-13 09:22:41]

On 2014/05/13 08:09:00, bsalomon wrote:
> On 2014/05/12 11:19:53, changjun.yang wrote:
> > As mentioned in the bug entry, I think deviation up to 1 should be accepted
> for
> > GPU shader PreservingPMConversion test.
> > PTAL.
> 
> There is a canvas2d layout test in blink that verifies that read/write/read
> produces the same results for both reads. IIRC other browsers have this
> property. Perhaps there is an alternate version of the upm<->pm converting
> shaders that would have this property for IMG?

Thanks for your comments!
I have tested all the existing 2 pairs of shaders but neither of them could
produce the exact same results on IMG. Luckily the result differences from the
first pair shader are all equal to 1 and I think this error margin should be
acceptable, thus I proposed this patch. An alternate way to resolve this issue
may be implemented by revising the shaders themselves, it would be a little more
complicated then. What's your opinions?

[bsalomon, 2014-05-13 11:12:56]

On 2014/05/13 09:22:41, changjun.yang wrote:
> On 2014/05/13 08:09:00, bsalomon wrote:
> > On 2014/05/12 11:19:53, changjun.yang wrote:
> > > As mentioned in the bug entry, I think deviation up to 1 should be
accepted
> > for
> > > GPU shader PreservingPMConversion test.
> > > PTAL.
> > 
> > There is a canvas2d layout test in blink that verifies that read/write/read
> > produces the same results for both reads. IIRC other browsers have this
> > property. Perhaps there is an alternate version of the upm<->pm converting
> > shaders that would have this property for IMG?
> 
> Thanks for your comments!
> I have tested all the existing 2 pairs of shaders but neither of them could
> produce the exact same results on IMG. Luckily the result differences from the
> first pair shader are all equal to 1 and I think this error margin should be
> acceptable, thus I proposed this patch. An alternate way to resolve this issue
> may be implemented by revising the shaders themselves, it would be a little
more
> complicated then. What's your opinions?

If the shaders can be revised to work (or have alternate shaders for IMG)
without the tolerance that'd be preferable.

[changjun.yang, 2014-05-17 11:26:31]

On 2014/05/13 11:12:56, bsalomon wrote:
> On 2014/05/13 09:22:41, changjun.yang wrote:
> > On 2014/05/13 08:09:00, bsalomon wrote:
> > > On 2014/05/12 11:19:53, changjun.yang wrote:
> > > > As mentioned in the bug entry, I think deviation up to 1 should be
> accepted
> > > for
> > > > GPU shader PreservingPMConversion test.
> > > > PTAL.
> > > 
> > > There is a canvas2d layout test in blink that verifies that
read/write/read
> > > produces the same results for both reads. IIRC other browsers have this
> > > property. Perhaps there is an alternate version of the upm<->pm converting
> > > shaders that would have this property for IMG?
> > 
> > Thanks for your comments!
> > I have tested all the existing 2 pairs of shaders but neither of them could
> > produce the exact same results on IMG. Luckily the result differences from
the
> > first pair shader are all equal to 1 and I think this error margin should be
> > acceptable, thus I proposed this patch. An alternate way to resolve this
issue
> > may be implemented by revising the shaders themselves, it would be a little
> more
> > complicated then. What's your opinions?
> 
> If the shaders can be revised to work (or have alternate shaders for IMG)
> without the tolerance that'd be preferable.

Haven't found a decent way to resolve this issue by revising the shaders or
adding new ones. :(
Unlike Intel GPUs (https://codereview.chromium.org/15719007/), IMG gets more
diff errors (more than 1000). I logged the whole 3 temp value for firstRead,
tempRead, secondRead during PM->UPM->PM conversion but could not identify how to
revise the shaders and make all of the comparisons between firstRead and
secondRead equal since there are too many diffs and behave irregular. Some
typical log looks like:

150 156 firstRead 244 244 244 156
        tmpRead 149 149 149 156
        secondRead 243 243 243 156
155 158 firstRead 249 249 249 158
        tmpRead 154 154 154 158
        secondRead 248 248 248 158
171 173 firstRead 251 251 251 173
        tmpRead 170 170 170 173
        secondRead 250 250 250 173
142 174 firstRead 207 207 207 174
        tmpRead 141 141 141 174
        secondRead 206 206 206 174
...
252 254 firstRead 252 252 252 254
        tmpRead 251 251 251 254
        secondRead 251 251 251 254
253 254 firstRead 253 253 253 254
        tmpRead 252 252 252 254
        secondRead 252 252 252 254
254 254 firstRead 254 254 254 254
        tmpRead 253 253 253 254
        secondRead 253 253 253 254
...

The first 2 columns stand for x and y. There are too many diffs and it is hard
to fix all of them by revising the accuracy without introducing new diffs.
Besides, I found different IMG GPUs may have the different x, y when comparison
diff occurs, although all the diffs equal to 1. While this might increase the
complexity for revising the shaders. 
I think the tolerance way maybe more straightforward for this case. Thoughts?

[bsalomon, 2014-05-20 14:50:22]

On 2014/05/17 11:26:31, changjun.yang wrote:
> On 2014/05/13 11:12:56, bsalomon wrote:
> > On 2014/05/13 09:22:41, changjun.yang wrote:
> > > On 2014/05/13 08:09:00, bsalomon wrote:
> > > > On 2014/05/12 11:19:53, changjun.yang wrote:
> > > > > As mentioned in the bug entry, I think deviation up to 1 should be
> > accepted
> > > > for
> > > > > GPU shader PreservingPMConversion test.
> > > > > PTAL.
> > > > 
> > > > There is a canvas2d layout test in blink that verifies that
> read/write/read
> > > > produces the same results for both reads. IIRC other browsers have this
> > > > property. Perhaps there is an alternate version of the upm<->pm
converting
> > > > shaders that would have this property for IMG?
> > > 
> > > Thanks for your comments!
> > > I have tested all the existing 2 pairs of shaders but neither of them
could
> > > produce the exact same results on IMG. Luckily the result differences from
> the
> > > first pair shader are all equal to 1 and I think this error margin should
be
> > > acceptable, thus I proposed this patch. An alternate way to resolve this
> issue
> > > may be implemented by revising the shaders themselves, it would be a
little
> > more
> > > complicated then. What's your opinions?
> > 
> > If the shaders can be revised to work (or have alternate shaders for IMG)
> > without the tolerance that'd be preferable.
> 
> Haven't found a decent way to resolve this issue by revising the shaders or
> adding new ones. :(
> Unlike Intel GPUs (https://codereview.chromium.org/15719007/), IMG gets more
> diff errors (more than 1000). I logged the whole 3 temp value for firstRead,
> tempRead, secondRead during PM->UPM->PM conversion but could not identify how
to
> revise the shaders and make all of the comparisons between firstRead and
> secondRead equal since there are too many diffs and behave irregular. Some
> typical log looks like:
> 
> 150 156 firstRead 244 244 244 156
>         tmpRead 149 149 149 156
>         secondRead 243 243 243 156
> 155 158 firstRead 249 249 249 158
>         tmpRead 154 154 154 158
>         secondRead 248 248 248 158
> 171 173 firstRead 251 251 251 173
>         tmpRead 170 170 170 173
>         secondRead 250 250 250 173
> 142 174 firstRead 207 207 207 174
>         tmpRead 141 141 141 174
>         secondRead 206 206 206 174
> ...
> 252 254 firstRead 252 252 252 254
>         tmpRead 251 251 251 254
>         secondRead 251 251 251 254
> 253 254 firstRead 253 253 253 254
>         tmpRead 252 252 252 254
>         secondRead 252 252 252 254
> 254 254 firstRead 254 254 254 254
>         tmpRead 253 253 253 254
>         secondRead 253 253 253 254
> ...
> 
> The first 2 columns stand for x and y. There are too many diffs and it is hard
> to fix all of them by revising the accuracy without introducing new diffs.
> Besides, I found different IMG GPUs may have the different x, y when
comparison
> diff occurs, although all the diffs equal to 1. While this might increase the
> complexity for revising the shaders. 
> I think the tolerance way maybe more straightforward for this case. Thoughts?

I don't think we can move forward with a blanket tolerance. I believe there is
an expectation that the read/write/read roundtrip will produce exactly the same
results on the two reads for canvas2D. I suppose we could consider having an
optional mode that allows the tolerance. What is your use case?

[changjun.yang, 2014-05-26 09:10:43]

On 2014/05/20 14:50:22, bsalomon wrote:
> On 2014/05/17 11:26:31, changjun.yang wrote:
> > On 2014/05/13 11:12:56, bsalomon wrote:
> > > On 2014/05/13 09:22:41, changjun.yang wrote:
> > > > On 2014/05/13 08:09:00, bsalomon wrote:
> > > > > On 2014/05/12 11:19:53, changjun.yang wrote:
> > > > > > As mentioned in the bug entry, I think deviation up to 1 should be
> > > accepted
> > > > > for
> > > > > > GPU shader PreservingPMConversion test.
> > > > > > PTAL.
> > > > > 
> > > > > There is a canvas2d layout test in blink that verifies that
> > read/write/read
> > > > > produces the same results for both reads. IIRC other browsers have
this
> > > > > property. Perhaps there is an alternate version of the upm<->pm
> converting
> > > > > shaders that would have this property for IMG?
> > > > 
> > > > Thanks for your comments!
> > > > I have tested all the existing 2 pairs of shaders but neither of them
> could
> > > > produce the exact same results on IMG. Luckily the result differences
from
> > the
> > > > first pair shader are all equal to 1 and I think this error margin
should
> be
> > > > acceptable, thus I proposed this patch. An alternate way to resolve this
> > issue
> > > > may be implemented by revising the shaders themselves, it would be a
> little
> > > more
> > > > complicated then. What's your opinions?
> > > 
> > > If the shaders can be revised to work (or have alternate shaders for IMG)
> > > without the tolerance that'd be preferable.
> > 
> > Haven't found a decent way to resolve this issue by revising the shaders or
> > adding new ones. :(
> > Unlike Intel GPUs (https://codereview.chromium.org/15719007/), IMG gets more
> > diff errors (more than 1000). I logged the whole 3 temp value for firstRead,
> > tempRead, secondRead during PM->UPM->PM conversion but could not identify
how
> to
> > revise the shaders and make all of the comparisons between firstRead and
> > secondRead equal since there are too many diffs and behave irregular. Some
> > typical log looks like:
> > 
> > 150 156 firstRead 244 244 244 156
> >         tmpRead 149 149 149 156
> >         secondRead 243 243 243 156
> > 155 158 firstRead 249 249 249 158
> >         tmpRead 154 154 154 158
> >         secondRead 248 248 248 158
> > 171 173 firstRead 251 251 251 173
> >         tmpRead 170 170 170 173
> >         secondRead 250 250 250 173
> > 142 174 firstRead 207 207 207 174
> >         tmpRead 141 141 141 174
> >         secondRead 206 206 206 174
> > ...
> > 252 254 firstRead 252 252 252 254
> >         tmpRead 251 251 251 254
> >         secondRead 251 251 251 254
> > 253 254 firstRead 253 253 253 254
> >         tmpRead 252 252 252 254
> >         secondRead 252 252 252 254
> > 254 254 firstRead 254 254 254 254
> >         tmpRead 253 253 253 254
> >         secondRead 253 253 253 254
> > ...
> > 
> > The first 2 columns stand for x and y. There are too many diffs and it is
hard
> > to fix all of them by revising the accuracy without introducing new diffs.
> > Besides, I found different IMG GPUs may have the different x, y when
> comparison
> > diff occurs, although all the diffs equal to 1. While this might increase
the
> > complexity for revising the shaders. 
> > I think the tolerance way maybe more straightforward for this case.
Thoughts?
> 
> I don't think we can move forward with a blanket tolerance. I believe there is
> an expectation that the read/write/read roundtrip will produce exactly the
same
> results on the two reads for canvas2D. I suppose we could consider having an
> optional mode that allows the tolerance. What is your use case?

Set precision to high may well resolve this issue. Uploaded Patch Set 2, PTAL.

[bsalomon, 2014-05-27 13:15:16]

On 2014/05/26 09:10:43, changjun.yang wrote:
> On 2014/05/20 14:50:22, bsalomon wrote:
> > On 2014/05/17 11:26:31, changjun.yang wrote:
> > > On 2014/05/13 11:12:56, bsalomon wrote:
> > > > On 2014/05/13 09:22:41, changjun.yang wrote:
> > > > > On 2014/05/13 08:09:00, bsalomon wrote:
> > > > > > On 2014/05/12 11:19:53, changjun.yang wrote:
> > > > > > > As mentioned in the bug entry, I think deviation up to 1 should be
> > > > accepted
> > > > > > for
> > > > > > > GPU shader PreservingPMConversion test.
> > > > > > > PTAL.
> > > > > > 
> > > > > > There is a canvas2d layout test in blink that verifies that
> > > read/write/read
> > > > > > produces the same results for both reads. IIRC other browsers have
> this
> > > > > > property. Perhaps there is an alternate version of the upm<->pm
> > converting
> > > > > > shaders that would have this property for IMG?
> > > > > 
> > > > > Thanks for your comments!
> > > > > I have tested all the existing 2 pairs of shaders but neither of them
> > could
> > > > > produce the exact same results on IMG. Luckily the result differences
> from
> > > the
> > > > > first pair shader are all equal to 1 and I think this error margin
> should
> > be
> > > > > acceptable, thus I proposed this patch. An alternate way to resolve
this
> > > issue
> > > > > may be implemented by revising the shaders themselves, it would be a
> > little
> > > > more
> > > > > complicated then. What's your opinions?
> > > > 
> > > > If the shaders can be revised to work (or have alternate shaders for
IMG)
> > > > without the tolerance that'd be preferable.
> > > 
> > > Haven't found a decent way to resolve this issue by revising the shaders
or
> > > adding new ones. :(
> > > Unlike Intel GPUs (https://codereview.chromium.org/15719007/), IMG gets
more
> > > diff errors (more than 1000). I logged the whole 3 temp value for
firstRead,
> > > tempRead, secondRead during PM->UPM->PM conversion but could not identify
> how
> > to
> > > revise the shaders and make all of the comparisons between firstRead and
> > > secondRead equal since there are too many diffs and behave irregular. Some
> > > typical log looks like:
> > > 
> > > 150 156 firstRead 244 244 244 156
> > >         tmpRead 149 149 149 156
> > >         secondRead 243 243 243 156
> > > 155 158 firstRead 249 249 249 158
> > >         tmpRead 154 154 154 158
> > >         secondRead 248 248 248 158
> > > 171 173 firstRead 251 251 251 173
> > >         tmpRead 170 170 170 173
> > >         secondRead 250 250 250 173
> > > 142 174 firstRead 207 207 207 174
> > >         tmpRead 141 141 141 174
> > >         secondRead 206 206 206 174
> > > ...
> > > 252 254 firstRead 252 252 252 254
> > >         tmpRead 251 251 251 254
> > >         secondRead 251 251 251 254
> > > 253 254 firstRead 253 253 253 254
> > >         tmpRead 252 252 252 254
> > >         secondRead 252 252 252 254
> > > 254 254 firstRead 254 254 254 254
> > >         tmpRead 253 253 253 254
> > >         secondRead 253 253 253 254
> > > ...
> > > 
> > > The first 2 columns stand for x and y. There are too many diffs and it is
> hard
> > > to fix all of them by revising the accuracy without introducing new diffs.
> > > Besides, I found different IMG GPUs may have the different x, y when
> > comparison
> > > diff occurs, although all the diffs equal to 1. While this might increase
> the
> > > complexity for revising the shaders. 
> > > I think the tolerance way maybe more straightforward for this case.
> Thoughts?
> > 
> > I don't think we can move forward with a blanket tolerance. I believe there
is
> > an expectation that the read/write/read roundtrip will produce exactly the
> same
> > results on the two reads for canvas2D. I suppose we could consider having an
> > optional mode that allows the tolerance. What is your use case?
> 
> Set precision to high may well resolve this issue. Uploaded Patch Set 2, PTAL.

That's a pretty heavy hammer to swing at this problem and could have perf
ramifications. Can you figure out which variables in the config conversion
shader need to be highp in order to fix the issue and then make only those vars
highp and only on IMG?

[bsalomon, 2014-05-27 13:24:34]

On 2014/05/27 13:15:16, bsalomon wrote:
> On 2014/05/26 09:10:43, changjun.yang wrote:
> > On 2014/05/20 14:50:22, bsalomon wrote:
> > > On 2014/05/17 11:26:31, changjun.yang wrote:
> > > > On 2014/05/13 11:12:56, bsalomon wrote:
> > > > > On 2014/05/13 09:22:41, changjun.yang wrote:
> > > > > > On 2014/05/13 08:09:00, bsalomon wrote:
> > > > > > > On 2014/05/12 11:19:53, changjun.yang wrote:
> > > > > > > > As mentioned in the bug entry, I think deviation up to 1 should
be
> > > > > accepted
> > > > > > > for
> > > > > > > > GPU shader PreservingPMConversion test.
> > > > > > > > PTAL.
> > > > > > > 
> > > > > > > There is a canvas2d layout test in blink that verifies that
> > > > read/write/read
> > > > > > > produces the same results for both reads. IIRC other browsers have
> > this
> > > > > > > property. Perhaps there is an alternate version of the upm<->pm
> > > converting
> > > > > > > shaders that would have this property for IMG?
> > > > > > 
> > > > > > Thanks for your comments!
> > > > > > I have tested all the existing 2 pairs of shaders but neither of
them
> > > could
> > > > > > produce the exact same results on IMG. Luckily the result
differences
> > from
> > > > the
> > > > > > first pair shader are all equal to 1 and I think this error margin
> > should
> > > be
> > > > > > acceptable, thus I proposed this patch. An alternate way to resolve
> this
> > > > issue
> > > > > > may be implemented by revising the shaders themselves, it would be a
> > > little
> > > > > more
> > > > > > complicated then. What's your opinions?
> > > > > 
> > > > > If the shaders can be revised to work (or have alternate shaders for
> IMG)
> > > > > without the tolerance that'd be preferable.
> > > > 
> > > > Haven't found a decent way to resolve this issue by revising the shaders
> or
> > > > adding new ones. :(
> > > > Unlike Intel GPUs (https://codereview.chromium.org/15719007/), IMG gets
> more
> > > > diff errors (more than 1000). I logged the whole 3 temp value for
> firstRead,
> > > > tempRead, secondRead during PM->UPM->PM conversion but could not
identify
> > how
> > > to
> > > > revise the shaders and make all of the comparisons between firstRead and
> > > > secondRead equal since there are too many diffs and behave irregular.
Some
> > > > typical log looks like:
> > > > 
> > > > 150 156 firstRead 244 244 244 156
> > > >         tmpRead 149 149 149 156
> > > >         secondRead 243 243 243 156
> > > > 155 158 firstRead 249 249 249 158
> > > >         tmpRead 154 154 154 158
> > > >         secondRead 248 248 248 158
> > > > 171 173 firstRead 251 251 251 173
> > > >         tmpRead 170 170 170 173
> > > >         secondRead 250 250 250 173
> > > > 142 174 firstRead 207 207 207 174
> > > >         tmpRead 141 141 141 174
> > > >         secondRead 206 206 206 174
> > > > ...
> > > > 252 254 firstRead 252 252 252 254
> > > >         tmpRead 251 251 251 254
> > > >         secondRead 251 251 251 254
> > > > 253 254 firstRead 253 253 253 254
> > > >         tmpRead 252 252 252 254
> > > >         secondRead 252 252 252 254
> > > > 254 254 firstRead 254 254 254 254
> > > >         tmpRead 253 253 253 254
> > > >         secondRead 253 253 253 254
> > > > ...
> > > > 
> > > > The first 2 columns stand for x and y. There are too many diffs and it
is
> > hard
> > > > to fix all of them by revising the accuracy without introducing new
diffs.
> > > > Besides, I found different IMG GPUs may have the different x, y when
> > > comparison
> > > > diff occurs, although all the diffs equal to 1. While this might
increase
> > the
> > > > complexity for revising the shaders. 
> > > > I think the tolerance way maybe more straightforward for this case.
> > Thoughts?
> > > 
> > > I don't think we can move forward with a blanket tolerance. I believe
there
> is
> > > an expectation that the read/write/read roundtrip will produce exactly the
> > same
> > > results on the two reads for canvas2D. I suppose we could consider having
an
> > > optional mode that allows the tolerance. What is your use case?
> > 
> > Set precision to high may well resolve this issue. Uploaded Patch Set 2,
PTAL.
> 
> That's a pretty heavy hammer to swing at this problem and could have perf
> ramifications. Can you figure out which variables in the config conversion
> shader need to be highp in order to fix the issue and then make only those
vars
> highp and only on IMG?

I should have written "only on IMG and other GPUs where this is known to work."

[changjun.yang, 2014-06-09 12:41:30]

On 2014/05/27 13:24:34, bsalomon wrote:
> On 2014/05/27 13:15:16, bsalomon wrote:
> > On 2014/05/26 09:10:43, changjun.yang wrote:
> > > On 2014/05/20 14:50:22, bsalomon wrote:
> > > > On 2014/05/17 11:26:31, changjun.yang wrote:
> > > > > On 2014/05/13 11:12:56, bsalomon wrote:
> > > > > > On 2014/05/13 09:22:41, changjun.yang wrote:
> > > > > > > On 2014/05/13 08:09:00, bsalomon wrote:
> > > > > > > > On 2014/05/12 11:19:53, changjun.yang wrote:
> > > > > > > > > As mentioned in the bug entry, I think deviation up to 1
should
> be
> > > > > > accepted
> > > > > > > > for
> > > > > > > > > GPU shader PreservingPMConversion test.
> > > > > > > > > PTAL.
> > > > > > > > 
> > > > > > > > There is a canvas2d layout test in blink that verifies that
> > > > > read/write/read
> > > > > > > > produces the same results for both reads. IIRC other browsers
have
> > > this
> > > > > > > > property. Perhaps there is an alternate version of the upm<->pm
> > > > converting
> > > > > > > > shaders that would have this property for IMG?
> > > > > > > 
> > > > > > > Thanks for your comments!
> > > > > > > I have tested all the existing 2 pairs of shaders but neither of
> them
> > > > could
> > > > > > > produce the exact same results on IMG. Luckily the result
> differences
> > > from
> > > > > the
> > > > > > > first pair shader are all equal to 1 and I think this error margin
> > > should
> > > > be
> > > > > > > acceptable, thus I proposed this patch. An alternate way to
resolve
> > this
> > > > > issue
> > > > > > > may be implemented by revising the shaders themselves, it would be
a
> > > > little
> > > > > > more
> > > > > > > complicated then. What's your opinions?
> > > > > > 
> > > > > > If the shaders can be revised to work (or have alternate shaders for
> > IMG)
> > > > > > without the tolerance that'd be preferable.
> > > > > 
> > > > > Haven't found a decent way to resolve this issue by revising the
shaders
> > or
> > > > > adding new ones. :(
> > > > > Unlike Intel GPUs (https://codereview.chromium.org/15719007/), IMG
gets
> > more
> > > > > diff errors (more than 1000). I logged the whole 3 temp value for
> > firstRead,
> > > > > tempRead, secondRead during PM->UPM->PM conversion but could not
> identify
> > > how
> > > > to
> > > > > revise the shaders and make all of the comparisons between firstRead
and
> > > > > secondRead equal since there are too many diffs and behave irregular.
> Some
> > > > > typical log looks like:
> > > > > 
> > > > > 150 156 firstRead 244 244 244 156
> > > > >         tmpRead 149 149 149 156
> > > > >         secondRead 243 243 243 156
> > > > > 155 158 firstRead 249 249 249 158
> > > > >         tmpRead 154 154 154 158
> > > > >         secondRead 248 248 248 158
> > > > > 171 173 firstRead 251 251 251 173
> > > > >         tmpRead 170 170 170 173
> > > > >         secondRead 250 250 250 173
> > > > > 142 174 firstRead 207 207 207 174
> > > > >         tmpRead 141 141 141 174
> > > > >         secondRead 206 206 206 174
> > > > > ...
> > > > > 252 254 firstRead 252 252 252 254
> > > > >         tmpRead 251 251 251 254
> > > > >         secondRead 251 251 251 254
> > > > > 253 254 firstRead 253 253 253 254
> > > > >         tmpRead 252 252 252 254
> > > > >         secondRead 252 252 252 254
> > > > > 254 254 firstRead 254 254 254 254
> > > > >         tmpRead 253 253 253 254
> > > > >         secondRead 253 253 253 254
> > > > > ...
> > > > > 
> > > > > The first 2 columns stand for x and y. There are too many diffs and it
> is
> > > hard
> > > > > to fix all of them by revising the accuracy without introducing new
> diffs.
> > > > > Besides, I found different IMG GPUs may have the different x, y when
> > > > comparison
> > > > > diff occurs, although all the diffs equal to 1. While this might
> increase
> > > the
> > > > > complexity for revising the shaders. 
> > > > > I think the tolerance way maybe more straightforward for this case.
> > > Thoughts?
> > > > 
> > > > I don't think we can move forward with a blanket tolerance. I believe
> there
> > is
> > > > an expectation that the read/write/read roundtrip will produce exactly
the
> > > same
> > > > results on the two reads for canvas2D. I suppose we could consider
having
> an
> > > > optional mode that allows the tolerance. What is your use case?
> > > 
> > > Set precision to high may well resolve this issue. Uploaded Patch Set 2,
> PTAL.
> > 
> > That's a pretty heavy hammer to swing at this problem and could have perf
> > ramifications. Can you figure out which variables in the config conversion
> > shader need to be highp in order to fix the issue and then make only those
> vars
> > highp and only on IMG?
> 
> I should have written "only on IMG and other GPUs where this is known to
work."

Sorry for a delayed response.
Uploaded patch set 3 which only sets PowerVR Rogue Hood to highp, PTAL.
However, looks skia under Chromium couldn't get the correct GPU vendor info,
filed another bug here:
https://code.google.com/p/skia/issues/detail?id=2652&thanks=2652&ts=1402302971

[bsalomon, 2014-06-09 13:56:45]

https://codereview.chromium.org/277323002/diff/40001/src/gpu/gl/GrGLShaderBui...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/277323002/diff/40001/src/gpu/gl/GrGLShaderBui...
src/gpu/gl/GrGLShaderBuilder.cpp:699:
append_default_precision_qualifier(GrGLShaderVar::kHigh_Precision,
Can you not just apply highp to the vars emitted by the config conversion
effect? Does this make any other shaders work better?

[changjun.yang, 2014-06-10 11:41:17]

https://codereview.chromium.org/277323002/diff/40001/src/gpu/gl/GrGLShaderBui...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/277323002/diff/40001/src/gpu/gl/GrGLShaderBui...
src/gpu/gl/GrGLShaderBuilder.cpp:532: this->fsCodeAppendf("\tvec4 %s = %s;\n",
inColorName.c_str(), inColor.c_str());
Sorry I am not quite familiar with the shader..
One possible way maybe overwrite this to "highp vec4" after the specific shader
being constructed? If so, could you please guide me more on how to implement
this overwrite?
Another way maybe pass a specific shader related "parameter" here to identify if
it is needed to switch to "highp vec4"? If so, what "parameter" would you
prefer?
Thanks a lot in advance!

https://codereview.chromium.org/277323002/diff/40001/src/gpu/gl/GrGLShaderBui...
src/gpu/gl/GrGLShaderBuilder.cpp:699:
append_default_precision_qualifier(GrGLShaderVar::kHigh_Precision,
On 2014/06/09 13:56:45, bsalomon wrote:
> Can you not just apply highp to the vars emitted by the config conversion
> effect? Does this make any other shaders work better?
Thanks! Pls see my comments above.

[bsalomon, 2014-06-10 13:22:15]

https://codereview.chromium.org/277323002/diff/40001/src/gpu/gl/GrGLShaderBui...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/277323002/diff/40001/src/gpu/gl/GrGLShaderBui...
src/gpu/gl/GrGLShaderBuilder.cpp:532: this->fsCodeAppendf("\tvec4 %s = %s;\n",
inColorName.c_str(), inColor.c_str());
On 2014/06/10 11:41:16, changjun.yang wrote:
> Sorry I am not quite familiar with the shader..
> One possible way maybe overwrite this to "highp vec4" after the specific
shader
> being constructed? If so, could you please guide me more on how to implement
> this overwrite?
> Another way maybe pass a specific shader related "parameter" here to identify
if
> it is needed to switch to "highp vec4"? If so, what "parameter" would you
> prefer?
> Thanks a lot in advance!

Question: Is this the variable that needs to be highp to make the conversion
work? Or does it work to make a highp temporary var in
GrGLConfigConversionEffect::emitCode? Also, the GPU in question supports
integers, so perhaps we could write the effect using integer operations when
they are supported.

[bsalomon, 2014-06-10 13:22:18]

https://codereview.chromium.org/277323002/diff/40001/src/gpu/gl/GrGLShaderBui...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/277323002/diff/40001/src/gpu/gl/GrGLShaderBui...
src/gpu/gl/GrGLShaderBuilder.cpp:532: this->fsCodeAppendf("\tvec4 %s = %s;\n",
inColorName.c_str(), inColor.c_str());
On 2014/06/10 11:41:16, changjun.yang wrote:
> Sorry I am not quite familiar with the shader..
> One possible way maybe overwrite this to "highp vec4" after the specific
shader
> being constructed? If so, could you please guide me more on how to implement
> this overwrite?
> Another way maybe pass a specific shader related "parameter" here to identify
if
> it is needed to switch to "highp vec4"? If so, what "parameter" would you
> prefer?
> Thanks a lot in advance!

Question: Is this the variable that needs to be highp to make the conversion
work? Or does it work to make a highp temporary var in
GrGLConfigConversionEffect::emitCode? Also, the GPU in question supports
integers, so perhaps we could write the effect using integer operations when
they are supported.

[changjun.yang, 2014-06-11 08:56:56]

https://codereview.chromium.org/277323002/diff/40001/src/gpu/gl/GrGLShaderBui...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/277323002/diff/40001/src/gpu/gl/GrGLShaderBui...
src/gpu/gl/GrGLShaderBuilder.cpp:532: this->fsCodeAppendf("\tvec4 %s = %s;\n",
inColorName.c_str(), inColor.c_str());
On 2014/06/10 13:22:18, bsalomon wrote:
> On 2014/06/10 11:41:16, changjun.yang wrote:
> > Sorry I am not quite familiar with the shader..
> > One possible way maybe overwrite this to "highp vec4" after the specific
> shader
> > being constructed? If so, could you please guide me more on how to implement
> > this overwrite?
> > Another way maybe pass a specific shader related "parameter" here to
identify
> if
> > it is needed to switch to "highp vec4"? If so, what "parameter" would you
> > prefer?
> > Thanks a lot in advance!
> 
> Question: Is this the variable that needs to be highp to make the conversion
> work? Or does it work to make a highp temporary var in
> GrGLConfigConversionEffect::emitCode? Also, the GPU in question supports
> integers, so perhaps we could write the effect using integer operations when
> they are supported.
> 

Yes, it's the var that would make the conversion work if being made highp, but
not to make a temp var only in GrGLConfigConversionEffect::emitCode I think. I
may check the side effects to other shaders if we make this var to highp only
for specific IMG GPUs.

[changjun.yang, 2014-07-01 09:01:15]

On 2014/06/11 08:56:56, changjun.yang wrote:
>
https://codereview.chromium.org/277323002/diff/40001/src/gpu/gl/GrGLShaderBui...
> File src/gpu/gl/GrGLShaderBuilder.cpp (right):
> 
>
https://codereview.chromium.org/277323002/diff/40001/src/gpu/gl/GrGLShaderBui...
> src/gpu/gl/GrGLShaderBuilder.cpp:532: this->fsCodeAppendf("\tvec4 %s = %s;\n",
> inColorName.c_str(), inColor.c_str());
> On 2014/06/10 13:22:18, bsalomon wrote:
> > On 2014/06/10 11:41:16, changjun.yang wrote:
> > > Sorry I am not quite familiar with the shader..
> > > One possible way maybe overwrite this to "highp vec4" after the specific
> > shader
> > > being constructed? If so, could you please guide me more on how to
implement
> > > this overwrite?
> > > Another way maybe pass a specific shader related "parameter" here to
> identify
> > if
> > > it is needed to switch to "highp vec4"? If so, what "parameter" would you
> > > prefer?
> > > Thanks a lot in advance!
> > 
> > Question: Is this the variable that needs to be highp to make the conversion
> > work? Or does it work to make a highp temporary var in
> > GrGLConfigConversionEffect::emitCode? Also, the GPU in question supports
> > integers, so perhaps we could write the effect using integer operations when
> > they are supported.
> > 
> 
> Yes, it's the var that would make the conversion work if being made highp, but
> not to make a temp var only in GrGLConfigConversionEffect::emitCode I think. I
> may check the side effects to other shaders if we make this var to highp only
> for specific IMG GPUs.

Hi bsalomon,

Sorry to make you have context switch back after a long time.
From section 4.5.2 of the GLSL spec
(http://www.khronos.org/registry/gles/specs/2.0/GLSL_ES_Specification_1.0.17.pdf),
we may see that it is hard to convert a var from mediump to highp. Thus I
uploaded Patch Set 4, which makes the least impact to others but gets GPU
acceleration for the UPM/PM conversion the specific IMG devices. Could we try to
accept this?

[bsalomon, 2014-07-01 13:54:52]

https://codereview.chromium.org/277323002/diff/80001/src/gpu/gl/GrGLShaderBui...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/277323002/diff/80001/src/gpu/gl/GrGLShaderBui...
src/gpu/gl/GrGLShaderBuilder.cpp:541: kPVRRogueHood_GrGLRenderer ==
this->ctxInfo().renderer())
Can't you have a temporary highp var in the config conversion effect where you
compute the result and then assign that to the mediump output var at the end of
the effect. Does that not work?


vec4 output_StageN;
{
   highp vec4 outputH = // config conversion stuff, texture lookup etc.

   output_StageN = outputH
}

[changjun.yang, 2014-07-02 14:24:17]

On 2014/07/01 13:54:52, bsalomon wrote:
>
https://codereview.chromium.org/277323002/diff/80001/src/gpu/gl/GrGLShaderBui...
> File src/gpu/gl/GrGLShaderBuilder.cpp (right):
> 
>
https://codereview.chromium.org/277323002/diff/80001/src/gpu/gl/GrGLShaderBui...
> src/gpu/gl/GrGLShaderBuilder.cpp:541: kPVRRogueHood_GrGLRenderer ==
> this->ctxInfo().renderer())
> Can't you have a temporary highp var in the config conversion effect where you
> compute the result and then assign that to the mediump output var at the end
of
> the effect. Does that not work?
> 
> 
> vec4 output_StageN;
> {
>    highp vec4 outputH = // config conversion stuff, texture lookup etc.
> 
>    output_StageN = outputH
> }

Yeah, it should work. Busy today, will update a new patch soon.
Thanks a lot!

[changjun.yang, 2014-07-03 13:35:33]

On 2014/07/02 14:24:17, changjun.yang wrote:
> On 2014/07/01 13:54:52, bsalomon wrote:
> >
>
https://codereview.chromium.org/277323002/diff/80001/src/gpu/gl/GrGLShaderBui...
> > File src/gpu/gl/GrGLShaderBuilder.cpp (right):
> > 
> >
>
https://codereview.chromium.org/277323002/diff/80001/src/gpu/gl/GrGLShaderBui...
> > src/gpu/gl/GrGLShaderBuilder.cpp:541: kPVRRogueHood_GrGLRenderer ==
> > this->ctxInfo().renderer())
> > Can't you have a temporary highp var in the config conversion effect where
you
> > compute the result and then assign that to the mediump output var at the end
> of
> > the effect. Does that not work?
> > 
> > 
> > vec4 output_StageN;
> > {
> >    highp vec4 outputH = // config conversion stuff, texture lookup etc.
> > 
> >    output_StageN = outputH
> > }
> 
> Yeah, it should work. Busy today, will update a new patch soon.
> Thanks a lot!

Adding new hiphp shaders which should make the GPU work without introducing side
effects.
PTAL

[bsalomon, 2014-07-07 13:21:39]

Thanks for revising the change! I have two small comments, otherwise looks good.

https://codereview.chromium.org/277323002/diff/100001/src/gpu/effects/GrConfi...
File src/gpu/effects/GrConfigConversionEffect.cpp (right):

https://codereview.chromium.org/277323002/diff/100001/src/gpu/effects/GrConfi...
src/gpu/effects/GrConfigConversionEffect.cpp:42: builder->fsCodeAppendf("\t\t%s
= %s;\n", outputColorH, outputColor);
Can we only emit this if one of the highp shaders is in use?

https://codereview.chromium.org/277323002/diff/100001/src/gpu/effects/GrConfi...
File src/gpu/effects/GrConfigConversionEffect.h (right):

https://codereview.chromium.org/277323002/diff/100001/src/gpu/effects/GrConfi...
src/gpu/effects/GrConfigConversionEffect.h:33:
kMulByAlpha_RoundUp_PMConversion_HIGHP,
To match our style, this should be something like
kMulByAlpha_RoundUp_Highp_PMConversion. The "_PMConversion" should be at the end
of the name.

[changjun.yang, 2014-07-08 02:33:59]

Thanks!

https://codereview.chromium.org/277323002/diff/100001/src/gpu/effects/GrConfi...
File src/gpu/effects/GrConfigConversionEffect.cpp (right):

https://codereview.chromium.org/277323002/diff/100001/src/gpu/effects/GrConfi...
src/gpu/effects/GrConfigConversionEffect.cpp:42: builder->fsCodeAppendf("\t\t%s
= %s;\n", outputColorH, outputColor);
On 2014/07/07 13:21:39, bsalomon wrote:
> Can we only emit this if one of the highp shaders is in use?

Done.

https://codereview.chromium.org/277323002/diff/100001/src/gpu/effects/GrConfi...
File src/gpu/effects/GrConfigConversionEffect.h (right):

https://codereview.chromium.org/277323002/diff/100001/src/gpu/effects/GrConfi...
src/gpu/effects/GrConfigConversionEffect.h:33:
kMulByAlpha_RoundUp_PMConversion_HIGHP,
On 2014/07/07 13:21:39, bsalomon wrote:
> To match our style, this should be something like
> kMulByAlpha_RoundUp_Highp_PMConversion. The "_PMConversion" should be at the
end
> of the name.

Done.

[bsalomon, 2014-07-08 13:10:15]

https://codereview.chromium.org/277323002/diff/140001/src/gpu/effects/GrConfi...
File src/gpu/effects/GrConfigConversionEffect.cpp (right):

https://codereview.chromium.org/277323002/diff/140001/src/gpu/effects/GrConfi...
src/gpu/effects/GrConfigConversionEffect.cpp:33: builder->fsCodeAppendf("\thighp
vec4 %s;\n", outputColorH);
It looks like we're still always declaring this variable even if it isn't used.
Can you move this into the four cases below that need it?

[changjun.yang, 2014-07-09 03:02:10]

https://codereview.chromium.org/277323002/diff/140001/src/gpu/effects/GrConfi...
File src/gpu/effects/GrConfigConversionEffect.cpp (right):

https://codereview.chromium.org/277323002/diff/140001/src/gpu/effects/GrConfi...
src/gpu/effects/GrConfigConversionEffect.cpp:33: builder->fsCodeAppendf("\thighp
vec4 %s;\n", outputColorH);
On 2014/07/08 13:10:15, bsalomon wrote:
> It looks like we're still always declaring this variable even if it isn't
used.
> Can you move this into the four cases below that need it?

Done. And looks we still need the outputColorH though.

[bsalomon, 2014-07-09 13:20:25]

lgtm

[commit-bot: I haz the power, 2014-07-09 13:21:09]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/changjun.yang@intel.com/277323002/160001

[commit-bot: I haz the power, 2014-07-09 13:47:32]

Change committed as 93cf46f7d687128d2ad05762bd65fea6c4539553

[bsalomon, 2014-07-09 15:04:06]

A revert of this CL has been created in
https://codereview.chromium.org/377263003/ by bsalomon@google.com.

The reason for reverting is: Need to only use the highp variations on GLES
contexts not GL contexts.

http://108.170.220.120:10117/builders/Test-Mac10.6-MacMini4.1-GeForce320M-x86...
.

[changjun.yang, 2014-07-10 08:33:49]

On 2014/07/09 15:04:06, bsalomon wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/377263003/ by mailto:bsalomon@google.com.
> 
> The reason for reverting is: Need to only use the highp variations on GLES
> contexts not GL contexts.
> 
>
http://108.170.220.120:10117/builders/Test-Mac10.6-MacMini4.1-GeForce320M-x86...
> .

Oops..How about making it work just for Android? Pls review patch set 8.

[bsalomon, 2014-07-10 13:09:53]

On 2014/07/10 08:33:49, changjun.yang wrote:
> On 2014/07/09 15:04:06, bsalomon wrote:
> > A revert of this CL has been created in
> > https://codereview.chromium.org/377263003/ by mailto:bsalomon@google.com.
> > 
> > The reason for reverting is: Need to only use the highp variations on GLES
> > contexts not GL contexts.
> > 
> >
>
http://108.170.220.120:10117/builders/Test-Mac10.6-MacMini4.1-GeForce320M-x86...
> > .
> 
> Oops..How about making it work just for Android? Pls review patch set 8.

I don't think that's right. It's really about GLES vs GL not Android vs other
OSs. You can distinguish them using (something like)
builder->ctxInfo().standard() which will either be kGL_GrGLStandard or
kGLES_GrGLStandard.

[changjun.yang, 2014-07-11 05:32:45]

On 2014/07/10 13:09:53, bsalomon wrote:
> On 2014/07/10 08:33:49, changjun.yang wrote:
> > On 2014/07/09 15:04:06, bsalomon wrote:
> > > A revert of this CL has been created in
> > > https://codereview.chromium.org/377263003/ by mailto:bsalomon@google.com.
> > > 
> > > The reason for reverting is: Need to only use the highp variations on GLES
> > > contexts not GL contexts.
> > > 
> > >
> >
>
http://108.170.220.120:10117/builders/Test-Mac10.6-MacMini4.1-GeForce320M-x86...
> > > .
> > 
> > Oops..How about making it work just for Android? Pls review patch set 8.
> 
> I don't think that's right. It's really about GLES vs GL not Android vs other
> OSs. You can distinguish them using (something like)
> builder->ctxInfo().standard() which will either be kGL_GrGLStandard or
> kGLES_GrGLStandard.

Done. And I think maybe we could leave the Android check here since this issue
was identified under Android and thus would bring no effects for other
platforms.
PTAL

[changjun.yang, 2014-07-21 07:06:19]

On 2014/07/11 05:32:45, changjun.yang wrote:
> On 2014/07/10 13:09:53, bsalomon wrote:
> > On 2014/07/10 08:33:49, changjun.yang wrote:
> > > On 2014/07/09 15:04:06, bsalomon wrote:
> > > > A revert of this CL has been created in
> > > > https://codereview.chromium.org/377263003/ by
mailto:bsalomon@google.com.
> > > > 
> > > > The reason for reverting is: Need to only use the highp variations on
GLES
> > > > contexts not GL contexts.
> > > > 
> > > >
> > >
> >
>
http://108.170.220.120:10117/builders/Test-Mac10.6-MacMini4.1-GeForce320M-x86...
> > > > .
> > > 
> > > Oops..How about making it work just for Android? Pls review patch set 8.
> > 
> > I don't think that's right. It's really about GLES vs GL not Android vs
other
> > OSs. You can distinguish them using (something like)
> > builder->ctxInfo().standard() which will either be kGL_GrGLStandard or
> > kGLES_GrGLStandard.
> 
> Done. And I think maybe we could leave the Android check here since this issue
> was identified under Android and thus would bring no effects for other
> platforms.
> PTAL

Ping..While do I need to create a new CL since this one has been reverted and
closed?

[changjun.yang, 2014-07-21 07:12:48]

Ping..

[bsalomon, 2014-07-21 14:23:12]

On 2014/07/21 07:12:48, changjun.yang wrote:
> Ping..

Sorry was out of office. The CL can be reopened if you go to the Edit Issue page
using the link on the left.

With this change do will we wind up testing the non-highp versions twice on
non-GLES devices?

Maybe we could do something like:

    static const PMConversion kConversionRules[][2] = {
        {kDivByAlpha_RoundDown_PMConversion, kMulByAlpha_RoundUp_PMConversion},
        {kDivByAlpha_RoundUp_PMConversion, kMulByAlpha_RoundDown_PMConversion},
        {kDivByAlpha_RoundDown_HIGHP_PMConversion,
kMulByAlpha_RoundUp_HIGHP_PMConversion},
        {kDivByAlpha_RoundUp_HIGHP_PMConversion,
kMulByAlpha_RoundDown_HIGHP_PMConversion},
    };

    // highp only has significance in GLES.
    int conversionRulesCnt = (kGL_GrGLStandard == this->glStandard()) ? 2 : 4;

    GrContext::AutoWideOpenIdentityDraw awoid(context, NULL);

    bool failed = true;

    for (size_t i = 0; i < conversionRulesCnt && failed; ++i) {
    }

GrConfigConversionEffect::TestCreate would also have to be updated to only use
the highp version when on GLES, too.

[changjun.yang, 2014-07-22 13:08:02]

On 2014/07/21 14:23:12, bsalomon wrote:
> On 2014/07/21 07:12:48, changjun.yang wrote:
> > Ping..
> 
> Sorry was out of office. The CL can be reopened if you go to the Edit Issue
page
> using the link on the left.
> 
> With this change do will we wind up testing the non-highp versions twice on
> non-GLES devices?
> 
> Maybe we could do something like:
> 
>     static const PMConversion kConversionRules[][2] = {
>         {kDivByAlpha_RoundDown_PMConversion,
kMulByAlpha_RoundUp_PMConversion},
>         {kDivByAlpha_RoundUp_PMConversion,
kMulByAlpha_RoundDown_PMConversion},
>         {kDivByAlpha_RoundDown_HIGHP_PMConversion,
> kMulByAlpha_RoundUp_HIGHP_PMConversion},
>         {kDivByAlpha_RoundUp_HIGHP_PMConversion,
> kMulByAlpha_RoundDown_HIGHP_PMConversion},
>     };
> 
>     // highp only has significance in GLES.
>     int conversionRulesCnt = (kGL_GrGLStandard == this->glStandard()) ? 2 : 4;
> 
>     GrContext::AutoWideOpenIdentityDraw awoid(context, NULL);
> 
>     bool failed = true;
> 
>     for (size_t i = 0; i < conversionRulesCnt && failed; ++i) {
>     }
> 
> GrConfigConversionEffect::TestCreate would also have to be updated to only use
> the highp version when on GLES, too.

Done. Thanks for the review!

[changjun.yang, 2014-07-29 08:27:32]

On 2014/07/22 13:08:02, changjun.yang wrote:
> On 2014/07/21 14:23:12, bsalomon wrote:
> > On 2014/07/21 07:12:48, changjun.yang wrote:
> > > Ping..
> > 
> > Sorry was out of office. The CL can be reopened if you go to the Edit Issue
> page
> > using the link on the left.
> > 
> > With this change do will we wind up testing the non-highp versions twice on
> > non-GLES devices?
> > 
> > Maybe we could do something like:
> > 
> >     static const PMConversion kConversionRules[][2] = {
> >         {kDivByAlpha_RoundDown_PMConversion,
> kMulByAlpha_RoundUp_PMConversion},
> >         {kDivByAlpha_RoundUp_PMConversion,
> kMulByAlpha_RoundDown_PMConversion},
> >         {kDivByAlpha_RoundDown_HIGHP_PMConversion,
> > kMulByAlpha_RoundUp_HIGHP_PMConversion},
> >         {kDivByAlpha_RoundUp_HIGHP_PMConversion,
> > kMulByAlpha_RoundDown_HIGHP_PMConversion},
> >     };
> > 
> >     // highp only has significance in GLES.
> >     int conversionRulesCnt = (kGL_GrGLStandard == this->glStandard()) ? 2 :
4;
> > 
> >     GrContext::AutoWideOpenIdentityDraw awoid(context, NULL);
> > 
> >     bool failed = true;
> > 
> >     for (size_t i = 0; i < conversionRulesCnt && failed; ++i) {
> >     }
> > 
> > GrConfigConversionEffect::TestCreate would also have to be updated to only
use
> > the highp version when on GLES, too.
> 
> Done. Thanks for the review!

Ping. :)

[changjun.yang, 2014-08-12 01:17:02]

On 2014/07/21 14:23:12, bsalomon wrote:
> On 2014/07/21 07:12:48, changjun.yang wrote:
> > Ping..
> 
> Sorry was out of office. The CL can be reopened if you go to the Edit Issue
page
> using the link on the left.
> 
> With this change do will we wind up testing the non-highp versions twice on
> non-GLES devices?
> 
> Maybe we could do something like:
> 
>     static const PMConversion kConversionRules[][2] = {
>         {kDivByAlpha_RoundDown_PMConversion,
kMulByAlpha_RoundUp_PMConversion},
>         {kDivByAlpha_RoundUp_PMConversion,
kMulByAlpha_RoundDown_PMConversion},
>         {kDivByAlpha_RoundDown_HIGHP_PMConversion,
> kMulByAlpha_RoundUp_HIGHP_PMConversion},
>         {kDivByAlpha_RoundUp_HIGHP_PMConversion,
> kMulByAlpha_RoundDown_HIGHP_PMConversion},
>     };
> 
>     // highp only has significance in GLES.
>     int conversionRulesCnt = (kGL_GrGLStandard == this->glStandard()) ? 2 : 4;
> 
>     GrContext::AutoWideOpenIdentityDraw awoid(context, NULL);
> 
>     bool failed = true;
> 
>     for (size_t i = 0; i < conversionRulesCnt && failed; ++i) {
>     }
> 
> GrConfigConversionEffect::TestCreate would also have to be updated to only use
> the highp version when on GLES, too.

Ping once more. Thanks! :)

[bsalomon, 2014-08-13 00:56:47]

Hi, Sorry for the long delay. I think this has gone in the wrong direction :(
With the latest patch I'm realizing that we're exposing GL-specific details in
the non-GL code and that led to the downcast GrGpu to GrGpuGL, which we
definitely don't want.

Here is how I think we should fix it: Go back to the original set of enum values
and either always use highp for the intermediate computation when on ES, or
detect IMG (or specific IMG GPUs) and always use highp on with that
vendor/renderer string.

[changjun.yang, 2014-08-13 08:59:37]

On 2014/08/13 00:56:47, bsalomon wrote:
> Hi, Sorry for the long delay. I think this has gone in the wrong direction :(
> With the latest patch I'm realizing that we're exposing GL-specific details in
> the non-GL code and that led to the downcast GrGpu to GrGpuGL, which we
> definitely don't want.
> 
> Here is how I think we should fix it: Go back to the original set of enum
values
> and either always use highp for the intermediate computation when on ES, or
> detect IMG (or specific IMG GPUs) and always use highp on with that
> vendor/renderer string.

Thanks for your comments. 
Maybe we could do something like previous Patch Set 4? And also I think detect
GLES would be better than detecting the specific GPU since the vendor/renderer
info is not yet available under Chromium.

[bsalomon, 2014-08-14 15:14:34]

On 2014/08/13 08:59:37, changjun.yang wrote:
> On 2014/08/13 00:56:47, bsalomon wrote:
> > Hi, Sorry for the long delay. I think this has gone in the wrong direction
:(
> > With the latest patch I'm realizing that we're exposing GL-specific details
in
> > the non-GL code and that led to the downcast GrGpu to GrGpuGL, which we
> > definitely don't want.
> > 
> > Here is how I think we should fix it: Go back to the original set of enum
> values
> > and either always use highp for the intermediate computation when on ES, or
> > detect IMG (or specific IMG GPUs) and always use highp on with that
> > vendor/renderer string.
> 
> Thanks for your comments. 
> Maybe we could do something like previous Patch Set 4? And also I think detect
> GLES would be better than detecting the specific GPU since the vendor/renderer
> info is not yet available under Chromium.

Yes like that, but as in the latter patchsets, using a temporary in
GrGLConfigConversionEffect rather than modifying GrGLShaderBuilder.

[changjun.yang, 2014-08-18 12:44:26]

On 2014/08/14 15:14:34, bsalomon wrote:
> On 2014/08/13 08:59:37, changjun.yang wrote:
> > On 2014/08/13 00:56:47, bsalomon wrote:
> > > Hi, Sorry for the long delay. I think this has gone in the wrong direction
> :(
> > > With the latest patch I'm realizing that we're exposing GL-specific
details
> in
> > > the non-GL code and that led to the downcast GrGpu to GrGpuGL, which we
> > > definitely don't want.
> > > 
> > > Here is how I think we should fix it: Go back to the original set of enum
> > values
> > > and either always use highp for the intermediate computation when on ES,
or
> > > detect IMG (or specific IMG GPUs) and always use highp on with that
> > > vendor/renderer string.
> > 
> > Thanks for your comments. 
> > Maybe we could do something like previous Patch Set 4? And also I think
detect
> > GLES would be better than detecting the specific GPU since the
vendor/renderer
> > info is not yet available under Chromium.
> 
> Yes like that, but as in the latter patchsets, using a temporary in
> GrGLConfigConversionEffect rather than modifying GrGLShaderBuilder.

Done. May still need some improvement.

[bsalomon, 2014-08-19 14:52:13]

lgtm. BTW the tabs and newlines aren't really needed anymore. The shaders go
through a pretty printer.

[changjun.yang, 2014-08-20 01:17:52]

On 2014/08/19 14:52:13, bsalomon wrote:
> lgtm. BTW the tabs and newlines aren't really needed anymore. The shaders go
> through a pretty printer.

Done. Thanks!

[changjun.yang, 2014-08-20 01:18:00]

The CQ bit was checked by changjun.yang@intel.com

[commit-bot: I haz the power, 2014-08-20 01:18:24]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/changjun.yang@intel.com/277323002/420001

[commit-bot: I haz the power, 2014-08-20 01:24:35]

Committed patchset #15 (420001) as cecc91c4446c9c2f9b95736e206bfd1d507a75a9