Do not apply hairline optimization for paths if nv_path_rendering is used

Apply hairline optimization only if the path renderer wants it

Make the decision to convert thin, non-hairline paths to hairline
paths at the renderer level.

The current nv_path_rendering implementation does not render
hairlines. Rendering the hairlines with normal renderers cause
unneccessary gl program changes, which is quite slow.

Changes the behavior of non-nv_path_rendering paths to always perform
the optimization if the shape ends up being painted by a renderer that
wants the optimization. Previously the optimization was applied only
when callgraph started with SkCanvas::drawPath.

Applies the optimization for GrAAHairLineRenderer and
GrDefaultPathRenderer.

This changes gm results for dashing3_{msaa4,gpu} and drawlooper_msaa4.

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

[Kimmo Kinnunen, 2013-10-24 12:28:46]

Not so neat place to call GrContext::getGpu() and related things. It's not
obvious to me where to put this without extending GrContext..

[bsalomon, 2013-10-24 13:28:35]

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

https://codereview.chromium.org/38573007/diff/1/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:859: bool treat_paint_as_hairline(const SkPaint& paint,
const GrContext* context, SkScalar* hairlineCoverage) {
Kimmo, maybe we can do this check from GrContext::drawPath()?

[Kimmo Kinnunen, 2013-10-30 09:33:41]

How about this?

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

https://codereview.chromium.org/38573007/diff/1/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:859: bool treat_paint_as_hairline(const SkPaint& paint,
const GrContext* context, SkScalar* hairlineCoverage) {
On 2013/10/24 13:28:36, bsalomon wrote:
> Kimmo, maybe we can do this check from GrContext::drawPath()?

Done.

[bsalomon, 2013-10-30 14:37:38]

https://codereview.chromium.org/38573007/diff/60001/src/core/SkDrawProcs.h
File src/core/SkDrawProcs.h (right):

https://codereview.chromium.org/38573007/diff/60001/src/core/SkDrawProcs.h#ne...
src/core/SkDrawProcs.h:72: SkScalar* coverage) {
param alignment

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

https://codereview.chromium.org/38573007/diff/60001/src/gpu/GrContext.cpp#new...
src/gpu/GrContext.cpp:1185: if (!paint.isAntiAlias()) {
Currently GrContext tries to convert the GrPaint to a GrDrawState early and then
forget about the GrPaint. It looks like there are two reasons for passing the
paint down to internalDrawPath in this CL.

1) To get this anti-alias flag. Currently we cannot just use the "useAA" param
to internalDrawPath(). The useAA param really means "do shader based-aa" and we
have this logic replicated in a handful places:

    bool useAA = paint.isAntiAlias() &&
                 !target->getDrawState().getRenderTarget()->isMultisampled() &&
                 !disable_coverage_aa_for_blend(target);

I think we should just pass paint.isAntiAlias() to internal draw path and have
that function do this logic.

Some of the callers need to do it anyway because they also pass useAA to helper
classes. I'm happy to go back later and make GrOvalRenderer, GrAARectRenderer,
etc take paint.isAntiAlias() rather than require their callers to do the tests
above.

2) To get the coverage. But that is already copied to GrDrawState.

[Kimmo Kinnunen, 2013-11-01 14:56:19]

https://codereview.chromium.org/38573007/diff/60001/src/core/SkDrawProcs.h
File src/core/SkDrawProcs.h (right):

https://codereview.chromium.org/38573007/diff/60001/src/core/SkDrawProcs.h#ne...
src/core/SkDrawProcs.h:72: SkScalar* coverage) {
On 2013/10/30 14:37:39, bsalomon wrote:
> param alignment

Done.

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

https://codereview.chromium.org/38573007/diff/60001/src/gpu/GrContext.cpp#new...
src/gpu/GrContext.cpp:1185: if (!paint.isAntiAlias()) {
On 2013/10/30 14:37:39, bsalomon wrote:
> Currently GrContext tries to convert the GrPaint to a GrDrawState early and
then
> forget about the GrPaint. It looks like there are two reasons for passing the
> paint down to internalDrawPath in this CL.
> 
> 1) To get this anti-alias flag. Currently we cannot just use the "useAA" param
> to internalDrawPath(). The useAA param really means "do shader based-aa" and
we
> have this logic replicated in a handful places:
> 
>     bool useAA = paint.isAntiAlias() &&
>                  !target->getDrawState().getRenderTarget()->isMultisampled()
&&
>                  !disable_coverage_aa_for_blend(target);
> 
> I think we should just pass paint.isAntiAlias() to internal draw path and have
> that function do this logic.
> 
> Some of the callers need to do it anyway because they also pass useAA to
helper
> classes. I'm happy to go back later and make GrOvalRenderer, GrAARectRenderer,
> etc take paint.isAntiAlias() rather than require their callers to do the tests
> above.
> 
> 2) To get the coverage. But that is already copied to GrDrawState.

