Crop the fast path dashed lines to the cull rect

Crop the fast path dashed lines to the cull rect

Without:

maxrss  loops   min     median  mean    max     stddev  samples         config  bench
  56M   1       13.3ms  13.6ms  13.6ms  14.2ms  2%      Ooooo.....      8888    GM_dashing5_bw
  56M   13      390us   417us   416us   459us   5%      ooooO..o.o      gpu     GM_dashing5_bw
  56M   1       13.4ms  13.9ms  14.1ms  15ms    3%      Oooo..ooOo      8888    GM_dashing5_aa
  56M   13      402us   421us   416us   425us   2%      Ooo.ooOOOO      gpu     GM_dashing5_aa

With:

  40M   1       1.53ms  1.54ms  1.54ms  1.55ms  0%      oo.O...o..      8888    GM_dashing5_bw
  40M   12      407us   412us   415us   445us   3%      ...Oo.....      gpu     GM_dashing5_bw
  40M   1       1.7ms   1.7ms   1.7ms   1.72ms  0%      o.O.......      8888    GM_dashing5_aa
  43M   13      405us   409us   409us   415us   1%      ooo.Ooo..o      gpu     GM_dashing5_aa

The GM images (including the new one) are the same with and without this CL.

BUG=428296
Committed: https://skia.googlesource.com/skia/+/9f2251c73ed6f417dd1057d487bf523e04488440

[robertphillips, 2014-11-03 21:17:38]

robertphillips@google.com changed reviewers:
+ danakj@chromium.org, egdaniel@google.com, reed@google.com

[robertphillips, 2014-11-03 21:20:07]

https://codereview.chromium.org/699623003/diff/1/src/effects/SkDashPathEffect...
File src/effects/SkDashPathEffect.cpp (right):

https://codereview.chromium.org/699623003/diff/1/src/effects/SkDashPathEffect...
src/effects/SkDashPathEffect.cpp:43: 
This code was adapted from src\utils\SkDashPath.cpp:cull_path.

[reed1, 2014-11-03 21:50:36]

Can we add this after the initial isLine() check? If so, then we could just pass
in the 2 points and have them clipped/chopped, and avoid creating another SkPath
object entirely.

[egdaniel, 2014-11-03 22:03:31]

https://codereview.chromium.org/699623003/diff/1/src/effects/SkDashPathEffect...
File src/effects/SkDashPathEffect.cpp (right):

https://codereview.chromium.org/699623003/diff/1/src/effects/SkDashPathEffect...
src/effects/SkDashPathEffect.cpp:98: minX = bounds.fLeft -
SkScalarMod(bounds.fLeft - minX, intervalLength);
This moding thing to get "in phase" seems off to me.

