Add fallback code for TextContexts that don't support all features

Add fallback code for TextContexts that don't support all features

BUG=skia:2018
NOTRY=true

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

[jvanverth1, 2014-01-29 17:14:08]

This covers all the cases we talked about except for color fonts, which I'm not
sure how to detect at this point.

[reed1, 2014-01-29 17:18:55]

https://codereview.chromium.org/135683006/diff/1/include/gpu/GrBitmapTextCont...
File include/gpu/GrBitmapTextContext.h (right):

https://codereview.chromium.org/135683006/diff/1/include/gpu/GrBitmapTextCont...
include/gpu/GrBitmapTextContext.h:31: return
!SkDraw::ShouldDrawTextAsPaths(paint, ctm);
If we move this impl into the .cpp, can we not add SkDraw.h to the #include? I
would like to make SkDraw.h private someday.

https://codereview.chromium.org/135683006/diff/1/include/gpu/GrTextContext.h
File include/gpu/GrTextContext.h (right):

https://codereview.chromium.org/135683006/diff/1/include/gpu/GrTextContext.h#...
include/gpu/GrTextContext.h:33: static bool CanDraw(const SkPaint&, const
SkMatrix&) { return false; }
all of these static-but-appear-to-be-overrides version of CanDraw a sligtly
confusing in public headers. Why is this one (in particular) here, since it
always returns false?

[reed1, 2014-01-29 17:19:40]

somewhat separate from this particular CL -- do we have good tests/gms to
exercise distance-field drawing, and (now) to exercise the cases that force
ganesh to fall back to masks?

[bsalomon, 2014-01-29 18:01:05]

This is going somewhat meta relative to this particular CL but I'm starting to
question the value of creating a text context object for each text draw. It
seemed to make sense when the pattern was:

construct
drawGlyph
drawGlyph
...
destroy

But now it's always:
construct
drawSomeText
destroy

Would it make more sense to have a long lived text context with a drawSomeText
function where drawSomeText takes the union of the stuff currently passed to the
cons and the draw function? Probably as a struct since the number of params
would start to get large.

