fix the error that path is inversed for stroke and strokeAndFill styles

fix the error that path is inversed for stroke and strokeAndFill styles.

However, because hairline stroke + fill = fill (see src/core/SkStrokeRec.cpp), strokeAndFill will be thought as fill style when paint.getStrokeWidth() <= 0, this edge case can be inverse-filled.

BUG=skia:2222
Committed: http://code.google.com/p/skia/source/detail?r=14561

[yunchao, 2014-03-03 09:32:25]

PTAL, thanks.

[reed1, 2014-03-03 15:24:32]

Is it sufficient to just change our internal stroker to never set "inverse" on
the result?

other questions:

what is skia supposed to do when stroking...
- empty path
- 0 x 0 non-empty path (i.e. all control points are equal)
?

[reed1, 2014-03-03 15:26:16]

I wonder if we can write unittests to assert some of the correct behavior (once
we decide what it is) w/o needing GMs...

e.g. if an empty path is to draw nothing when stroked, we can setup an offscreen
surface, draw the path, and then inspect all the pixels and assert they are
unchanged.

[yunchao, 2014-03-04 02:51:40]

On 2014/03/03 15:24:32, reed1 wrote:
> Is it sufficient to just change our internal stroker to never set "inverse" on
> the result?
> 
> other questions:
> 
> what is skia supposed to do when stroking...
> - empty path
> - 0 x 0 non-empty path (i.e. all control points are equal)
> ?

Hi Mike, I think its sufficient. The internal stroker will got the Style info of
SkPaint instance. After set non-inverse-filling to the path right before
blitters/pathRenders rasterization, the blitters or pathRenderers should
guarantee that they can draw the geometry correctly.

As I stated in http://skbug:2222, I think the caller should tell
blitters/pathRenderers the right behavior. However, changing the impl logic in
all blitters/pathRenderers to avoid inverse-stroking is not a good solution.

For empty path and 0x0 non-empty path(path.getBounds() will be 0x0), I think we
should draw nothing when stroking, even the path is inverse-fillstyle. SkPaint
should have higher priority than SkPath to control the final bitmap. If user
indeed want to inverse-filling, they can set current painter to correct Style.

[yunchao, 2014-03-04 03:02:18]

On 2014/03/03 15:26:16, reed1 wrote:
> I wonder if we can write unittests to assert some of the correct behavior
(once
> we decide what it is) w/o needing GMs...
> 
> e.g. if an empty path is to draw nothing when stroked, we can setup an
offscreen
> surface, draw the path, and then inspect all the pixels and assert they are
> unchanged.

Yeah, we can do this for empty path and 0x0 non-empty path. But GM cases for
fidelity test for inverse-filling and avoid inverse-stroking are necessary too,
and the former case can be included in GM cases. But all these GM cases can not
be well tested in unittests. BTW, I think more important thing in Skia is to
make it clear what is the correct behaviors for many edge cases. How to test
them is secondary. What's about your oponion?

[yunchao, 2014-04-23 09:02:18]

ping... please see the comments at skbug:2222

[bsalomon, 2014-04-23 15:06:12]

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

https://codereview.chromium.org/183683010/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:1210: // make sure the path will not be inverse-stroked
Shouldn't applyToPath just return a path with the correct fill type?

We shouldn't need to look at stroke after calling applyToPath since applyToPath
should return a path that just needs to be filled.

[yunchao, 2014-04-24 16:12:42]

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

https://codereview.chromium.org/183683010/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:1210: // make sure the path will not be inverse-stroked
On 2014/04/23 15:06:12, bsalomon wrote:
> Shouldn't applyToPath just return a path with the correct fill type?
> 
> We shouldn't need to look at stroke after calling applyToPath since
applyToPath
> should return a path that just needs to be filled.

applyToPath doesn't change the style. For a stroke path, it will record the
inner contour and outer contour for filling the area between these two contours.
So it will inverse-stroke for stroke style and strokeAndFill style if we don't
exclude these styles here.

[bsalomon, 2014-04-24 17:33:21]

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

https://codereview.chromium.org/183683010/diff/40001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:1210: // make sure the path will not be inverse-stroked
On 2014/04/24 16:12:43, Richard Ho wrote:
> On 2014/04/23 15:06:12, bsalomon wrote:
> > Shouldn't applyToPath just return a path with the correct fill type?
> > 
> > We shouldn't need to look at stroke after calling applyToPath since
> applyToPath
> > should return a path that just needs to be filled.
> 
> applyToPath doesn't change the style. For a stroke path, it will record the
> inner contour and outer contour for filling the area between these two
contours.
> So it will inverse-stroke for stroke style and strokeAndFill style if we don't
> exclude these styles here.

It *should* set the correct fill type.

[yunchao, 2014-04-25 09:05:16]

