Push dash checks into GrContext.

Push dash checks into GrContext.

Add class to hold stroke and dash info.

R=bsalomon@google.com

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

[egdaniel, 2014-06-04 18:57:36]

This patch will depend on my previous cl
(https://codereview.chromium.org/314623004/) to land first.

This cl basically adds in a GrPaintStyle which holds a StrokeRec and DashInfo.
And this class is sent down into context where we can make rendering decisions
based on the dashed/stroke attributes

[bsalomon, 2014-06-04 19:05:04]

Adding Rob and Jim since this is a fairly big change to the GrContext API
(moving dashing from SkGpuDevice to GrContext). I think it is the right
direction but want them to see it.

https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:797: if (SkDashPath::FilterDashPath(effectPath.init(),
path, stroke, cullRect, *info)) {
I think this should just call drawPath with the undashed path and let drawPath
decide how to apply the dashing to the path. It avoids calling FilterDashPath
all over the place and if in the future a path renderer knows how to dash all
call sites could benefit.

Also, this stuff should go at the very top of the function, before we call
prepareToDraw.

https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrPaintStyle.h
File src/gpu/GrPaintStyle.h (right):

https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrPaintStyle.h#n...
src/gpu/GrPaintStyle.h:14: class GrPaintStyle {
I dunno about PaintStyle... it's more about styling the geometry than the
paint...

GrPathStyle?
GrGeomStyle?
...

https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrPaintStyle.h#n...
src/gpu/GrPaintStyle.h:35: SkStrokeRec* fStroke;
Can we do this in a safer way (not raw ptrs) without incurring copies?

[tfarina, 2014-06-04 19:10:38]

I have updated the CL description.

Please, for future patches, remember that the first line is the summary and
should be a single sentence at most 72 chars ending with period.

Also try to be more descriptive and if possible include the reason/why changes
are being made.

Thanks,

[mtklein, 2014-06-04 19:27:08]

On 2014/06/04 19:10:38, tfarina wrote:
> I have updated the CL description.
> 
> Please, for future patches, remember that the first line is the summary and
> should be a single sentence at most 72 chars ending with period.
> 
> Also try to be more descriptive and if possible include the reason/why changes
> are being made.
> 
> Thanks,

Woah, Thiago, do not edit other people's CLs like that.  This was particularly
destructive, as there is no obvious history for descriptions.

We are all welcome to ask for a more descriptive description, but we certainly
cannot prescribe it, let alone edit it without consent.

[robertphillips, 2014-06-04 19:33:00]

https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrPaintStyle.h
File src/gpu/GrPaintStyle.h (right):

https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrPaintStyle.h#n...
src/gpu/GrPaintStyle.h:14: class GrPaintStyle {
I also find PaintStyle less then intuitive:

GrGeomModifiers?
GrDrawMode?
GrDrawSpecialCases?

[egdaniel, 2014-06-04 19:34:29]

https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrPaintStyle.h
File src/gpu/GrPaintStyle.h (right):

https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrPaintStyle.h#n...
src/gpu/GrPaintStyle.h:14: class GrPaintStyle {
On 2014/06/04 19:05:05, bsalomon wrote:
> I dunno about PaintStyle... it's more about styling the geometry than the
> paint...
> 
> GrPathStyle?
> GrGeomStyle?
> ...

It should be changed but I think Path is too specific and Geom is not really
descriptive. How about LineStyle or EdgeStyle?

[bsalomon, 2014-06-04 19:39:01]

On 2014/06/04 19:34:29, egdaniel wrote:
> https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrPaintStyle.h
> File src/gpu/GrPaintStyle.h (right):
> 
>
https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrPaintStyle.h#n...
> src/gpu/GrPaintStyle.h:14: class GrPaintStyle {
> On 2014/06/04 19:05:05, bsalomon wrote:
> > I dunno about PaintStyle... it's more about styling the geometry than the
> > paint...
> > 
> > GrPathStyle?
> > GrGeomStyle?
> > ...
> 
> It should be changed but I think Path is too specific and Geom is not really
> descriptive. How about LineStyle or EdgeStyle?

GrPathEffect
*ducks*

[tfarina, 2014-06-04 19:48:58]

On 2014/06/04 19:27:08, mtklein wrote:
> Woah, Thiago, do not edit other people's CLs like that.  This was particularly
> destructive, as there is no obvious history for descriptions.
> 
I have not "edited" it in the sense that I did not add or remove any information
from the original description.

Regards,

[egdaniel, 2014-06-05 17:18:16]

https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:797: if (SkDashPath::FilterDashPath(effectPath.init(),
path, stroke, cullRect, *info)) {
On 2014/06/04 19:05:05, bsalomon wrote:
> I think this should just call drawPath with the undashed path and let drawPath
> decide how to apply the dashing to the path. It avoids calling FilterDashPath
> all over the place and if in the future a path renderer knows how to dash all
> call sites could benefit.
> 
> Also, this stuff should go at the very top of the function, before we call
> prepareToDraw.

done

https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrPaintStyle.h
File src/gpu/GrPaintStyle.h (right):

https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrPaintStyle.h#n...
src/gpu/GrPaintStyle.h:14: class GrPaintStyle {
On 2014/06/04 19:34:30, egdaniel wrote:
> On 2014/06/04 19:05:05, bsalomon wrote:
> > I dunno about PaintStyle... it's more about styling the geometry than the
> > paint...
> > 
> > GrPathStyle?
> > GrGeomStyle?
> > ...
> 
> It should be changed but I think Path is too specific and Geom is not really
> descriptive. How about LineStyle or EdgeStyle?

Changed to GrStrokeInfo

https://codereview.chromium.org/311183002/diff/20001/src/gpu/GrPaintStyle.h#n...
src/gpu/GrPaintStyle.h:35: SkStrokeRec* fStroke;
On 2014/06/04 19:05:05, bsalomon wrote:
> Can we do this in a safer way (not raw ptrs) without incurring copies?

The SkStrokeRec and DashInfo are now stored inside of GrStrokeInfo.

[robertphillips, 2014-06-05 18:28:03]

https://codereview.chromium.org/311183002/diff/40001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/311183002/diff/40001/include/gpu/GrContext.h#...
include/gpu/GrContext.h:455: *  @param paint        describes how to color
pixels.
strokeInfo ? Does the text need changing ?

https://codereview.chromium.org/311183002/diff/40001/include/gpu/GrContext.h#...
include/gpu/GrContext.h:493: *  @param rrect        the roundrect to draw
strokeInfo ?

https://codereview.chromium.org/311183002/diff/40001/include/gpu/GrContext.h#...
include/gpu/GrContext.h:514: * @param path          the path to draw
strokeInfo ?

https://codereview.chromium.org/311183002/diff/40001/include/gpu/GrContext.h#...
include/gpu/GrContext.h:548: * @param oval          the bounding rect of the
oval.
strokeInfo ?

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

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:781: const SkMatrix* matrix) {
NULL != ?

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:794: 
NULL ==

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:842: 
strokeRec ?

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:1038: 
strokeRec ?

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:1095: 
strokeRec ?

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:1172: SkTLazy<SkPath> effectPath;
Do we need cullRect here ? Can we just pass NULL to FilterDashPath ?

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:1196: 
strokeRec ?

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrStrokeInfo.h
File src/gpu/GrStrokeInfo.h (right):

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:13: 
// GrStrokeInfo encapsulates ... ?

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:16: GrStrokeInfo(SkStrokeRec::InitStyle style) :
We usually don't put default ctors in the initialization list.

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:20: if (includeDash) {
We don't usually put "this->" in this context (accessing a member variable).

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:28: explicit GrStrokeInfo(const SkPaint& paint) :
same here (r.e. default ctor).

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:41: // is a dashed effect and we are stroking, otherwise
it retruns false.
Can 'pe' be const ?

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:42: bool setDashInfo(SkPathEffect* pe, SkScalar*
intervals = NULL) {
NULL != pe ?

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:43: if (pe && !fStroke.isFillStyle()) {
NULL !=

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:63: private:
line these guys up?

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

https://codereview.chromium.org/311183002/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:493: SkPathEffect* pe = paint.getPathEffect();
NULL != pe ?

https://codereview.chromium.org/311183002/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:562: SkPathEffect* pe = paint.getPathEffect();
NULL != pe ?

https://codereview.chromium.org/311183002/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:623: SkPathEffect* pe = paint.getPathEffect();
NULL != pe ?

https://codereview.chromium.org/311183002/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:890: SkPaint::Style style = stroke.isHairlineStyle() ?
SkPaint::kStroke_Style :
Fix this alignment?

[egdaniel, 2014-06-05 19:07:25]

https://codereview.chromium.org/311183002/diff/40001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/311183002/diff/40001/include/gpu/GrContext.h#...
include/gpu/GrContext.h:455: *  @param paint        describes how to color
pixels.
On 2014/06/05 18:28:00, robertphillips wrote:
> strokeInfo ? Does the text need changing ?

Ah yes. Same with all others pointed out below

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

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:781: const SkMatrix* matrix) {
On 2014/06/05 18:28:00, robertphillips wrote:
> NULL != ?

Yes to this and all the other ones

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:781: const SkMatrix* matrix) {
On 2014/06/05 18:28:00, robertphillips wrote:
> NULL != ?

yes to this and all the other ones

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:842: 
On 2014/06/05 18:28:00, robertphillips wrote:
> strokeRec ?

sure

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:1172: SkTLazy<SkPath> effectPath;
On 2014/06/05 18:28:00, robertphillips wrote:
> Do we need cullRect here ? Can we just pass NULL to FilterDashPath ?

So I was matching the format used by the filterPath call used in drawPath in
SkGpuDevice. But looking at it more closely, the cullRect does not seem to be
used anywhere there either. Does anyone know why we have it there versus passing
in NULL? Will remove both if no one has a good reason.

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrStrokeInfo.h
File src/gpu/GrStrokeInfo.h (right):

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:13: 
On 2014/06/05 18:28:01, robertphillips wrote:
> // GrStrokeInfo encapsulates ... ?

Will add description

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:16: GrStrokeInfo(SkStrokeRec::InitStyle style) :
On 2014/06/05 18:28:01, robertphillips wrote:
> We usually don't put default ctors in the initialization list.

removed

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:41: // is a dashed effect and we are stroking, otherwise
it retruns false.
On 2014/06/05 18:28:01, robertphillips wrote:
> Can 'pe' be const ?

yes

[bsalomon, 2014-06-05 19:22:54]

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

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:780: const GrStrokeInfo* strokeInfo,
Why does this take a StrokeRec* and others a StrokeRec&

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

https://codereview.chromium.org/311183002/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:561: 
 else {
    SkPathEffect* pe = paint.getPathEffect();
    if (NULL != pe && !strokeInfo.setDashInfo(pe)) {
        usePath = true;
    }
}

that seems slightly clearer to me.

https://codereview.chromium.org/311183002/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:574: SkAutoTArray<SkScalar>
intervals(strokeInfo.dashCount());
This feels kinda ugly to me. Can StrokeInfo own the intervals array so we only
need to do one setDashInfo call?

[egdaniel, 2014-06-05 20:18:17]

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

https://codereview.chromium.org/311183002/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:780: const GrStrokeInfo* strokeInfo,
On 2014/06/05 19:22:54, bsalomon wrote:
> Why does this take a StrokeRec* and others a StrokeRec&

There are a few locations throughout the code that call drawRect without any
stroke information, thus it just defaults a NULL pointer in this case. It saves
the overhead of having to create a StrokeRec object (now GrStrokeInfo) since
drawRect views a NULL here as doing no stroking.

[egdaniel, 2014-06-05 20:31:37]

ptal

[bsalomon, 2014-06-05 20:43:32]

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

https://codereview.chromium.org/311183002/diff/80001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:27: fDashInfo = src.fDashInfo;
Don't you have to copy the intervals here?

https://codereview.chromium.org/311183002/diff/80001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:35: fStroke(paint),
fDashType(SkPathEffect::kNone_DashType) {}
Do we use this ctor? It seems weird that paint has the dash info in a pe but we
aren't extracting it in this cons.

https://codereview.chromium.org/311183002/diff/80001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:76: SkAutoSTArray<2, SkScalar> fIntervals;
This can use true for the third MEM_COPY template param (defaults to false).

[egdaniel, 2014-06-05 21:12:09]

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

https://codereview.chromium.org/311183002/diff/80001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:27: fDashInfo = src.fDashInfo;
On 2014/06/05 20:43:32, bsalomon wrote:
> Don't you have to copy the intervals here?

Ah yes missed that. Fixed now

https://codereview.chromium.org/311183002/diff/80001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:35: fStroke(paint),
fDashType(SkPathEffect::kNone_DashType) {}
On 2014/06/05 20:43:32, bsalomon wrote:
> Do we use this ctor? It seems weird that paint has the dash info in a pe but
we
> aren't extracting it in this cons.

This is the main ctor. It is now set up to call setDashInfo inside of this ctor

https://codereview.chromium.org/311183002/diff/80001/src/gpu/GrStrokeInfo.h#n...
src/gpu/GrStrokeInfo.h:76: SkAutoSTArray<2, SkScalar> fIntervals;
On 2014/06/05 20:43:32, bsalomon wrote:
> This can use true for the third MEM_COPY template param (defaults to false).

Using AutoSTArray not STArray so there is no third template param

[bsalomon, 2014-06-05 21:18:58]

lgtm

[egdaniel, 2014-06-11 17:18:37]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-06-11 17:19:03]

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

[commit-bot: I haz the power, 2014-06-11 17:30:09]

Change committed as d58a0ba9cff9fcefe5047e88ccb4a6e76c591c40