Fixed lingering gpu-path AA clip mask generation bug

The bug this delivery fixes is that for some cases where the gpu clip mask generation path combines gpu-drawn and SW-drawn clip masks the final mask would be incorrect. The problematic behavior was in the SW path renderer which has 3 issues (when it comes to clip mask rendering)

1) It uses the context's matrix to draw. This can/did lead to double application of the transforms. (This can be worked around by setting the Context's matrix to I - but that is rather side-effecty).

2) It uses the current clip to bound the size of the drawn area. During clip mask generation we are in the process of creating the clip mask and when we move the mask to the UL corner of the texture this bounding process can clip off part of the result.

3) The SW path renderer doesn't use the draw state's matrix. The clip mask manager alters the draw state's matrix to offset the mask (to the UL corner). Because the other path renderers respect the matrix but the SW path renderer does not this lead to mis-registration of SW path rendered masks.

The solution implemented here (rather than passing special purpose flags to the SW path renderer) is to bypass the SW path renderer and directly call the SWMaskHelper (like the full SW path does). This avoids monkeying with get_path_and_clip_bounds (which performs the intersection with the current clip bound) and messing with the translate passed to the old sw_draw_path_to_mask_texture method.

[bsalomon, 2012-06-29 17:55:27]

I think the SWMH class needs some docs.

I also don't get the bit about the SW PR not using the draw state's matrix. Is
that a bug in the SW PR? It looks like it calls SWMH with the useMatrix bool set
to true. Does that not translate into using the ctx's drawstate's matrix?

http://codereview.appspot.com/6351055/diff/1/src/gpu/GrSWMaskHelper.h
File src/gpu/GrSWMaskHelper.h (right):

http://codereview.appspot.com/6351055/diff/1/src/gpu/GrSWMaskHelper.h#newcode38
src/gpu/GrSWMaskHelper.h:38: GrPathFill fill, bool antiAlias, GrColor color);
I missed this before, but does color really mean alpha? IIRC the underlying
bitmap is always A8.

http://codereview.appspot.com/6351055/diff/1/src/gpu/GrSWMaskHelper.h#newcode52
src/gpu/GrSWMaskHelper.h:52: bool drawToTexture(const SkPath& clientPath,
It's not obvious to me from the header how this class is used. It appears like
there is a stateful and a non-stateful way of using it. The stateful way is:

init()
draw()
draw()
...
draw()
toTexture() or getTexture()

and this new function is the all-in-one function.  What happens if I call this
if I've already called init()? Is that legal?

Should this be a static function that takes a SWMH and devolves into making the
stateful calls?


Does "client" in clientPath convey something or should we call this path? (or
remove than name :-) )

http://codereview.appspot.com/6351055/diff/1/src/gpu/GrSWMaskHelper.h#newcode53
src/gpu/GrSWMaskHelper.h:53: const GrIRect& pathDevBounds,
Is this used as a clip rect? It feels like the param name is a hold-over from
the original user of this class (sw pr) who happens to compute this rect based
on path's bounds (and other rects). If it's just a rect that the path is clipped
against let's call it clipRect or devClipRect.

http://codereview.appspot.com/6351055/diff/1/src/gpu/GrSWMaskHelper.h#newcode55
src/gpu/GrSWMaskHelper.h:55: const GrPoint* translate,
Let's document when the translate is applied (before the matrix).

http://codereview.appspot.com/6351055/diff/1/src/gpu/GrSWMaskHelper.h#newcode58
src/gpu/GrSWMaskHelper.h:58: bool useMatrix);
The useMatrix param seems somewhat confusing to me. We always use *some* matrix.
Could we just take a Matrix* and say NULL means identity?  Then if someone (e.g.
the sw pr) wants to use the context matrix they can just pass it in.

[robertphillips, 2012-06-29 20:38:38]

I believe this addresses all the review issues. Note that the SWPR now uses the
drawState's matrix rather than the Context's.

http://codereview.appspot.com/6351055/diff/1/src/gpu/GrSWMaskHelper.h
File src/gpu/GrSWMaskHelper.h (right):

