fix the visual difference of raster vs gpu -- inverse_paths

fix the visual difference of raster vs gpu -- inverse_paths

If a path has no verbs, it is empty. If a path has verbs, but both the width and height of the bounds are zero. the path is empty too. This situation happens when you add an empty rect or circle... , say a rect{x, y, width, height} = {100, 100, 0, 0}, to a path.

For 8888 config, drawPath() checked the bounds.
For gpu config, it doesn't.

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

[yunchao, 2014-02-14 06:27:57]

PTAL

[bsalomon, 2014-02-14 13:56:38]

https://codereview.chromium.org/166023002/diff/40001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/166023002/diff/40001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:1742: if (SkScalarNearlyZero(r.width()) &&
SkScalarNearlyZero(r.height())) {
It's not obvious to me that this is the right check.  Where does the sw
rasterizer check the bounds? Is that check redundant with this change? Also, why
nearly zero and not simply path.getBounds().isEmpty()?

[yunchao, 2014-02-17 09:54:32]

https://codereview.chromium.org/166023002/diff/40001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/166023002/diff/40001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:1742: if (SkScalarNearlyZero(r.width()) &&
SkScalarNearlyZero(r.height())) {
On 2014/02/14 13:56:40, bsalomon wrote:
> It's not obvious to me that this is the right check.  Where does the sw
> rasterizer check the bounds? Is that check redundant with this change? Also,
why
> nearly zero and not simply path.getBounds().isEmpty()?

Hi Brian, SW rasterizer check the bounds in some cases, the code scattered in
different files for FillPath, AntiFillPath. But Hairline Path and Anti-Hairline
Path didn't check the bounds for stroked path. HW rasterizer didn't check the
bounds at all.

path.getBounds().isEmpty() basically equals to r.width() == 0 || r.height() ==
0, not r.width() == 0 && r.height() == 0. So it is not suitable for this path
who has only a line. This kind path is not empty. 

Personally, I think check the bounds in SkCanvas::drawPath() is appropriate.
checking path bounds is a revision of path.isEmpty(). It just cover all
practical empty paths -- although these paths have verbs, they are not treated
as empty paths by SkPath::isEmpty(). 

If you prefer to check the bounds in different files for different situation, I
will upload a corresponding patch.

[bsalomon, 2014-02-17 14:47:29]

I'm adding Stephen who may have an opinion on this, especially as it relates to
the svg spec. We probably should weigh what other 2D libraries do and what web
specs say. Though, I suspect this check is probably the right direction.

Thanks for the clarification about checking for empty in both x and y. That
makes sense. I'm still not sure about the nearly zero check. Also, it seems like
the check is relying on a particular implementation of SkPath and SkRect, where
a zero-area path has a rect that has l == r and t == b and never r < l or b < t.
I'd propose (path.getBounds().width() <= 0 a&& path.getBounds().height() <= 0).

I agree that if we do need this check then SkCanvas is the right place to do it
so that we don't have to check this in device-specific code to get consistent
behavior.

Tangential to this change, SkPath::isEmpty() has an ambiguous name. The name
could just as easily mean either "has zero area" or "has no verbs/points". I
think we should consider changing it to isReset() or something like that and
auditing callers to see if this same mistake is made anywhere else.

[yunchao, 2014-02-18 06:15:16]

On 2014/02/17 14:47:29, bsalomon wrote:
> I'm adding Stephen who may have an opinion on this, especially as it relates
to
> the svg spec. We probably should weigh what other 2D libraries do and what web
> specs say. Though, I suspect this check is probably the right direction.
> 
> Thanks for the clarification about checking for empty in both x and y. That
> makes sense. I'm still not sure about the nearly zero check. Also, it seems
like
> the check is relying on a particular implementation of SkPath and SkRect,
where
> a zero-area path has a rect that has l == r and t == b and never r < l or b <
t.
> I'd propose (path.getBounds().width() <= 0 a&& path.getBounds().height() <=
0).
> 
> I agree that if we do need this check then SkCanvas is the right place to do
it
> so that we don't have to check this in device-specific code to get consistent
> behavior.
> 
> Tangential to this change, SkPath::isEmpty() has an ambiguous name. The name
> could just as easily mean either "has zero area" or "has no verbs/points". I
> think we should consider changing it to isReset() or something like that and
> auditing callers to see if this same mistake is made anywhere else.

Yeah, we need to double check with other spec, such as svg spec, and other 2D
lib's spec, although I think this is reasonable.

About the SkPath::isEmpty(), I totally agree with you. Another problem is that
some APIs in Skia is not clearly defined, either. Take SkRect::isEmpty() as an
example, In gm case strokerect, there are cases r < l or b < t, but Skia draws
rects for these cases. According to SkRect::isEmpty(), it should draw nothing. I
suspect that using those ambiguous or not-clearly-defined APIs lead to the
visual difference of raster vs gpu in some other gm cases, not only this one.