Done.

[Kimmo Kinnunen, 2013-11-07 13:37:36]

Brian, ping. Was this anything you intended?

[bsalomon, 2013-11-07 16:11:56]

On 2013/11/07 13:37:36, kkinnunen wrote:
> Brian, ping. Was this anything you intended?

Thanks for the modifications, Kimmo. LGTM.

[commit-bot: I haz the power, 2013-11-08 06:09:00]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/38573007/130001

[commit-bot: I haz the power, 2013-11-08 06:19:39]

Change committed as 12185

[Kimmo Kinnunen, 2013-11-08 06:25:02]

Thanks for the review.
The dashing gm pictures would need to be updated. Is there a way for me to
update them or how would that work?

[bsalomon, 2013-11-08 14:49:51]

On 2013/11/08 06:25:02, kkinnunen wrote:
> Thanks for the review.
> The dashing gm pictures would need to be updated. Is there a way for me to
> update them or how would that work?

Hey Kimmo,

I wound up reverting this change because some images got worse on the Xoom bot
(you should be on a thread started by Jim Van Verth). In general if you know
ahead of time from local testing or bots that images will change in ways that
are acceptable then we add the tests to a file
expectations/gm/ignored-tests.txt. This will cause the bots to ignore the test.
After the bots have had a change to generate new images you rebaseline the
images and remove the test from the ignored-tests.txt file. Here is some info
about the rebaseline process:
https://sites.google.com/site/skiadocs/developer-documentation/using-gm-golde...

[Kimmo Kinnunen, 2013-11-13 13:27:26]

On 08.11.2013 16:15, Jim Van Verth wrote:
> has caused a regression in the ovals GM on the Xoom. The hairlines
> have gone from being nice and smooth to having gaps. The super thin
> ovals (one pixel wide) in particular look pretty bad.

On 08.11.2013 16:41, Brian Salomon wrote:
> Several others look worse on the Xoom too (e.g. patheffect)
> 
> Before: 
>
http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/p...
>
>  After: 
>
http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/p...
>
>  I'll revert for now.

I couldn't repro the XOOM expectations. On my xoom, the gaps were there in the
images without the patch too. The gaps are painted by the
GrSoftwarePathRenderer.

The assumption is that widening the cases where hairline optimization was
applied, most of the cases went from GrXXPathRenderer to GrSoftwarePathRenderer.
However, reading the code + running on Xoom, I couldn't figure out which
GrXXPathRenderer should have painted the shapes. Most of the special case
renderers need standard derivates extension, which Xoom does not advertise.


I uploaded a new version of the patch that tries to be less invasive in some
sense. I wonder if it would be ok? I can not verify it on Xoom as described
above, though..

[bsalomon, 2013-11-13 18:23:06]

On 2013/11/13 13:27:26, kkinnunen wrote:
> On 08.11.2013 16:15, Jim Van Verth wrote:
> > has caused a regression in the ovals GM on the Xoom. The hairlines
> > have gone from being nice and smooth to having gaps. The super thin
> > ovals (one pixel wide) in particular look pretty bad.
> 
> On 08.11.2013 16:41, Brian Salomon wrote:
> > Several others look worse on the Xoom too (e.g. patheffect)
> > 
> > Before: 
> >
>
http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/p...
> >
> >  After: 
> >
>
http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/p...
> >
> >  I'll revert for now.
> 
> I couldn't repro the XOOM expectations. On my xoom, the gaps were there in the
> images without the patch too. The gaps are painted by the
> GrSoftwarePathRenderer.
> 
> The assumption is that widening the cases where hairline optimization was
> applied, most of the cases went from GrXXPathRenderer to
GrSoftwarePathRenderer.
> However, reading the code + running on Xoom, I couldn't figure out which
> GrXXPathRenderer should have painted the shapes. Most of the special case
> renderers need standard derivates extension, which Xoom does not advertise.
> 
> 
> I uploaded a new version of the patch that tries to be less invasive in some
> sense. I wonder if it would be ok? I can not verify it on Xoom as described
> above, though..