http://codereview.appspot.com/6351055/diff/1/src/gpu/GrSWMaskHelper.h#newcode38
src/gpu/GrSWMaskHelper.h:38: GrPathFill fill, bool antiAlias, GrColor color);
On 2012/06/29 17:55:27, bsalomon wrote:
> I missed this before, but does color really mean alpha? IIRC the underlying
> bitmap is always A8.

Done.

http://codereview.appspot.com/6351055/diff/1/src/gpu/GrSWMaskHelper.h#newcode52
src/gpu/GrSWMaskHelper.h:52: bool drawToTexture(const SkPath& clientPath,
On 2012/06/29 17:55:27, bsalomon wrote:
> It's not obvious to me from the header how this class is used. It appears like
> there is a stateful and a non-stateful way of using it. The stateful way is:
> 
> init()
> draw()
> draw()
> ...
> draw()
> toTexture() or getTexture()
> 
> and this new function is the all-in-one function.  What happens if I call this
> if I've already called init()? Is that legal?
> 
> Should this be a static function that takes a SWMH and devolves into making
the
> stateful calls?
> 
> 
> Does "client" in clientPath convey something or should we call this path? (or
> remove than name :-) )

Done.

http://codereview.appspot.com/6351055/diff/1/src/gpu/GrSWMaskHelper.h#newcode53
src/gpu/GrSWMaskHelper.h:53: const GrIRect& pathDevBounds,
On 2012/06/29 17:55:27, bsalomon wrote:
> Is this used as a clip rect? It feels like the param name is a hold-over from
> the original user of this class (sw pr) who happens to compute this rect based
> on path's bounds (and other rects). If it's just a rect that the path is
clipped
> against let's call it clipRect or devClipRect.

Done.

http://codereview.appspot.com/6351055/diff/1/src/gpu/GrSWMaskHelper.h#newcode55
src/gpu/GrSWMaskHelper.h:55: const GrPoint* translate,
On 2012/06/29 17:55:27, bsalomon wrote:
> Let's document when the translate is applied (before the matrix).

Done.

http://codereview.appspot.com/6351055/diff/1/src/gpu/GrSWMaskHelper.h#newcode58
src/gpu/GrSWMaskHelper.h:58: bool useMatrix);
On 2012/06/29 17:55:27, bsalomon wrote:
> The useMatrix param seems somewhat confusing to me. We always use *some*
matrix.
> Could we just take a Matrix* and say NULL means identity?  Then if someone
(e.g.
> the sw pr) wants to use the context matrix they can just pass it in.

Done.

[bsalomon, 2012-06-29 21:25:20]

LGTM modulo nits

http://codereview.appspot.com/6351055/diff/9002/src/gpu/GrSWMaskHelper.h
File src/gpu/GrSWMaskHelper.h (right):

http://codereview.appspot.com/6351055/diff/9002/src/gpu/GrSWMaskHelper.h#newc...
src/gpu/GrSWMaskHelper.h:73: static bool drawToTexture(GrContext* context,
DrawToTexture

http://codereview.appspot.com/6351055/diff/9002/src/gpu/GrSoftwarePathRendere...
File src/gpu/GrSoftwarePathRenderer.cpp (right):

http://codereview.appspot.com/6351055/diff/9002/src/gpu/GrSoftwarePathRendere...
src/gpu/GrSoftwarePathRenderer.cpp:138: &ast, antiAlias, &vm)) {
indentation

[robertphillips, 2012-06-29 21:39:45]

committed as r4416

http://codereview.appspot.com/6351055/diff/9002/src/gpu/GrSWMaskHelper.h
File src/gpu/GrSWMaskHelper.h (right):

http://codereview.appspot.com/6351055/diff/9002/src/gpu/GrSWMaskHelper.h#newc...
src/gpu/GrSWMaskHelper.h:73: static bool drawToTexture(GrContext* context,
On 2012/06/29 21:25:20, bsalomon wrote:
> DrawToTexture

Done.

http://codereview.appspot.com/6351055/diff/9002/src/gpu/GrSoftwarePathRendere...
File src/gpu/GrSoftwarePathRenderer.cpp (right):

http://codereview.appspot.com/6351055/diff/9002/src/gpu/GrSoftwarePathRendere...
src/gpu/GrSoftwarePathRenderer.cpp:138: &ast, antiAlias, &vm)) {
On 2012/06/29 21:25:20, bsalomon wrote:
> indentation

Done.