cc: Improve raster performance of PicturePileImpl

cc: Improve raster performance of PicturePileImpl

Rather than rasterizing back-to-front, raster pictures from front-to-back
using clips to prevent drawing over previous content.  This prevents
overdraw when rasterizing.

R=nduca@chromium.org
BUG=167306
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175748

[nduca, 2012-12-28 02:32:36]

lgtm but should we have someone from skia land comment on perf implications of
using clip?

[enne (OOO), 2012-12-28 02:35:16]

On 2012/12/28 02:32:36, nduca wrote:
> lgtm but should we have someone from skia land comment on perf implications of
> using clip?

Sure.  +reed, +junov

I'll also try to run this through telemetry and see if I can get better local
numbers, at least for Desktop.

[enne (OOO), 2012-12-28 20:54:35]

Numbers look pretty good, although it's not a clear win in all cases:
http://goto.google.com/picturepilerasterperf.

Across the board, it's about 10% fewer pixels rastered and 10% faster to raster
a pile.  The cases where this new path is not a win are when both methods end up
playing back the same number of pictures in the pile (i.e. piles aren't that
complex and some underlying picture doesn't get fully clipped by one above).  In
these cases, the new path is usually about ~7% slower.  Most of these sites
(barring digg as the one exception) are also really simple and quick, so some of
this could be noise.

I'd still be interested in having an opinion from Skia folks, especially if they
see a way to make this better.

[vangelis, 2013-01-04 00:44:00]

On 2012/12/28 20:54:35, enne wrote:
> Numbers look pretty good, although it's not a clear win in all cases:
> http://goto.google.com/picturepilerasterperf.
> 
> Across the board, it's about 10% fewer pixels rastered and 10% faster to
raster
> a pile.  The cases where this new path is not a win are when both methods end
up
> playing back the same number of pictures in the pile (i.e. piles aren't that
> complex and some underlying picture doesn't get fully clipped by one above). 
In
> these cases, the new path is usually about ~7% slower.  Most of these sites
> (barring digg as the one exception) are also really simple and quick, so some
of
> this could be noise.
> 
> I'd still be interested in having an opinion from Skia folks, especially if
they
> see a way to make this better.

The pixels rastered number doesn't take into account the clip regions, right? 
If that the case, then we seem to be getting the biggest benefit when we can
skip entire pictures in the pile.  If adding clip regions to the playback canvas
slows things down, could we simply avoid rasterizing pictures that are
completely covered by subsequent pictures in the pile and not worry about clip
regions?

[enne (OOO), 2013-01-04 01:08:01]

On 2013/01/04 00:44:00, vangelis wrote:

> The pixels rastered number doesn't take into account the clip regions, right? 
> If that the case, then we seem to be getting the biggest benefit when we can
> skip entire pictures in the pile.  If adding clip regions to the playback
canvas
> slows things down, could we simply avoid rasterizing pictures that are
> completely covered by subsequent pictures in the pile and not worry about clip
> regions?

I think your observation is correct.

Doing that makes the code a little bit more complicated since you have raster
bottom-up through the pile if you're not using clips, but need to calculate the
clips top-down to see what's skipped.  I could do a quick perf comparison and
see how it looks.

It'd be nice to get input from Skia folks.

[reed1, 2013-01-04 13:13:29]

Your measurements sound valid. Drawing less is better, but non-rectangular clips
have some overhead, so its not always obvious when its worth it to use them.

Some of our primitives are more efficient w/ non-rectangular clips than others.
We can work to optimize more for this case now that it may get used more often
(just in webkit we see this pretty rarely).

[vangelis, 2013-01-04 17:13:28]

On 2013/01/04 13:13:29, reed1 wrote:
> Your measurements sound valid. Drawing less is better, but non-rectangular
clips
> have some overhead, so its not always obvious when its worth it to use them.

I believe in this case all our clips should be rectangular. Would it be better
if we create an SkRegion for the clips and pass it in or is that what the
clipping code already does? 

> 
> Some of our primitives are more efficient w/ non-rectangular clips than
others.
> We can work to optimize more for this case now that it may get used more often
> (just in webkit we see this pretty rarely).

[reed1, 2013-01-04 18:29:05]

I thought that in (at least) one of the proposed cases, we were creating
clips with DIFFERENCE, which can result in a non-rectangular result.


On Fri, Jan 4, 2013 at 12:13 PM, <vangelis@google.com> wrote:

> On 2013/01/04 13:13:29, reed1 wrote:
>
>> Your measurements sound valid. Drawing less is better, but non-rectangular
>>
> clips
>
>> have some overhead, so its not always obvious when its worth it to use
>> them.
>>
>
> I believe in this case all our clips should be rectangular. Would it be
> better
> if we create an SkRegion for the clips and pass it in or is that what the
> clipping code already does?
>
>
>
>  Some of our primitives are more efficient w/ non-rectangular clips than
>>
> others.
>
>> We can work to optimize more for this case now that it may get used more
>> often
>> (just in webkit we see this pretty rarely).
>>
>
>
>
>
https://codereview.chromium.**org/11674004/<https://codereview.chromium.org/1...
>

[vangelis, 2013-01-04 18:38:24]

On 2013/01/04 18:29:05, reed1 wrote:
> I thought that in (at least) one of the proposed cases, we were creating
> clips with DIFFERENCE, which can result in a non-rectangular result.
> 

Oh, you are correct. I was thinking of unions of rectangular regions, which in
general won't be rectangular themselves.  They will however have axis aligned
boundaries. Is the underlying code storing the clip rects into an SkRegion and
doing inclusion queries with it?

[reed1, 2013-01-04 18:42:31]

Yes, skia internally stores complex BW clips as regions, and complex AA clips as
a RLE encoded mask.

[enne (OOO), 2013-01-04 21:52:06]

Eh, rect vs. region-based clips with the same region-based rejection end up
about the same.  Regions end up being slightly faster (1.34% on average) in my
data set.  This sort of makes some sense.  You have to do more clipping work but
end up clipping more.

I'm going to go with the original solution as it's less complicated.  It also
turns out to be worth the time to do region-based rejection of pictures vs.
quickReject, so I'll add that in as well.

[commit-bot: I haz the power, 2013-01-08 17:24:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11674004/16001

[commit-bot: I haz the power, 2013-01-08 17:41:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-08 18:10:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11674004/16001

[commit-bot: I haz the power, 2013-01-08 18:22:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-08 19:43:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11674004/16001

[commit-bot: I haz the power, 2013-01-08 20:00:36]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-08 20:50:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11674004/16001

[commit-bot: I haz the power, 2013-01-08 21:06:00]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-08 22:14:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11674004/40001

[commit-bot: I haz the power, 2013-01-08 22:18:50]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-09 00:02:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11674004/40001

[commit-bot: I haz the power, 2013-01-09 04:05:34]

Retried try job too often on win_aura for step(s) content_browsertests

[commit-bot: I haz the power, 2013-01-09 04:53:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11674004/40001

[commit-bot: I haz the power, 2013-01-09 05:25:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11674004/40001

[commit-bot: I haz the power, 2013-01-09 09:10:34]

Change committed as 175748