Looking at the max equation, say my line is horizontal and goes from [0, X]. The
dash interval is 2 on, 2 off (doesn't really matter the intervals). My right
bounds is at R where 0 < R < X. So we would want to chop the end here. So we
chop at R plus some value. However, this value is dependent on X. This confuses
me because in my mind I should be able to extend X to infinity and where I chop
at shouldn't change as X changes.

[robertphillips, 2014-11-04 18:10:23]

PTAL 

This version:

enhances the GM/bench with phases, flipping the order of the line endpoints and
different stroke widths

removes the creation of the path (for the cropped line)

[danakj, 2014-11-04 18:19:19]

On 2014/11/04 18:10:23, robertphillips wrote:
> PTAL 
> 
> This version:
> 
> enhances the GM/bench with phases, flipping the order of the line endpoints
and
> different stroke widths
> 
> removes the creation of the path (for the cropped line)

I applied this and tried out the jabong.com site as per
https://code.google.com/p/chromium/issues/detail?id=428296

I see that the drawPoints call is much faster now, except for one tile where it
is still 65ms.

[danakj, 2014-11-04 18:24:11]

On 2014/11/04 18:19:19, danakj wrote:
> On 2014/11/04 18:10:23, robertphillips wrote:
> > PTAL 
> > 
> > This version:
> > 
> > enhances the GM/bench with phases, flipping the order of the line endpoints
> and
> > different stroke widths
> > 
> > removes the creation of the path (for the cropped line)
> 
> I applied this and tried out the http://jabong.com site as per
> https://code.google.com/p/chromium/issues/detail?id=428296
> 
> I see that the drawPoints call is much faster now, except for one tile where
it
> is still 65ms.

In the slow case, the cullRect's LTRB is (0, 20, 206, 21). So this would be the
right-most tile in the line.

The pts[] are:
pts[0] = { 227, 4592.5 }
pts[1] = { 10000224, 4592.5 }

[reed1, 2014-11-04 18:24:58]

https://codereview.chromium.org/699623003/diff/20001/src/effects/SkDashPathEf...
File src/effects/SkDashPathEffect.cpp (right):

https://codereview.chromium.org/699623003/diff/20001/src/effects/SkDashPathEf...
src/effects/SkDashPathEffect.cpp:77: if (dx && dy) {
can/should we move this check earlier? seems like we don't need to perform the
outset to check this.

https://codereview.chromium.org/699623003/diff/20001/src/effects/SkDashPathEf...
src/effects/SkDashPathEffect.cpp:92: return;
If we're returning here, does that mean we're culled-out? should we return a
flag that says don't draw?

[danakj, 2014-11-04 18:30:05]

https://codereview.chromium.org/699623003/diff/20001/src/effects/SkDashPathEf...
File src/effects/SkDashPathEffect.cpp (right):

https://codereview.chromium.org/699623003/diff/20001/src/effects/SkDashPathEf...
src/effects/SkDashPathEffect.cpp:91: if (maxX < bounds.fLeft || minX >
bounds.fRight) {
This early out is causing us to miss the last tile and not cull anything for it.

minX is 227
maxX is 10000224

bounds.fLeft is -2
bounds.fRight is 208

It appears that bounds.fRight is not correct?

[robertphillips, 2014-11-04 18:51:16]

Dana - could you try this version?

Note: it still doesn't deal correctly with the matrix but that will be
forthcoming.

https://codereview.chromium.org/699623003/diff/20001/src/effects/SkDashPathEf...
File src/effects/SkDashPathEffect.cpp (right):

https://codereview.chromium.org/699623003/diff/20001/src/effects/SkDashPathEf...
src/effects/SkDashPathEffect.cpp:77: if (dx && dy) {
On 2014/11/04 18:24:58, reed1 wrote:
> can/should we move this check earlier? seems like we don't need to perform the
> outset to check this.

Done.

https://codereview.chromium.org/699623003/diff/20001/src/effects/SkDashPathEf...
src/effects/SkDashPathEffect.cpp:91: if (maxX < bounds.fLeft || minX >
bounds.fRight) {
On 2014/11/04 18:30:04, danakj wrote:
> This early out is causing us to miss the last tile and not cull anything for
it.
> 
> minX is 227
> maxX is 10000224
> 
> bounds.fLeft is -2
> bounds.fRight is 208
> 
> It appears that bounds.fRight is not correct?

Done.

https://codereview.chromium.org/699623003/diff/20001/src/effects/SkDashPathEf...
src/effects/SkDashPathEffect.cpp:92: return;
On 2014/11/04 18:24:58, reed1 wrote:
> If we're returning here, does that mean we're culled-out? should we return a
> flag that says don't draw?

Done.

[danakj, 2014-11-04 18:59:46]

On 2014/11/04 18:51:16, robertphillips wrote:
> Dana - could you try this version?

Yep it works now for that site.

[reed1, 2014-11-04 19:01:24]

have we been able to catch these tricky cases in a unittest or some other thing?

[danakj, 2014-11-04 19:14:54]

On 2014/11/04 18:59:46, danakj wrote:
> On 2014/11/04 18:51:16, robertphillips wrote:
> > Dana - could you try this version?
> 
> Yep it works now for that site.

Oh, I should've looked at what it draws too. The dashed line is now missing on
all tiles except the first and last.

[robertphillips, 2014-11-04 19:25:08]

On 2014/11/04 19:14:54, danakj wrote:
> On 2014/11/04 18:59:46, danakj wrote:
> > On 2014/11/04 18:51:16, robertphillips wrote:
> > > Dana - could you try this version?
> > 
> > Yep it works now for that site.
> 
> Oh, I should've looked at what it draws too. The dashed line is now missing on
> all tiles except the first and last.

That is probably the matrix handling I'm working on right now.

[robertphillips, 2014-11-04 20:12:14]

Okay - new version with coordinate transform support.

[danakj, 2014-11-04 20:25:45]

On 2014/11/04 20:12:14, robertphillips wrote:
> Okay - new version with coordinate transform support.

This is great, I don't see it hitting the early out on that last tile anymore,
and it's fast for all the tiles, and it looks visually correct to me.

[robertphillips, 2014-11-04 20:55:16]

On 2014/11/04 19:01:24, reed1 wrote:
> have we been able to catch these tricky cases in a unittest or some other
thing?

The new GM/bench does exercise a lot of the possible variables (e.g., stroke
thickness, phase and order of vertices) but we will need a explicit unit test to
directly exercise asPoints on the corner cases.

I propose that be its own CL however so we can get this churning through the
bots.

[reed1, 2014-11-04 21:11:37]

ok. I was thinking specifically of whatever the conditions were that had been
caught just now during ad-hoc testing, but later is fine too. lgtm

[robertphillips, 2014-11-04 21:20:28]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-11-04 21:21:24]

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

[commit-bot: I haz the power, 2014-11-04 21:33:54]

Committed patchset #4 (id:60001) as 9f2251c73ed6f417dd1057d487bf523e04488440