[bsalomon, 2014-02-18 17:58:10]

https://codereview.chromium.org/166023002/diff/130001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/166023002/diff/130001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:1741: SkRect r = path.getBounds();
const SkRect& ?

[yunchao, 2014-02-19 01:36:59]

https://codereview.chromium.org/166023002/diff/130001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/166023002/diff/130001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:1741: SkRect r = path.getBounds();
On 2014/02/18 17:58:11, bsalomon wrote:
> const SkRect& ?

Done.

[bsalomon, 2014-02-20 19:44:18]

On 2014/02/19 01:36:59, Richard He wrote:
> https://codereview.chromium.org/166023002/diff/130001/src/core/SkCanvas.cpp
> File src/core/SkCanvas.cpp (right):
> 
>
https://codereview.chromium.org/166023002/diff/130001/src/core/SkCanvas.cpp#n...
> src/core/SkCanvas.cpp:1741: SkRect r = path.getBounds();
> On 2014/02/18 17:58:11, bsalomon wrote:
> > const SkRect& ?
> 
> Done.

lgtm

[yunchao, 2014-02-21 02:38:13]

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

[commit-bot: I haz the power, 2014-02-21 02:38:19]

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

[commit-bot: I haz the power, 2014-02-21 05:42:59]

Change committed as 13526

[reed2, 2014-02-21 10:48:49]

https://codereview.chromium.org/166023002/diff/260001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/166023002/diff/260001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:1751: if (r.width() <= 0 && r.height() <= 0) {
Do we have unit-tests added to exercise this?
Does this catch a width > 0 but height < 0 filled path (e.g. horizontal line as
a path)?
Do we perform the same checks on drawRect/drawOval?

[yunchao, 2014-02-24 03:07:20]

Please correct me if something is wrong.

https://codereview.chromium.org/166023002/diff/260001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/166023002/diff/260001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:1751: if (r.width() <= 0 && r.height() <= 0) {
On 2014/02/21 10:48:49, reed2 wrote:
> Do we have unit-tests added to exercise this?
> Does this catch a width > 0 but height < 0 filled path (e.g. horizontal line
as
> a path)?
> Do we perform the same checks on drawRect/drawOval?

Hi Mike,
Do you think we need to add a unit-tests like tests/GpuDrawPathTest.cpp to show
that inverse-filling an empty path will not crash? The previous tests didn't
test this in unit-tests too. If we want to test the images generated by drawPath
for inverse-filled empty path, gm/inversepaths have done this.

a width > 0 but height < 0 filled path is not included in this fix, I think.
because It is not an empty path. It is a hairline, right? Skia can draw it
correctly.

Rects and Ovals have no FillType if they are not added to a path, so they didn't
need to check this. If they are empty, just draw nothing. GM cases have tested
empty rects/ovals too.

[reed1, 2014-02-24 13:33:14]

On 2014/02/24 03:07:20, Richard He wrote:
> Please correct me if something is wrong.
> 
> https://codereview.chromium.org/166023002/diff/260001/src/core/SkCanvas.cpp
> File src/core/SkCanvas.cpp (right):
> 
>
https://codereview.chromium.org/166023002/diff/260001/src/core/SkCanvas.cpp#n...
> src/core/SkCanvas.cpp:1751: if (r.width() <= 0 && r.height() <= 0) {
> On 2014/02/21 10:48:49, reed2 wrote:
> > Do we have unit-tests added to exercise this?
> > Does this catch a width > 0 but height < 0 filled path (e.g. horizontal line
> as
> > a path)?
> > Do we perform the same checks on drawRect/drawOval?
> 
> Hi Mike,
> Do you think we need to add a unit-tests like tests/GpuDrawPathTest.cpp to
show
> that inverse-filling an empty path will not crash? The previous tests didn't
> test this in unit-tests too. If we want to test the images generated by
drawPath
> for inverse-filled empty path, gm/inversepaths have done this.
> 
> a width > 0 but height < 0 filled path is not included in this fix, I think.
> because It is not an empty path. It is a hairline, right? Skia can draw it
> correctly.
> 
> Rects and Ovals have no FillType if they are not added to a path, so they
didn't
> need to check this. If they are empty, just draw nothing. GM cases have tested
> empty rects/ovals too.

This is the only place in all of skia where we use the AND of width==0 and
height==0 to tell us something. I don't think it is wrong, but I think it (like
the version before perhaps) is a little misleading -- it could be read to say
"iff this condition, we should fill everything". 

For example, if only width==0 or only height==0, and the paint is FILL, then we
could also take the paint-everything path. Perhaps we can document that this is
just a quick test, but not exhaustive.

Any code/checks that we put in SkCanvas and *not* in the specific device
backends just has to be that much more clear/useful.