Then there could be a meta-text context that encapsulates a particular fallback
pattern (e.g. distance-field->bmp->draw-as-paths.

I'm thinking of something along the lines of the path renderer chain. Not saying
this is the right answer, but throwing it out there as food for though.

[jvanverth1, 2014-01-29 18:38:12]

On 2014/01/29 17:19:40, reed1 wrote:
> somewhat separate from this particular CL -- do we have good tests/gms to
> exercise distance-field drawing, and (now) to exercise the cases that force
> ganesh to fall back to masks?

There are about 70 GMs that include text in some form or another. There are 2
that use stroked text, 1 that uses blurred text, 1 that has rasterizers, and 1
that has color text. The remainder test a variety of font families and styles.
I'm not sure what other cases we would have that force back to masks.

[jvanverth1, 2014-01-29 18:40:38]

On 2014/01/29 17:18:55, reed1 wrote:
>
https://codereview.chromium.org/135683006/diff/1/include/gpu/GrBitmapTextCont...
> File include/gpu/GrBitmapTextContext.h (right):
> 
>
https://codereview.chromium.org/135683006/diff/1/include/gpu/GrBitmapTextCont...
> include/gpu/GrBitmapTextContext.h:31: return
> !SkDraw::ShouldDrawTextAsPaths(paint, ctm);
> If we move this impl into the .cpp, can we not add SkDraw.h to the #include? I
> would like to make SkDraw.h private someday.

Sure, I was allowing for the function to be inlined.

> https://codereview.chromium.org/135683006/diff/1/include/gpu/GrTextContext.h
> File include/gpu/GrTextContext.h (right):
> 
>
https://codereview.chromium.org/135683006/diff/1/include/gpu/GrTextContext.h#...
> include/gpu/GrTextContext.h:33: static bool CanDraw(const SkPaint&, const
> SkMatrix&) { return false; }
> all of these static-but-appear-to-be-overrides version of CanDraw a sligtly
> confusing in public headers. Why is this one (in particular) here, since it
> always returns false?

In retrospect it's not needed -- I can kill it. Alternatively, I can make them
non-static methods if that would be less confusing.

[jvanverth1, 2014-01-29 19:02:39]

On 2014/01/29 18:01:05, bsalomon wrote:
> This is going somewhat meta relative to this particular CL but I'm starting to
> question the value of creating a text context object for each text draw. It
> seemed to make sense when the pattern was:
> 
> construct
> drawGlyph
> drawGlyph
> ...
> destroy
> 
> But now it's always:
> construct
> drawSomeText
> destroy
> 
> Would it make more sense to have a long lived text context with a drawSomeText
> function where drawSomeText takes the union of the stuff currently passed to
the
> cons and the draw function? Probably as a struct since the number of params
> would start to get large.
> 
> Then there could be a meta-text context that encapsulates a particular
fallback
> pattern (e.g. distance-field->bmp->draw-as-paths.
> 
> I'm thinking of something along the lines of the path renderer chain. Not
saying
> this is the right answer, but throwing it out there as food for though.

I guess I see what you're saying. The construct and destroy are only conceptual,
as it repeatedly calls placement new on an allocation, so it just as well could
be part of the function call. My only issue is that it makes the drawText
function parameter list rather long, but the struct could encapsulate that.

I don't think there's a need for a meta-text context -- the pattern is fairly
simple at the moment, so SkGpuDevice can handle it as I have it here
(specialized->bitmap->paths). SkGpuDevice could have two TextContexts: the
specialized one (distance or NVPR) and Bitmap.

[jvanverth1, 2014-01-29 19:25:58]

https://codereview.chromium.org/135683006/diff/1/include/gpu/GrBitmapTextCont...
File include/gpu/GrBitmapTextContext.h (right):

https://codereview.chromium.org/135683006/diff/1/include/gpu/GrBitmapTextCont...
include/gpu/GrBitmapTextContext.h:31: return
!SkDraw::ShouldDrawTextAsPaths(paint, ctm);
On 2014/01/29 17:18:56, reed1 wrote:
> If we move this impl into the .cpp, can we not add SkDraw.h to the #include? I
> would like to make SkDraw.h private someday.

Done.

https://codereview.chromium.org/135683006/diff/1/include/gpu/GrTextContext.h
File include/gpu/GrTextContext.h (right):

https://codereview.chromium.org/135683006/diff/1/include/gpu/GrTextContext.h#...
include/gpu/GrTextContext.h:33: static bool CanDraw(const SkPaint&, const
SkMatrix&) { return false; }
On 2014/01/29 17:18:56, reed1 wrote:
> all of these static-but-appear-to-be-overrides version of CanDraw a sligtly
> confusing in public headers. Why is this one (in particular) here, since it
> always returns false?

I've removed this one. I left the others as static for now.

[reed1, 2014-01-29 19:37:57]

thanks.

btw -- why are the first two include/gpu files in the review public?

[bsalomon, 2014-01-29 19:38:53]

On 2014/01/29 19:02:39, JimVV wrote:
> I don't think there's a need for a meta-text context -- the pattern is fairly
> simple at the moment, so SkGpuDevice can handle it as I have it here
> (specialized->bitmap->paths). SkGpuDevice could have two TextContexts: the
> specialized one (distance or NVPR) and Bitmap.

I guess I wonder whether baking that logic into SkGpuDevice will be still be
desirable when we have color bitmap font support and NVPR or DF. Maybe it's just
a hard-coded chain where the first context type is specializable. 

Anyway, lgtm as it is now.

https://codereview.chromium.org/135683006/diff/20001/src/gpu/GrDistanceFieldT...
File src/gpu/GrDistanceFieldTextContext.cpp (right):

https://codereview.chromium.org/135683006/diff/20001/src/gpu/GrDistanceFieldT...
src/gpu/GrDistanceFieldTextContext.cpp:32: && paint.getStyle() ==
SkPaint::kFill_Style
style nit: &&s on prev line.

[bsalomon, 2014-01-29 19:39:54]

On 2014/01/29 19:37:57, reed1 wrote:
> thanks.
> 
> btw -- why are the first two include/gpu files in the review public?


I think the text context is public for historical reasons. It was part of the
Sk/Gr glue layer. It should be made private.

[reed1, 2014-01-29 19:40:32]

On 2014/01/29 19:39:54, bsalomon wrote:
> On 2014/01/29 19:37:57, reed1 wrote:
> > thanks.
> > 
> > btw -- why are the first two include/gpu files in the review public?
> 
> 
> I think the text context is public for historical reasons. It was part of the
> Sk/Gr glue layer. It should be made private.

destroy all monsters == make all interfaces private

[jvanverth1, 2014-01-29 20:29:18]

On 2014/01/29 19:40:32, reed1 wrote:
> On 2014/01/29 19:39:54, bsalomon wrote:
> > On 2014/01/29 19:37:57, reed1 wrote:
> > > thanks.
> > > 
> > > btw -- why are the first two include/gpu files in the review public?
> > 
> > 
> > I think the text context is public for historical reasons. It was part of
the
> > Sk/Gr glue layer. It should be made private.
> 
> destroy all monsters == make all interfaces private

I'll create a new change to put all the Gr*TextContext.h files in src/gpu.

[jvanverth1, 2014-01-29 20:29:24]

On 2014/01/29 19:40:32, reed1 wrote:
> On 2014/01/29 19:39:54, bsalomon wrote:
> > On 2014/01/29 19:37:57, reed1 wrote:
> > > thanks.
> > > 
> > > btw -- why are the first two include/gpu files in the review public?
> > 
> > 
> > I think the text context is public for historical reasons. It was part of
the
> > Sk/Gr glue layer. It should be made private.
> 
> destroy all monsters == make all interfaces private

I'll create a new change to put all the Gr*TextContext.h files in src/gpu.

[commit-bot: I haz the power, 2014-01-29 20:38:19]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jvanverth@google.com/135683006/40001

[commit-bot: I haz the power, 2014-01-29 23:06:01]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jvanverth@google.com/135683006/40001

[commit-bot: I haz the power, 2014-01-29 23:16:00]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jvanverth@google.com/135683006/40001

[commit-bot: I haz the power, 2014-01-29 23:18:06]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jvanverth@google.com/135683006/40001

[commit-bot: I haz the power, 2014-01-30 14:40:47]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 14:41:08]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jvanverth@google.com/135683006/40001

[commit-bot: I haz the power, 2014-01-30 15:03:16]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 15:05:59]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jvanverth@google.com/135683006/40001

[commit-bot: I haz the power, 2014-01-30 15:11:14]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 15:16:39]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jvanverth@google.com/135683006/40001

[commit-bot: I haz the power, 2014-01-30 15:19:01]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 15:21:41]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jvanverth@google.com/135683006/40001

[commit-bot: I haz the power, 2014-01-30 15:22:06]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 15:22:44]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jvanverth@google.com/135683006/40001

[commit-bot: I haz the power, 2014-01-30 15:22:57]

Change committed as 13236

[commit-bot: I haz the power, 2014-01-30 15:23:26]

CQ bit was unchecked on CL. Ignoring.