Add Dashing gpu effect for simple dashed lines

Add Dashing gpu effect for simple dashed lines

BUG=skia:

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

[egdaniel, 2014-05-09 14:36:48]

First pass at dashing. This version supports all lines that have only a single
on/off pair. It can handle square and butt caps but not round.

The viewmatrix must also preserve right angles.

[bsalomon, 2014-05-09 15:02:37]

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:569: static void calcDashScaling(SkScalar*
parallelScale, SkScalar* perpScale,
style nit: we've been tending towards
naming_static_non_class_functions_in_hacker_style().

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:603: // Assumes phase < sum of all intervals
is that a valid assumption? add an assert?

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:605: SkScalar srcIntervalLen = info.fIntervals[0] +
info.fIntervals[1];
don't need this until inside both ifs, right?

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:607: if (info.fIntervals[0] !=0 || info.fPhase != 0) {
if intervals[0] is != 0 then phase can't be either, right (since it is >=
intervals[0])? I guess I'm assuming intervals[0] is never negative.

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:634: bool SkGpuDevice::drawDash(const SkPoint pts[2],
const SkPaint& paint, SkCanvas::PointMode mode) {
maybe this should be called drawDashedLine()? It only allows two points as the
input geometry. Any reason to take the mode? Do you think it might be expanded
to support anything but kLines in the future?

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:652: GrPaint grPaint;
maybe hold off on this until after your cap check? It's non-trivial and wasted
work if we reject below.

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:689: // TODO: what happens if adjustments make
degenerate line?
Can these conditions cause us to draw incorrectly today? Are there test cases?

[egdaniel, 2014-05-12 19:21:44]

updated version

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:569: static void calcDashScaling(SkScalar*
parallelScale, SkScalar* perpScale,
fixed
On 2014/05/09 15:02:37, bsalomon wrote:
> style nit: we've been tending towards
> naming_static_non_class_functions_in_hacker_style().

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:603: // Assumes phase < sum of all intervals
In the creation of SkDashEffect the phase is normalized to be between [0, len).
Since this helper function should only be called from within drawDash, which
itself gets the phase from an SkPaint then this assumption should be valid in
out current use cases. But I'll add an assert regardless.

On 2014/05/09 15:02:37, bsalomon wrote:
> is that a valid assumption? add an assert?

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:605: SkScalar srcIntervalLen = info.fIntervals[0] +
info.fIntervals[1];
yup
On 2014/05/09 15:02:37, bsalomon wrote:
> don't need this until inside both ifs, right?

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:607: if (info.fIntervals[0] !=0 || info.fPhase != 0) {
Ahh you're right. I was just inverting the logic in my mind that I can't have
both 0.

On 2014/05/09 15:02:37, bsalomon wrote:
> if intervals[0] is != 0 then phase can't be either, right (since it is >=
> intervals[0])? I guess I'm assuming intervals[0] is never negative.

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:634: bool SkGpuDevice::drawDash(const SkPoint pts[2],
const SkPaint& paint, SkCanvas::PointMode mode) {
Name changed. Moved the mode check to the caller.

On 2014/05/09 15:02:37, bsalomon wrote:
> maybe this should be called drawDashedLine()? It only allows two points as the
> input geometry. Any reason to take the mode? Do you think it might be expanded
> to support anything but kLines in the future?

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:652: GrPaint grPaint;
done
On 2014/05/09 15:02:37, bsalomon wrote:
> maybe hold off on this until after your cap check? It's non-trivial and wasted
> work if we reject below.

https://codereview.chromium.org/274673004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:689: // TODO: what happens if adjustments make
degenerate line?
Changed so that if the first or last dashes in the line (and using AA), we will
draw those dashes separately using drawRects, and then using the shader for the
interior part of the line.

Also added checks to see if we ever end up in a case where the start point and
end point have been adjusted such that the end is to the left of the start. In
this case we just return since nothing is left to draw.

On 2014/05/09 15:02:37, bsalomon wrote:
> Can these conditions cause us to draw incorrectly today? Are there test cases?
>

[bsalomon, 2014-05-13 09:10:32]

https://codereview.chromium.org/274673004/diff/60001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/274673004/diff/60001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:637: 
This file has gotten huge. It seems like the only member being used in fContext.
What do you think about making this a helper function declared in GrDashEffect.h
and implemented in GrDashEffect.cpp. The details of how this works and when it
fails seem more intimately connected to the code in that file rather than
anything in SkGpuDevice.

https://codereview.chromium.org/274673004/diff/60001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:638: bool SkGpuDevice::drawDashLine(const SkPoint
pts[2], const SkPaint& paint) {
I think this function needs to check if the target uses MSAA and fail, since it
can't discard on a per-sample basis as of yet.

https://codereview.chromium.org/274673004/diff/60001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:700: bool hasCap =SkPaint::kSquare_Cap == cap && 0 !=
srcStrokeWidth;
space after =

https://codereview.chromium.org/274673004/diff/60001/src/gpu/effects/GrDashin...
File src/gpu/effects/GrDashingEffect.h (right):

https://codereview.chromium.org/274673004/diff/60001/src/gpu/effects/GrDashin...
src/gpu/effects/GrDashingEffect.h:28: class GrDashingEffect : public GrEffect {
AFAICT none of the public members or the function are relevant to the caller of
Create() but rather facilitate extracting out params for the GrGLEffect subclass
or are virtuals used by the GrEffect shader building system. It's a virtue that
the creator gets back a generic effect rather than having any knowledge that
it's a GrDrashingEffect. Maybe this should be shuffled around so that the
GrEffect subclass itself is private to the cpp file (a la
GrConvexPolyEffect.h/cpp).

[egdaniel, 2014-05-13 14:00:11]

https://codereview.chromium.org/274673004/diff/60001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/274673004/diff/60001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:637: 
Agree completely. Will move it out.

On 2014/05/13 09:10:33, bsalomon wrote:
> This file has gotten huge. It seems like the only member being used in
fContext.
> What do you think about making this a helper function declared in
GrDashEffect.h
> and implemented in GrDashEffect.cpp. The details of how this works and when it
> fails seem more intimately connected to the code in that file rather than
> anything in SkGpuDevice.

https://codereview.chromium.org/274673004/diff/60001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:638: bool SkGpuDevice::drawDashLine(const SkPoint
pts[2], const SkPaint& paint) {
is that not what the check below this comment is doing?

On 2014/05/13 09:10:33, bsalomon wrote:
> I think this function needs to check if the target uses MSAA and fail, since
it
> can't discard on a per-sample basis as of yet.

https://codereview.chromium.org/274673004/diff/60001/src/gpu/effects/GrDashin...
File src/gpu/effects/GrDashingEffect.h (right):

https://codereview.chromium.org/274673004/diff/60001/src/gpu/effects/GrDashin...
src/gpu/effects/GrDashingEffect.h:28: class GrDashingEffect : public GrEffect {
Slightly confused here. I thought I understood your various arguments in the
first half of what you wrote, but then when you said like GrConvexPolyEffect you
lost me. To me GrCPE and GrDE are set up almost identically. Unless are you
referring the AARectEffect class that is inside GrCPE?

On 2014/05/13 09:10:33, bsalomon wrote:
> AFAICT none of the public members or the function are relevant to the caller
of
> Create() but rather facilitate extracting out params for the GrGLEffect
subclass
> or are virtuals used by the GrEffect shader building system. It's a virtue
that
> the creator gets back a generic effect rather than having any knowledge that
> it's a GrDrashingEffect. Maybe this should be shuffled around so that the
> GrEffect subclass itself is private to the cpp file (a la
> GrConvexPolyEffect.h/cpp).

[bsalomon, 2014-05-13 14:14:36]

If it is required, the skpaint->grpaint conversion functions can be moved to
SkGr.[h|cpp].

https://codereview.chromium.org/274673004/diff/60001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/274673004/diff/60001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:638: bool SkGpuDevice::drawDashLine(const SkPoint
pts[2], const SkPaint& paint) {
dOn 2014/05/13 14:00:11, egdaniel wrote:
> is that not what the check below this comment is doing?
> 
> On 2014/05/13 09:10:33, bsalomon wrote:
> > I think this function needs to check if the target uses MSAA and fail, since
> it
> > can't discard on a per-sample basis as of yet.
>

sorry.. yes.

https://codereview.chromium.org/274673004/diff/60001/src/gpu/effects/GrDashin...
File src/gpu/effects/GrDashingEffect.h (right):

https://codereview.chromium.org/274673004/diff/60001/src/gpu/effects/GrDashin...
src/gpu/effects/GrDashingEffect.h:28: class GrDashingEffect : public GrEffect {
On 2014/05/13 14:00:11, egdaniel wrote:
> Slightly confused here. I thought I understood your various arguments in the
> first half of what you wrote, but then when you said like GrConvexPolyEffect
you
> lost me. To me GrCPE and GrDE are set up almost identically. Unless are you
> referring the AARectEffect class that is inside GrCPE?
> 
> On 2014/05/13 09:10:33, bsalomon wrote:
> > AFAICT none of the public members or the function are relevant to the caller
> of
> > Create() but rather facilitate extracting out params for the GrGLEffect
> subclass
> > or are virtuals used by the GrEffect shader building system. It's a virtue
> that
> > the creator gets back a generic effect rather than having any knowledge that
> > it's a GrDrashingEffect. Maybe this should be shuffled around so that the
> > GrEffect subclass itself is private to the cpp file (a la
> > GrConvexPolyEffect.h/cpp).
> 

You're right. I picked a bad example. See GrRRectEffect. GrConvexPolyEffect
should probably follow this pattern as well.

[egdaniel, 2014-05-15 19:37:15]

Still need to get the Sk2GrPaint landed but once it is this should be good

[egdaniel, 2014-05-19 13:44:02]

ptal

[egdaniel, 2014-05-19 13:44:03]

ptal

[bsalomon, 2014-05-19 14:12:30]

lgtm. nit: We should either nest DrawDashLine in the namespace or give it a Gr
prefix. It'd be nice to see if DrawDashLine/SkPaint2GrPaint could take GrContext
instead of SkGpuDevice (as a follow up change).

[egdaniel, 2014-05-19 14:27:34]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-05-19 14:27:43]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/egdaniel@google.com/274673004/140001

[commit-bot: I haz the power, 2014-05-19 14:33:01]

Change committed as 14775

[f(malita), 2014-05-19 19:46:30]

I'm seeing related valgrind failures:

http://108.170.220.120:10117/builders/Test-Ubuntu12-ShuttleA-GTX550Ti-x86_64-...