[PDF] Improve path effect and drawPath(prePathMatrix) support.

[PDF] Improve path effect and drawPath(prePathMatrix) support.

Add path effect handling for drawPoints (via drawPath).
Move path effect handling from drawRect to drawPath.
Add prePathMatrix support to drawPath.

BUG=crbug.com/82849

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

[Steve VanDeBogart, 2011-05-10 00:02:34]

Mike,
Can you clarify the use of SkPaint:getFillPath?  How do I need to modify the
paint after calling getFillPath? Obviously removing the path effect.  The
comment implies I should set the style and possible the line widths to
particular values, but I don't see that in the existing calls to it.

[reed1, 2011-05-10 16:27:07]

getFillPath returns a new path that should be "filled": i.e. any
stroking or patheffects have already been applied, so just a plain
paint will do.

To modify the paint that was used to create the fill, you can do this:
- paint.setStyle(SkPaint::kFill_Style);
- paint.setPathEffect(NULL);

On Mon, May 9, 2011 at 5:02 PM,  <vandebo@chromium.org> wrote:
> Reviewers: reed1,
>
> Message:
> Mike,
> Can you clarify the use of SkPaint:getFillPath?  How do I need to modify
> the paint after calling getFillPath? Obviously removing the path effect.
>  The comment implies I should set the style and possible the line widths
> to particular values, but I don't see that in the existing calls to it.
>
> Description:
> [PDF] Improve path effect and drawPath(prePathMatrix) support.
>
> Add path effect handling for drawPoints (via drawPath).
> Move path effect handling from drawRect to drawPath.
> Add prePathMatrix support to drawPath.
>
>
> Please review this at http://codereview.appspot.com/4531047/
>
> Affected files:
>  M src/pdf/SkPDFDevice.cpp
>
>
> Index: src/pdf/SkPDFDevice.cpp
> diff --git a/src/pdf/SkPDFDevice.cpp b/src/pdf/SkPDFDevice.cpp
> index
>
2d4f0fbac41cf5cb6915cb99a19fdc4ecfad6208..cee3b72432d0c858ac1f19ed7acb53796f80fd99
> 100644
> --- a/src/pdf/SkPDFDevice.cpp
> +++ b/src/pdf/SkPDFDevice.cpp
> @@ -590,6 +590,19 @@ void SkPDFDevice::drawPoints(const SkDraw& d,
> SkCanvas::PointMode mode,
>         return;
>     }
>
> +    // SkDraw::drawPoints converts to multiple calls to fDevice->drawPath.
> +    // We only use this when there's a path effect because of the overhead
> +    // of multiple calls to setUpContentEntry it causes.
> +    if (passedPaint.getPathEffect()) {
> +        if (d.fClip->isEmpty()) {
> +            return;
> +        }
> +        SkDraw pointDraw(d);
> +        pointDraw.fDevice = this;
> +        pointDraw.drawPoints(mode, count, points, passedPaint, true);
> +        return;
> +    }
> +
>     const SkPaint* paint = &passedPaint;
>     SkPaint modifiedPaint;
>
> @@ -659,14 +672,9 @@ void SkPDFDevice::drawRect(const SkDraw& d, const
> SkRect& r,
>         if (d.fClip->isEmpty()) {
>             return;
>         }
> -        // Create a path for the rectangle and apply the path effect to it.
>         SkPath path;
>         path.addRect(r);
> -        paint.getFillPath(path, &path);
> -
> -        SkPaint noEffectPaint(paint);
> -        SkSafeUnref(noEffectPaint.setPathEffect(NULL));
> -        drawPath(d, path, noEffectPaint, NULL, true);
> +        drawPath(d, path, paint, NULL, true);
>         return;
>     }
>     if (!setUpContentEntry(*d.fClipStack, *d.fClip, *d.fMatrix, paint)) {
> @@ -678,30 +686,52 @@ void SkPDFDevice::drawRect(const SkDraw& d, const
> SkRect& r,
>                           &fCurrentContentEntry->fContent);
>  }
>
> -void SkPDFDevice::drawPath(const SkDraw& d, const SkPath& path,
> +void SkPDFDevice::drawPath(const SkDraw& d, const SkPath& origPath,
>                            const SkPaint& paint, const SkMatrix*
> prePathMatrix,
>                            bool pathIsMutable) {
> -    NOT_IMPLEMENTED(prePathMatrix != NULL, true);
> +    SkPath modifiedPath;
> +    SkPath* curPath = (SkPath*)&origPath;
> +    const SkPath* prevPath = &origPath;
> +
> +    SkMatrix matrix = *d.fMatrix;
> +    if (prePathMatrix) {
> +        if (paint.getPathEffect() || paint.getStyle() !=
> SkPaint::kFill_Style) {
> +            if (!pathIsMutable) {
> +                prevPath = curPath;
> +                curPath = &modifiedPath;
> +                pathIsMutable = true;
> +            }
> +            prevPath->transform(*prePathMatrix, curPath);
> +        } else {
> +            if (!matrix.preConcat(*prePathMatrix)) {
> +                return;
> +            }
> +        }
> +    }
>
>     if (paint.getPathEffect()) {
>         if (d.fClip->isEmpty()) {
>             return;
>         }
> -        // Apply the path effect to path and draw it that way.
> -        SkPath noEffectPath;
> -        paint.getFillPath(path, &noEffectPath);
> +        if (!pathIsMutable) {
> +            prevPath = curPath;
> +            curPath = &modifiedPath;
> +            pathIsMutable = true;
> +        }
> +        paint.getFillPath(*prevPath, curPath);
>
>         SkPaint noEffectPaint(paint);
>         SkSafeUnref(noEffectPaint.setPathEffect(NULL));
> -        drawPath(d, noEffectPath, noEffectPaint, NULL, true);
> +        drawPath(d, *curPath, noEffectPaint, NULL, true);
>         return;
>     }
> +
>     if (!setUpContentEntry(*d.fClipStack, *d.fClip, *d.fMatrix, paint)) {
>         return;
>     }
>
> -    SkPDFUtils::EmitPath(path, &fCurrentContentEntry->fContent);
> -    SkPDFUtils::PaintPath(paint.getStyle(), path.getFillType(),
> +    SkPDFUtils::EmitPath(*curPath, &fCurrentContentEntry->fContent);
> +    SkPDFUtils::PaintPath(paint.getStyle(), curPath->getFillType(),
>                           &fCurrentContentEntry->fContent);
>  }
>
>
>
>

[Steve VanDeBogart, 2011-05-11 23:18:21]

On 2011/05/10 16:27:07, reed1 wrote:
> getFillPath returns a new path that should be "filled": i.e. any
> stroking or patheffects have already been applied, so just a plain
> paint will do.
> 
> To modify the paint that was used to create the fill, you can do this:
> - paint.setStyle(SkPaint::kFill_Style);
> - paint.setPathEffect(NULL);

Thanks.  Can you take a look?  I don't think there's anything PDF specific in
this code.

[reed1, 2011-05-11 23:50:54]

the use of getFillPath() seems correct.

http://codereview.appspot.com/4531047/diff/3001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (left):

http://codereview.appspot.com/4531047/diff/3001/src/pdf/SkPDFDevice.cpp#oldco...
src/pdf/SkPDFDevice.cpp:694: SkPaint noEffectPaint(paint);
The safeunref is wrong here (I know its not part of this change). No harm, but
completely unnecessary.

noEffectPaint.setPathEffect(NULL) is all you need to say. It takes care of
unrefing the effect (if present). The return value for any of the setFooEffect
is always the parameter, not the prev value, so in this case it will always
return NULL.

[Steve VanDeBogart, 2011-05-11 23:57:44]

http://codereview.appspot.com/4531047/diff/3001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (left):

http://codereview.appspot.com/4531047/diff/3001/src/pdf/SkPDFDevice.cpp#oldco...
src/pdf/SkPDFDevice.cpp:694: SkPaint noEffectPaint(paint);
On 2011/05/11 23:50:54, reed1 wrote:
> The safeunref is wrong here (I know its not part of this change). No harm, but
> completely unnecessary.
> 
> noEffectPaint.setPathEffect(NULL) is all you need to say. It takes care of
> unrefing the effect (if present). The return value for any of the setFooEffect
> is always the parameter, not the prev value, so in this case it will always
> return NULL.

Thanks. Fixed.

[Chris Guillory, 2011-05-16 20:52:37]

LGTM.

http://codereview.appspot.com/4531047/diff/7001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4531047/diff/7001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:689: void SkPDFDevice::drawPath(const SkDraw& d, const
SkPath& origPath,
Optional: Rename in header?

http://codereview.appspot.com/4531047/diff/7001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:693: SkPath* curPath = (SkPath*)&origPath;
Nit: Use the more explicit const_cast<>?

http://codereview.appspot.com/4531047/diff/7001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:693: SkPath* curPath = (SkPath*)&origPath;
Optional: Can we do away with the prefix. curPath -> path? Or curPath -> pathPtr
like in SkDraw.cpp.

http://codereview.appspot.com/4531047/diff/7001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:704: prevPath->transform(*prePathMatrix, curPath);
Nit: Can we remove the prePath local and give use origPath directly?

[Steve VanDeBogart, 2011-05-17 18:36:55]

Thanks.

http://codereview.appspot.com/4531047/diff/7001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4531047/diff/7001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:689: void SkPDFDevice::drawPath(const SkDraw& d, const
SkPath& origPath,
On 2011/05/16 20:52:37, Chris Guillory wrote:
> Optional: Rename in header?

Done.

http://codereview.appspot.com/4531047/diff/7001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:693: SkPath* curPath = (SkPath*)&origPath;
On 2011/05/16 20:52:37, Chris Guillory wrote:
> Nit: Use the more explicit const_cast<>? 

Done.

http://codereview.appspot.com/4531047/diff/7001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:693: SkPath* curPath = (SkPath*)&origPath;
On 2011/05/16 20:52:37, Chris Guillory wrote:
> Optional: Can we do away with the prefix. curPath -> path? Or curPath ->
pathPtr
> like in SkDraw.cpp.

Done.

http://codereview.appspot.com/4531047/diff/7001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:704: prevPath->transform(*prePathMatrix, curPath);
On 2011/05/16 20:52:37, Chris Guillory wrote:
> Nit: Can we remove the prePath local and give use origPath directly?

I added it because I didn't think so, but looking at it more closely, looks like
yes - we'll never execute both !pathIsMutable blocks, only one or the other will
ever run.

Done.

[reed1, 2011-05-17 18:41:36]

LGTM