[bsalomon, 2013-11-13 18:23:46]

> > 
> > I uploaded a new version of the patch that tries to be less invasive in some
> > sense. I wonder if it would be ok? I can not verify it on Xoom as described
> > above, though..

Oops, accidentally hit Send when on an empty msg.

I'll submit to the Xoom bot and see what happens.

[bsalomon, 2013-11-13 18:26:34]

Hey Ravi, any idea why there is no "try more bots" link on this issue?

[jvanverth1, 2013-11-13 18:44:53]

While rebaselining I noticed that some of the hairline roundrects in
pathinterior and rrect also have gaps on the Xoom. So there may be a separate
issue. I've filed bug 1838.

Also, the bots are ignoring any GMs with roundrects at the moment so I'm not
sure submitting to the trybots will show anything. I'm trying to bring them
back, little by little.

[bsalomon, 2013-11-13 19:12:39]

On 2013/11/13 18:44:53, JimVV wrote:
> While rebaselining I noticed that some of the hairline roundrects in
> pathinterior and rrect also have gaps on the Xoom. So there may be a separate
> issue. I've filed bug 1838.
> 
> Also, the bots are ignoring any GMs with roundrects at the moment so I'm not
> sure submitting to the trybots will show anything. I'm trying to bring them
> back, little by little.

Ok, so let's hold off on this CL until we can resolve 1838. Or we can remove
tests from the ignored-tests.txt in this patch *just for the try bot run*.

[Kimmo Kinnunen, 2013-11-20 09:53:38]

On 2013/11/13 19:12:39, bsalomon wrote:
> On 2013/11/13 18:44:53, JimVV wrote:
> > While rebaselining I noticed that some of the hairline roundrects in
> > pathinterior and rrect also have gaps on the Xoom. So there may be a
separate
> > issue. I've filed bug 1838.
> Ok, so let's hold off on this CL until we can resolve 1838. Or we can remove
> tests from the ignored-tests.txt in this patch *just for the try bot run*.

I rootcaused the bug 1838. By reverting the mentioned commit, hairline ovals and
paths are nice again.

I confirmed that the newest patch does not change the state of the paths on
xoom.

I wonder if we still need to block this until somebody starts to work on bug
1838?

[jvanverth1, 2013-11-20 14:31:32]

On 2013/11/20 09:53:38, kkinnunen wrote:
> On 2013/11/13 19:12:39, bsalomon wrote:
> > On 2013/11/13 18:44:53, JimVV wrote:
> > > While rebaselining I noticed that some of the hairline roundrects in
> > > pathinterior and rrect also have gaps on the Xoom. So there may be a
> separate
> > > issue. I've filed bug 1838.
> > Ok, so let's hold off on this CL until we can resolve 1838. Or we can remove
> > tests from the ignored-tests.txt in this patch *just for the try bot run*.
> 
> I rootcaused the bug 1838. By reverting the mentioned commit, hairline ovals
and
> paths are nice again.
> 
> I confirmed that the newest patch does not change the state of the paths on
> xoom.
> 
> I wonder if we still need to block this until somebody starts to work on bug
> 1838?

I agree that 1838 is not related to this change -- it was a false positive due
to the fact that the roundrect GMs were being ignored for a while. I don't see
any reason to hold this up.

[bsalomon, 2013-11-20 15:50:37]

On 2013/11/20 14:31:32, JimVV wrote:
> On 2013/11/20 09:53:38, kkinnunen wrote:
> > On 2013/11/13 19:12:39, bsalomon wrote:
> > > On 2013/11/13 18:44:53, JimVV wrote:
> > > > While rebaselining I noticed that some of the hairline roundrects in
> > > > pathinterior and rrect also have gaps on the Xoom. So there may be a
> > separate
> > > > issue. I've filed bug 1838.
> > > Ok, so let's hold off on this CL until we can resolve 1838. Or we can
remove
> > > tests from the ignored-tests.txt in this patch *just for the try bot run*.
> > 
> > I rootcaused the bug 1838. By reverting the mentioned commit, hairline ovals
> and
> > paths are nice again.
> > 
> > I confirmed that the newest patch does not change the state of the paths on
> > xoom.
> > 
> > I wonder if we still need to block this until somebody starts to work on bug
> > 1838?
> 
> I agree that 1838 is not related to this change -- it was a false positive due
> to the fact that the roundrect GMs were being ignored for a while. I don't see
> any reason to hold this up.

