Workaround for PowerVR clear issue.

Workaround for PowerVR clear issue.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/63b21962867af0f98e12a3ccbe5eef76b7ecc3aa

[bsalomon, 2014-11-04 18:42:29]

bsalomon@google.com changed reviewers:
+ joshualitt@google.com, robertphillips@google.com

[bsalomon, 2014-11-04 18:43:35]

On 2014/11/04 18:42:30, bsalomon wrote:

This isn't showing all the diffs... hold off until I figure that out.

[bsalomon, 2014-11-04 19:12:45]

Ok, it's now using the correct baseline.

[yunchao, 2014-11-05 06:38:46]

On 2014/11/04 19:12:45, bsalomon wrote:
> Ok, it's now using the correct baseline.

This fix is interesting. Brian, is there a corresponding bug?

[robertphillips, 2014-11-05 14:23:00]

lgtm modulo strcmp issue

https://codereview.chromium.org/701573002/diff/80001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/701573002/diff/80001/src/gpu/GrDrawTarget.cpp...
src/gpu/GrDrawTarget.cpp:559: }
// We cannot just set canIgnoreRect to false and fall through to onClear b.c.
that will devolve to a clear and the bug requires something to actually be
drawn.

https://codereview.chromium.org/701573002/diff/80001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/701573002/diff/80001/src/gpu/GrDrawTarget.h#n...
src/gpu/GrDrawTarget.h:858: virtual void onClear(const SkIRect* rect, GrColor
color, bool canIgnoreRect,
add a space ?

https://codereview.chromium.org/701573002/diff/80001/src/gpu/gl/GrGLUtil.cpp
File src/gpu/gl/GrGLUtil.cpp (right):

https://codereview.chromium.org/701573002/diff/80001/src/gpu/gl/GrGLUtil.cpp#...
src/gpu/gl/GrGLUtil.cpp:211: }
0 == or !strcmp ?

[bsalomon, 2014-11-05 14:56:47]

https://codereview.chromium.org/701573002/diff/80001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/701573002/diff/80001/src/gpu/GrDrawTarget.cpp...
src/gpu/GrDrawTarget.cpp:559: }
On 2014/11/05 14:22:59, robertphillips wrote:
> // We cannot just set canIgnoreRect to false and fall through to onClear b.c.
> that will devolve to a clear and the bug requires something to actually be
> drawn.
Combined this with the comment above (right after the if).

https://codereview.chromium.org/701573002/diff/80001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/701573002/diff/80001/src/gpu/GrDrawTarget.h#n...
src/gpu/GrDrawTarget.h:858: virtual void onClear(const SkIRect* rect, GrColor
color, bool canIgnoreRect,
On 2014/11/05 14:22:59, robertphillips wrote:
> add a space ?

Done.

https://codereview.chromium.org/701573002/diff/80001/src/gpu/gl/GrGLUtil.cpp
File src/gpu/gl/GrGLUtil.cpp (right):

https://codereview.chromium.org/701573002/diff/80001/src/gpu/gl/GrGLUtil.cpp#...
src/gpu/gl/GrGLUtil.cpp:211: }
On 2014/11/05 14:23:00, robertphillips wrote:
> 0 == or !strcmp ?

Oh, duh. Thought I had fixed this locally, but must have been lost in the
shuffle.
Changed to:

        static const char kPowerVRRogueStr = "PowerVR Rogue";
        if (0 == strncmp(rendererString, kPowerVRRogueStr,
                         SK_ARRAY_COUNT(kPowerVRRogueStr)-1)) {
            return kPowerVRRogue_GrGLRenderer;
        }

since there may be more to the str than just "PowerVR Rogue"

[bsalomon, 2014-11-05 14:56:55]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-11-05 14:57:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701573002/120001

[commit-bot: I haz the power, 2014-11-05 15:05:39]

Committed patchset #7 (id:120001) as 63b21962867af0f98e12a3ccbe5eef76b7ecc3aa

[bsalomon, 2014-11-05 15:28:05]

On 2014/11/05 06:38:46, yunchao wrote:
> On 2014/11/04 19:12:45, bsalomon wrote:
> > Ok, it's now using the correct baseline.
> 
> This fix is interesting. Brian, is there a corresponding bug?

The Chromium bug that generated this is currently restricted to Googlers.
However, the repro is pretty simple.

Create two FBOs with textures as the color targets (A and B), then do a loop
like this:

for (0 to n)
   Bind A
   Clear color buffer
   Bind B
   Bind A as a texture
   Draw a textured rectangle
done
ReadPixels on B

The GM called "discard" is a more complicated version of this that exhibits the
bug. See this incorrect results:

Galaxy S4 w/ PVR 544MP:
http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/d...

vs the correct image:

http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/d...


I see the similar incorrect results on every PowerVR device I've tested
including a Venue 8 with RougeHood and the Galaxy Nexus with a 540.

[yunchao, 2014-11-06 01:48:56]

On 2014/11/05 15:28:05, bsalomon wrote:
> On 2014/11/05 06:38:46, yunchao wrote:
> > On 2014/11/04 19:12:45, bsalomon wrote:
> > > Ok, it's now using the correct baseline.
> > 
> > This fix is interesting. Brian, is there a corresponding bug?
> 
> The Chromium bug that generated this is currently restricted to Googlers.
> However, the repro is pretty simple.
> 
> Create two FBOs with textures as the color targets (A and B), then do a loop
> like this:
> 
> for (0 to n)
>    Bind A
>    Clear color buffer
>    Bind B
>    Bind A as a texture
>    Draw a textured rectangle
> done
> ReadPixels on B
> 
> The GM called "discard" is a more complicated version of this that exhibits
the
> bug. See this incorrect results:
> 
> Galaxy S4 w/ PVR 544MP:
>
http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/d...
> 
> vs the correct image:
> 
>
http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/d...
> 
> 
> I see the similar incorrect results on every PowerVR device I've tested
> including a Venue 8 with RougeHood and the Galaxy Nexus with a 540.

Thanks for your explanation, Brian. We came across a similar glClear issue
exposed by Skia code too. Now it is fixed in the driver of Gen graphics card.

[tfarina, 2014-11-06 14:23:01]

https://codereview.chromium.org/701573002/diff/120001/src/gpu/gl/GrGLUtil.cpp
File src/gpu/gl/GrGLUtil.cpp (right):

https://codereview.chromium.org/701573002/diff/120001/src/gpu/gl/GrGLUtil.cpp...
src/gpu/gl/GrGLUtil.cpp:214: SK_ARRAY_COUNT(kPowerVRRogueStr)-1)) {
can strlen() be used here?

[bsalomon, 2014-11-06 15:00:30]

https://codereview.chromium.org/701573002/diff/120001/src/gpu/gl/GrGLUtil.cpp
File src/gpu/gl/GrGLUtil.cpp (right):

https://codereview.chromium.org/701573002/diff/120001/src/gpu/gl/GrGLUtil.cpp...
src/gpu/gl/GrGLUtil.cpp:214: SK_ARRAY_COUNT(kPowerVRRogueStr)-1)) {
On 2014/11/06 14:23:01, tfarina wrote:
> can strlen() be used here?

It could