On 2014/04/24 17:33:21, bsalomon wrote:
> https://codereview.chromium.org/183683010/diff/40001/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (right):
> 
>
https://codereview.chromium.org/183683010/diff/40001/src/gpu/GrContext.cpp#ne...
> src/gpu/GrContext.cpp:1210: // make sure the path will not be inverse-stroked
> On 2014/04/24 16:12:43, Richard Ho wrote:
> > On 2014/04/23 15:06:12, bsalomon wrote:
> > > Shouldn't applyToPath just return a path with the correct fill type?
> > > 
> > > We shouldn't need to look at stroke after calling applyToPath since
> > applyToPath
> > > should return a path that just needs to be filled.
> > 
> > applyToPath doesn't change the style. For a stroke path, it will record the
> > inner contour and outer contour for filling the area between these two
> contours.
> > So it will inverse-stroke for stroke style and strokeAndFill style if we
don't
> > exclude these styles here.
> 
> It *should* set the correct fill type.

Agree, It should correctly set the style in this function. We should not call
toggleInverseFillType() wrongly for stroke and strokeAndFill styles in
applyToPath(), then immediately call toggleInverseFillType() again  after
applyToPath(). I have updated the code, please take a look.

[bsalomon, 2014-04-25 13:30:48]

This lgtm but I'd like reed@ to have a look as well.

[yunchao, 2014-04-25 14:26:34]

On 2014/04/25 13:30:48, bsalomon wrote:
> This lgtm but I'd like reed@ to have a look as well.

Hi Mike, could you have a look?

[yunchao, 2014-04-29 16:00:38]

Ping... Mike, could you make a double check about this CL?

[yunchao, 2014-05-01 15:16:38]

I will land this patch. Please revert it if you think this CL is not
appropriate.

[yunchao, 2014-05-04 03:37:32]

The CQ bit was checked by yunchao.he@intel.com

[commit-bot: I haz the power, 2014-05-04 03:37:36]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/yunchao.he@intel.com/183683010/90001

[commit-bot: I haz the power, 2014-05-04 03:43:23]

Change committed as 14561

[yunchao, 2014-05-04 06:11:17]

A revert of this CL has been created in
https://codereview.chromium.org/267923008/ by yunchao.he@intel.com.

The reason for reverting is: broke the original unittest EmptyPath, will update
that unittest accordingly..

[yunchao, 2014-05-04 06:16:17]

On 2014/05/04 06:11:17, Richard Ho wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/267923008/ by mailto:yunchao.he@intel.com.
> 
> The reason for reverting is: broke the original unittest EmptyPath, will
update
> that unittest accordingly..

please see the CL to update the unittest accordingly:
https://codereview.chromium.org/262113003/

[reed2, 2014-05-04 18:05:31]

A revert of this CL has been created in
https://codereview.chromium.org/269903002/ by reed@chromium.org.

The reason for reverting is: broke unittests.

[yunchao, 2014-05-05 01:36:05]

On 2014/05/04 18:05:31, reed2 wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/269903002/ by mailto:reed@chromium.org.
> 
> The reason for reverting is: broke unittests.

update the unittest accordingly, PTAL. Thanks.

[bsalomon, 2014-05-13 08:00:00]

On 2014/05/05 01:36:05, Richard Ho wrote:
> On 2014/05/04 18:05:31, reed2 wrote:
> > A revert of this CL has been created in
> > https://codereview.chromium.org/269903002/ by mailto:reed@chromium.org.
> > 
> > The reason for reverting is: broke unittests.
> 
> update the unittest accordingly, PTAL. Thanks.

If we're going to remove kStrokeAndFill then I'd prefer to hold off on this
until after that is completed.

[yunchao, 2014-05-13 08:50:40]

On 2014/05/13 08:00:00, bsalomon wrote:
> On 2014/05/05 01:36:05, Richard Ho wrote:
> > On 2014/05/04 18:05:31, reed2 wrote:
> > > A revert of this CL has been created in
> > > https://codereview.chromium.org/269903002/ by mailto:reed@chromium.org.
> > > 
> > > The reason for reverting is: broke unittests.
> > 
> > update the unittest accordingly, PTAL. Thanks.
> 
> If we're going to remove kStrokeAndFill then I'd prefer to hold off on this
> until after that is completed.

OK.

[yunchao, 2014-05-14 15:52:11]

On 2014/05/13 08:50:40, Richard Ho wrote:
> On 2014/05/13 08:00:00, bsalomon wrote:
> > On 2014/05/05 01:36:05, Richard Ho wrote:
> > > On 2014/05/04 18:05:31, reed2 wrote:
> > > > A revert of this CL has been created in
> > > > https://codereview.chromium.org/269903002/ by mailto:reed@chromium.org.
> > > > 
> > > > The reason for reverting is: broke unittests.
> > > 
> > > update the unittest accordingly, PTAL. Thanks.
> > 
> > If we're going to remove kStrokeAndFill then I'd prefer to hold off on this
> > until after that is completed.
> 
> OK.

pending on the removal of kStrokeAndFill, also see
https://codereview.chromium.org/262113003/