SGTM

[Kimmo Kinnunen, 2013-11-21 07:14:04]

On 2013/11/20 15:50:37, bsalomon wrote:
> On 2013/11/20 14:31:32, JimVV wrote:
> > On 2013/11/20 09:53:38, kkinnunen wrote:
> > > I wonder if we still need to block this until somebody starts to work on
bug
> > > 1838?
> > 
> > I agree that 1838 is not related to this change -- it was a false positive
due
> > to the fact that the roundrect GMs were being ignored for a while. I don't
see
> > any reason to hold this up.
> 
> SGTM

Just to be on the same page: Brian, when you have time, would it be possible for
you to re-review the latest patch? I changed the place of the checks a bit to be
less subtle wrt where the optimization wants the execution to go.

[bsalomon, 2013-11-21 21:37:15]

On 2013/11/21 07:14:04, kkinnunen wrote:
> On 2013/11/20 15:50:37, bsalomon wrote:
> > On 2013/11/20 14:31:32, JimVV wrote:
> > > On 2013/11/20 09:53:38, kkinnunen wrote:
> > > > I wonder if we still need to block this until somebody starts to work on
> bug
> > > > 1838?
> > > 
> > > I agree that 1838 is not related to this change -- it was a false positive
> due
> > > to the fact that the roundrect GMs were being ignored for a while. I don't
> see
> > > any reason to hold this up.
> > 
> > SGTM
> 
> Just to be on the same page: Brian, when you have time, would it be possible
for
> you to re-review the latest patch? I changed the place of the checks a bit to
be
> less subtle wrt where the optimization wants the execution to go.

Kimmo, I like the new patch better than the earlier one. Thanks.

[bsalomon, 2013-11-21 21:37:24]

On 2013/11/21 21:37:15, bsalomon wrote:
> On 2013/11/21 07:14:04, kkinnunen wrote:
> > On 2013/11/20 15:50:37, bsalomon wrote:
> > > On 2013/11/20 14:31:32, JimVV wrote:
> > > > On 2013/11/20 09:53:38, kkinnunen wrote:
> > > > > I wonder if we still need to block this until somebody starts to work
on
> > bug
> > > > > 1838?
> > > > 
> > > > I agree that 1838 is not related to this change -- it was a false
positive
> > due
> > > > to the fact that the roundrect GMs were being ignored for a while. I
don't
> > see
> > > > any reason to hold this up.
> > > 
> > > SGTM
> > 
> > Just to be on the same page: Brian, when you have time, would it be possible
> for
> > you to re-review the latest patch? I changed the place of the checks a bit
to
> be
> > less subtle wrt where the optimization wants the execution to go.
> 
> Kimmo, I like the new patch better than the earlier one. Thanks.

lgtm

[commit-bot: I haz the power, 2013-11-22 06:50:08]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/38573007/530001

[commit-bot: I haz the power, 2013-11-22 07:02:17]

Change committed as 12357

[robertphillips, 2013-11-22 13:44:09]

I have rebaselined the majority of the dashing3 changes in r12359 (Rebaseline
dashing3 GMs for r12357 https://codereview.chromium.org/83543002/). 

Kimmo - are these changes expected? From the title of this patch I wouldn't
expect any changes when nv path rendering isn't being used.

[Kimmo Kinnunen, 2013-11-25 06:29:29]

On 2013/11/22 13:44:09, robertphillips wrote:
> I have rebaselined the majority of the dashing3 changes in r12359 (Rebaseline
> dashing3 GMs for r12357 https://codereview.chromium.org/83543002/). 
> 
> Kimmo - are these changes expected? From the title of this patch I wouldn't
> expect any changes when nv path rendering isn't being used.

Sorry for being ambiguous. Yes, changes to dashing3 were expected for
non-nv_path_rendering codepaths. Based on my (non-expert) visual inspection, the
change was ok..