Split GrTextContext into baseclass and subclass

Split GrTextContext into baseclass and subclass

* Turn GrTextContext into interface. Make the drawPackedGlyph method pure virtual.
* Introduce GrBatchedTextContext - a base class for TextContexts that can defere multiple glyphs into single draw.
* Introduce GrDefaultTextContext - a TextContext that behaves exactly as the old GrTextContext.
* Add a factory method to SkGpuDevice that creates new GrTextContext.
* Add a method to SkGpuDevice that destroys GrTextContext previously created by the factory.

This will enable integration with GLyphy - a GPU-accelerated text rendering
library. It should also make it easy to introduce other GPU-accelerated text
rendering algorithms.

TEST=out/Debug/gm; out/Debug/tests

[bsalomon, 2012-03-13 13:04:00]

https://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1759: void SkGpuDevice::setupTextContextMemPool() {
This seems like a lot of trouble. We've made it necessary to call
releaseTextContext() after we use the text context. It seems like this would be
more straight-forward:

1. GrTextContext is refcounted (inherits from GrRefCnt)
2. GrTextContext has a virtual flush() function which must be called after
drawing text.
3. SkGpuDevice has GrTextContext* fTextContext, initalized to new
GrDefaultTextContext(), settable by SkGpuDevice::setTextContext(GrTextContext*
textContext)

If it feels cleaner we could have a stack helper for flushing.

class GrAutoTextContext : public GrNoncopyable {
public:
    GrAutoFlushTextContext(GrTextContext* textContext) {
        GrSafeRef(textContext);
        fTextContext = textContext;
    }
    void set(GrTextContext* textContext) {
         if (NULL != fTextContext)
             fTextContext->flush();
             fTextContext->unref();
         }
         fTextContext = textContext;
    }
    ~GrAutoFlushTextContext() {
         if (NULL != fTextContext)
             fTextContext->fTextContext();
             fTextContext->unref();
         }
    }
    GrTextContext* get() { return fTextContext; }
private:
    GrTextContext* fTextContext;
};

void SkGpuDevice::getTextContext(GrAutoTextContext* txtCtx) {
     txtCtx->set(fTextContext);
}

[baranowski, 2012-03-13 14:22:26]

https://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1759: void SkGpuDevice::setupTextContextMemPool() {
On 2012/03/13 13:04:01, bsalomon wrote:
> (...)
> 3. SkGpuDevice has GrTextContext* fTextContext, initalized to new
> GrDefaultTextContext(), settable by SkGpuDevice::setTextContext(GrTextContext*
> textContext)
> (...)

GrDefaultTextContext doesn't have default constructor. It's parameters are quite
big so it seems we have to create new TextContext with each call to
getTextContext(...).

Alternatively, as I mentioned in the other issue, we could invalidate
TextContext object at the end of drawText call. The next call to getTextContext
would re-initialize it. We would need to complicate a bit TextContext semantics:
add methods init(...) and invalidate(); constructor and destructor would be
either empty or they would call these methods. Still, we would have to call
invalidate() at the end of drawText() which might not be cleaner than
releaseTextContext()

[bsalomon, 2012-03-13 14:37:53]

https://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1759: void SkGpuDevice::setupTextContextMemPool() {
On 2012/03/13 14:22:26, baranowski wrote:
> On 2012/03/13 13:04:01, bsalomon wrote:
> > (...)
> > 3. SkGpuDevice has GrTextContext* fTextContext, initalized to new
> > GrDefaultTextContext(), settable by
SkGpuDevice::setTextContext(GrTextContext*
> > textContext)
> > (...)
> 
> GrDefaultTextContext doesn't have default constructor. It's parameters are
quite
> big so it seems we have to create new TextContext with each call to
> getTextContext(...).
> 
> Alternatively, as I mentioned in the other issue, we could invalidate
> TextContext object at the end of drawText call. The next call to
getTextContext
> would re-initialize it. We would need to complicate a bit TextContext
semantics:
> add methods init(...) and invalidate(); constructor and destructor would be
> either empty or they would call these methods. Still, we would have to call
> invalidate() at the end of drawText() which might not be cleaner than
> releaseTextContext()

How about init(GrContext*, const GrPaint&, const GrMatrix*) and flush()?

Then the stack helper would call init() in set / const and flush in the ~.

[baranowski, 2012-03-13 15:27:27]

https://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1759: void SkGpuDevice::setupTextContextMemPool() {
On 2012/03/13 14:37:54, bsalomon wrote:
> On 2012/03/13 14:22:26, baranowski wrote:
> > On 2012/03/13 13:04:01, bsalomon wrote:
> > > (...)
> > > 3. SkGpuDevice has GrTextContext* fTextContext, initalized to new
> > > GrDefaultTextContext(), settable by
> SkGpuDevice::setTextContext(GrTextContext*
> > > textContext)
> > > (...)
> > 
> > GrDefaultTextContext doesn't have default constructor. It's parameters are
> quite
> > big so it seems we have to create new TextContext with each call to
> > getTextContext(...).
> > 
> > Alternatively, as I mentioned in the other issue, we could invalidate
> > TextContext object at the end of drawText call. The next call to
> getTextContext
> > would re-initialize it. We would need to complicate a bit TextContext
> semantics:
> > add methods init(...) and invalidate(); constructor and destructor would be
> > either empty or they would call these methods. Still, we would have to call
> > invalidate() at the end of drawText() which might not be cleaner than
> > releaseTextContext()
> 
> How about init(GrContext*, const GrPaint&, const GrMatrix*) and flush()?
> 
> Then the stack helper would call init() in set / const and flush in the ~.

I believe we would still need invalidate() to avoid dangling pointers. If
dangling pointers are ok, we could simply defer TextContext deletion until the
next call to getTextContext(...).

GrTextContext::init would need more parameters to satisfy all subclasses. For
now that means GrContext* plus all parameters of getTextContext(). If we decide
to add another subclass that requires new parameters, we would have to adjust
signatures in all the other subclasses.

[baranowski, 2012-03-14 16:11:45]

http://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1759: void SkGpuDevice::setupTextContextMemPool() {
On 2012/03/13 15:27:27, baranowski wrote:
> On 2012/03/13 14:37:54, bsalomon wrote:
> > On 2012/03/13 14:22:26, baranowski wrote:
> > > On 2012/03/13 13:04:01, bsalomon wrote:
> > > > (...)
> > > > 3. SkGpuDevice has GrTextContext* fTextContext, initalized to new
> > > > GrDefaultTextContext(), settable by
> > SkGpuDevice::setTextContext(GrTextContext*
> > > > textContext)
> > > > (...)
> > > 
> > > GrDefaultTextContext doesn't have default constructor. It's parameters are
> > quite
> > > big so it seems we have to create new TextContext with each call to
> > > getTextContext(...).
> > > 
> > > Alternatively, as I mentioned in the other issue, we could invalidate
> > > TextContext object at the end of drawText call. The next call to
> > getTextContext
> > > would re-initialize it. We would need to complicate a bit TextContext
> > semantics:
> > > add methods init(...) and invalidate(); constructor and destructor would
be
> > > either empty or they would call these methods. Still, we would have to
call
> > > invalidate() at the end of drawText() which might not be cleaner than
> > > releaseTextContext()
> > 
> > How about init(GrContext*, const GrPaint&, const GrMatrix*) and flush()?
> > 
> > Then the stack helper would call init() in set / const and flush in the ~.
> 
> I believe we would still need invalidate() to avoid dangling pointers. If
> dangling pointers are ok, we could simply defer TextContext deletion until the
> next call to getTextContext(...).
> 
> GrTextContext::init would need more parameters to satisfy all subclasses. For
> now that means GrContext* plus all parameters of getTextContext(). If we
decide
> to add another subclass that requires new parameters, we would have to adjust
> signatures in all the other subclasses.

I should have asked explicitly: Do you think that init()/invalidate() handled by
stack helper would be better than the way it is now?

If so, how about different semantics of the stack helper? For me it seems
slightly cleaner:
class GrAutoFlushTextContext : public GrNoncopyable {
public:
    GrAutoFlushTextContext(SkGpuDevice* device, GrContext* context,
                           const SkPaint& skPaint, const GrPaint& grPaint,
                           const GrMatrix* extMatrix,
                           const GrMatrix* mvMatrix) {
        fTextContext = device->getTextContext();
        GrAssert(NULL != fTextContext);
        GrSafeRef(textContext);
        fTextContext->init(context, skPaint, grPaint, extMatrix, mvMatrix);
    }
    ~GrAutoFlushTextContext() {
         fTextContext->invalidate();
         fTextContext->unref();
    }
    GrTextContext* get() { return fTextContext; }
private:
    GrTextContext* fTextContext;
};

[bsalomon, 2012-03-14 17:38:26]

On 2012/03/14 16:11:45, baranowski wrote:
> http://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
http://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp#newco...
> src/gpu/SkGpuDevice.cpp:1759: void SkGpuDevice::setupTextContextMemPool() {
> On 2012/03/13 15:27:27, baranowski wrote:
> > On 2012/03/13 14:37:54, bsalomon wrote:
> > > On 2012/03/13 14:22:26, baranowski wrote:
> > > > On 2012/03/13 13:04:01, bsalomon wrote:
> > > > > (...)
> > > > > 3. SkGpuDevice has GrTextContext* fTextContext, initalized to new
> > > > > GrDefaultTextContext(), settable by
> > > SkGpuDevice::setTextContext(GrTextContext*
> > > > > textContext)
> > > > > (...)
> > > > 
> > > > GrDefaultTextContext doesn't have default constructor. It's parameters
are
> > > quite
> > > > big so it seems we have to create new TextContext with each call to
> > > > getTextContext(...).
> > > > 
> > > > Alternatively, as I mentioned in the other issue, we could invalidate
> > > > TextContext object at the end of drawText call. The next call to
> > > getTextContext
> > > > would re-initialize it. We would need to complicate a bit TextContext
> > > semantics:
> > > > add methods init(...) and invalidate(); constructor and destructor would
> be
> > > > either empty or they would call these methods. Still, we would have to
> call
> > > > invalidate() at the end of drawText() which might not be cleaner than
> > > > releaseTextContext()
> > > 
> > > How about init(GrContext*, const GrPaint&, const GrMatrix*) and flush()?
> > > 
> > > Then the stack helper would call init() in set / const and flush in the ~.
> > 
> > I believe we would still need invalidate() to avoid dangling pointers. If
> > dangling pointers are ok, we could simply defer TextContext deletion until
the
> > next call to getTextContext(...).
> > 
> > GrTextContext::init would need more parameters to satisfy all subclasses.
For
> > now that means GrContext* plus all parameters of getTextContext(). If we
> decide
> > to add another subclass that requires new parameters, we would have to
adjust
> > signatures in all the other subclasses.
> 
> I should have asked explicitly: Do you think that init()/invalidate() handled
by
> stack helper would be better than the way it is now?
> 
> If so, how about different semantics of the stack helper? For me it seems
> slightly cleaner:
> class GrAutoFlushTextContext : public GrNoncopyable {
> public:
>     GrAutoFlushTextContext(SkGpuDevice* device, GrContext* context,
>                            const SkPaint& skPaint, const GrPaint& grPaint,
>                            const GrMatrix* extMatrix,
>                            const GrMatrix* mvMatrix) {
>         fTextContext = device->getTextContext();
>         GrAssert(NULL != fTextContext);
>         GrSafeRef(textContext);
>         fTextContext->init(context, skPaint, grPaint, extMatrix, mvMatrix);
>     }
>     ~GrAutoFlushTextContext() {
>          fTextContext->invalidate();
>          fTextContext->unref();
>     }
>     GrTextContext* get() { return fTextContext; }
> private:
>     GrTextContext* fTextContext;
> };

Sorry I thought I had replied to you last message but I must have left a
dangling tab or something.

I like passing the init options to the stack helper. Maybe the stack helper
should be a nested class of GrTextContext and init could be protected?

Can we call invalidate finish or flush?

Why do we need the mvMatrix param? The matrix should already be set on
GrContext.

Also, can the glyphy subclass create its own SkPaint internally?

[baranowski, 2012-03-14 18:25:36]

On 2012/03/14 17:38:26, bsalomon wrote:
> On 2012/03/14 16:11:45, baranowski wrote:
> > http://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp
> > File src/gpu/SkGpuDevice.cpp (right):
> > 
> >
>
http://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp#newco...
> > src/gpu/SkGpuDevice.cpp:1759: void SkGpuDevice::setupTextContextMemPool() {
> > On 2012/03/13 15:27:27, baranowski wrote:
> > > On 2012/03/13 14:37:54, bsalomon wrote:
> > > > On 2012/03/13 14:22:26, baranowski wrote:
> > > > > On 2012/03/13 13:04:01, bsalomon wrote:
> > > > > > (...)
> > > > > > 3. SkGpuDevice has GrTextContext* fTextContext, initalized to new
> > > > > > GrDefaultTextContext(), settable by
> > > > SkGpuDevice::setTextContext(GrTextContext*
> > > > > > textContext)
> > > > > > (...)
> > > > > 
> > > > > GrDefaultTextContext doesn't have default constructor. It's parameters
> are
> > > > quite
> > > > > big so it seems we have to create new TextContext with each call to
> > > > > getTextContext(...).
> > > > > 
> > > > > Alternatively, as I mentioned in the other issue, we could invalidate
> > > > > TextContext object at the end of drawText call. The next call to
> > > > getTextContext
> > > > > would re-initialize it. We would need to complicate a bit TextContext
> > > > semantics:
> > > > > add methods init(...) and invalidate(); constructor and destructor
would
> > be
> > > > > either empty or they would call these methods. Still, we would have to
> > call
> > > > > invalidate() at the end of drawText() which might not be cleaner than
> > > > > releaseTextContext()
> > > > 
> > > > How about init(GrContext*, const GrPaint&, const GrMatrix*) and flush()?
> > > > 
> > > > Then the stack helper would call init() in set / const and flush in the
~.
> > > 
> > > I believe we would still need invalidate() to avoid dangling pointers. If
> > > dangling pointers are ok, we could simply defer TextContext deletion until
> the
> > > next call to getTextContext(...).
> > > 
> > > GrTextContext::init would need more parameters to satisfy all subclasses.
> For
> > > now that means GrContext* plus all parameters of getTextContext(). If we
> > decide
> > > to add another subclass that requires new parameters, we would have to
> adjust
> > > signatures in all the other subclasses.
> > 
> > I should have asked explicitly: Do you think that init()/invalidate()
handled
> by
> > stack helper would be better than the way it is now?
> > 
> > If so, how about different semantics of the stack helper? For me it seems
> > slightly cleaner:
> > class GrAutoFlushTextContext : public GrNoncopyable {
> > public:
> >     GrAutoFlushTextContext(SkGpuDevice* device, GrContext* context,
> >                            const SkPaint& skPaint, const GrPaint& grPaint,
> >                            const GrMatrix* extMatrix,
> >                            const GrMatrix* mvMatrix) {
> >         fTextContext = device->getTextContext();
> >         GrAssert(NULL != fTextContext);
> >         GrSafeRef(textContext);
> >         fTextContext->init(context, skPaint, grPaint, extMatrix, mvMatrix);
> >     }
> >     ~GrAutoFlushTextContext() {
> >          fTextContext->invalidate();
> >          fTextContext->unref();
> >     }
> >     GrTextContext* get() { return fTextContext; }
> > private:
> >     GrTextContext* fTextContext;
> > };
> 
> Sorry I thought I had replied to you last message but I must have left a
> dangling tab or something.
> 
> I like passing the init options to the stack helper. Maybe the stack helper
> should be a nested class of GrTextContext and init could be protected?

Sounds good.

> Can we call invalidate finish or flush?

flush does something different but finish IMO is a better name than invalidate

> Why do we need the mvMatrix param? The matrix should already be set on
> GrContext.

mvMatrix is the mattrix applied to glyph when obtaining the mask or outline.
With GLyphy, istead of applying it to the glyph, we want to apply it to quad
vertices.

> Also, can the glyphy subclass create its own SkPaint internally?

It could. But then we need to pass all the SkPaint attributes that are relevant
to computing glyph outline. I'm not sure if these are limited to just fTypeface
and fTextSize.

Even if we do that, finish()/invalidate() will be still needed to reset
GrTextStrike* fStrike in both subclasses. Making GrTextStrike ref-counted would
require some GrFontCache refactoring (probably making it listen to GrTextStrike
deletion).

[bsalomon, 2012-03-14 19:02:33]

On 2012/03/14 18:25:36, baranowski wrote:
> On 2012/03/14 17:38:26, bsalomon wrote:
> > On 2012/03/14 16:11:45, baranowski wrote:
> > > http://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp
> > > File src/gpu/SkGpuDevice.cpp (right):
> > > 
> > >
> >
>
http://codereview.appspot.com/5796071/diff/2001/src/gpu/SkGpuDevice.cpp#newco...
> > > src/gpu/SkGpuDevice.cpp:1759: void SkGpuDevice::setupTextContextMemPool()
{
> > > On 2012/03/13 15:27:27, baranowski wrote:
> > > > On 2012/03/13 14:37:54, bsalomon wrote:
> > > > > On 2012/03/13 14:22:26, baranowski wrote:
> > > > > > On 2012/03/13 13:04:01, bsalomon wrote:
> > > > > > > (...)
> > > > > > > 3. SkGpuDevice has GrTextContext* fTextContext, initalized to new
> > > > > > > GrDefaultTextContext(), settable by
> > > > > SkGpuDevice::setTextContext(GrTextContext*
> > > > > > > textContext)
> > > > > > > (...)
> > > > > > 
> > > > > > GrDefaultTextContext doesn't have default constructor. It's
parameters
> > are
> > > > > quite
> > > > > > big so it seems we have to create new TextContext with each call to
> > > > > > getTextContext(...).
> > > > > > 
> > > > > > Alternatively, as I mentioned in the other issue, we could
invalidate
> > > > > > TextContext object at the end of drawText call. The next call to
> > > > > getTextContext
> > > > > > would re-initialize it. We would need to complicate a bit
TextContext
> > > > > semantics:
> > > > > > add methods init(...) and invalidate(); constructor and destructor
> would
> > > be
> > > > > > either empty or they would call these methods. Still, we would have
to
> > > call
> > > > > > invalidate() at the end of drawText() which might not be cleaner
than
> > > > > > releaseTextContext()
> > > > > 
> > > > > How about init(GrContext*, const GrPaint&, const GrMatrix*) and
flush()?
> > > > > 
> > > > > Then the stack helper would call init() in set / const and flush in
the
> ~.
> > > > 
> > > > I believe we would still need invalidate() to avoid dangling pointers.
If
> > > > dangling pointers are ok, we could simply defer TextContext deletion
until
> > the
> > > > next call to getTextContext(...).
> > > > 
> > > > GrTextContext::init would need more parameters to satisfy all
subclasses.
> > For
> > > > now that means GrContext* plus all parameters of getTextContext(). If we
> > > decide
> > > > to add another subclass that requires new parameters, we would have to
> > adjust
> > > > signatures in all the other subclasses.
> > > 
> > > I should have asked explicitly: Do you think that init()/invalidate()
> handled
> > by
> > > stack helper would be better than the way it is now?
> > > 
> > > If so, how about different semantics of the stack helper? For me it seems
> > > slightly cleaner:
> > > class GrAutoFlushTextContext : public GrNoncopyable {
> > > public:
> > >     GrAutoFlushTextContext(SkGpuDevice* device, GrContext* context,
> > >                            const SkPaint& skPaint, const GrPaint& grPaint,
> > >                            const GrMatrix* extMatrix,
> > >                            const GrMatrix* mvMatrix) {
> > >         fTextContext = device->getTextContext();
> > >         GrAssert(NULL != fTextContext);
> > >         GrSafeRef(textContext);
> > >         fTextContext->init(context, skPaint, grPaint, extMatrix,
mvMatrix);
> > >     }
> > >     ~GrAutoFlushTextContext() {
> > >          fTextContext->invalidate();
> > >          fTextContext->unref();
> > >     }
> > >     GrTextContext* get() { return fTextContext; }
> > > private:
> > >     GrTextContext* fTextContext;
> > > };
> > 
> > Sorry I thought I had replied to you last message but I must have left a
> > dangling tab or something.
> > 
> > I like passing the init options to the stack helper. Maybe the stack helper
> > should be a nested class of GrTextContext and init could be protected?
> 
> Sounds good.
> 
> > Can we call invalidate finish or flush?
> 
> flush does something different but finish IMO is a better name than invalidate
> 
> > Why do we need the mvMatrix param? The matrix should already be set on
> > GrContext.
> 
> mvMatrix is the mattrix applied to glyph when obtaining the mask or outline.
> With GLyphy, istead of applying it to the glyph, we want to apply it to quad
> vertices.

What determines mvMatrix?

> 
> > Also, can the glyphy subclass create its own SkPaint internally?
> 
> It could. But then we need to pass all the SkPaint attributes that are
relevant
> to computing glyph outline. I'm not sure if these are limited to just
fTypeface
> and fTextSize.
> 
> Even if we do that, finish()/invalidate() will be still needed to reset
> GrTextStrike* fStrike in both subclasses. Making GrTextStrike ref-counted
would
> require some GrFontCache refactoring (probably making it listen to
GrTextStrike
> deletion).

I realize we'll still need finish() since it will have to flush for batched
subclasses. I just want to reduce the params required to init the txt ctx. Are
the typeface and text size obtainable by the params seen by drawPackedGlyph?

[baranowski, 2012-03-14 19:16:15]

On 2012/03/14 19:02:33, bsalomon wrote:
> On 2012/03/14 18:25:36, baranowski wrote:
> > mvMatrix is the mattrix applied to glyph when obtaining the mask or outline.
> > With GLyphy, istead of applying it to the glyph, we want to apply it to quad
> > vertices.
> 
> What determines mvMatrix?

It is copied from fMVMatrix in the list SkCanvas::fMCRec.fTopLayer. I couldn't
understand what those lists do and how they are handled.

> > > Also, can the glyphy subclass create its own SkPaint internally?
> > 
> > It could. But then we need to pass all the SkPaint attributes that are
> relevant
> > to computing glyph outline. I'm not sure if these are limited to just
> fTypeface
> > and fTextSize.
> > 
> > Even if we do that, finish()/invalidate() will be still needed to reset
> > GrTextStrike* fStrike in both subclasses. Making GrTextStrike ref-counted
> would
> > require some GrFontCache refactoring (probably making it listen to
> GrTextStrike
> > deletion).
> 
> I realize we'll still need finish() since it will have to flush for batched
> subclasses. I just want to reduce the params required to init the txt ctx. Are
> the typeface and text size obtainable by the params seen by drawPackedGlyph?

I believe they are not.

[baranowski, 2012-03-14 20:18:03]

Uploaded new patch set with init()/finish() methods. I decided to make the stack
helper nested class of SkGpuDevice, since it also needs access to the private
SkGpuDevice::getTextContext().

[bsalomon, 2012-03-14 20:36:19]

On 2012/03/14 19:16:15, baranowski wrote:
> On 2012/03/14 19:02:33, bsalomon wrote:
> > On 2012/03/14 18:25:36, baranowski wrote:
> > > mvMatrix is the mattrix applied to glyph when obtaining the mask or
outline.
> > > With GLyphy, istead of applying it to the glyph, we want to apply it to
quad
> > > vertices.
> > 
> > What determines mvMatrix?
> 
> It is copied from fMVMatrix in the list SkCanvas::fMCRec.fTopLayer. I couldn't
> understand what those lists do and how they are handled.

That should be the same as GrContext::getMatrix(), so I don't think it needs to
be a separate param.

> 
> > > > Also, can the glyphy subclass create its own SkPaint internally?
> > > 
> > > It could. But then we need to pass all the SkPaint attributes that are
> > relevant
> > > to computing glyph outline. I'm not sure if these are limited to just
> > fTypeface
> > > and fTextSize.
> > > 
> > > Even if we do that, finish()/invalidate() will be still needed to reset
> > > GrTextStrike* fStrike in both subclasses. Making GrTextStrike ref-counted
> > would
> > > require some GrFontCache refactoring (probably making it listen to
> > GrTextStrike
> > > deletion).
> > 
> > I realize we'll still need finish() since it will have to flush for batched
> > subclasses. I just want to reduce the params required to init the txt ctx.
Are
> > the typeface and text size obtainable by the params seen by drawPackedGlyph?
> 
> I believe they are not.

The default text context is able to get the outline using the GrFontScaler
passed in to drawPackedGlyph. It boils down to calling
SkScalerContext::getPath(), same as SkPaint::getTextPath. Can this be made to
work for the GLyphy subclass as well?

[bsalomon, 2012-03-14 20:41:54]

On 2012/03/14 20:18:03, baranowski wrote:
> Uploaded new patch set with init()/finish() methods. I decided to make the
stack
> helper nested class of SkGpuDevice, since it also needs access to the private
> SkGpuDevice::getTextContext().

If the callsite is in SkGpuDevice why does it need access to a private getter?
Is it just to lazily init fTextContext? That doesn't seem like a necessity.
Creating a SkGpuDevice is not a high-frequency operation. If we create a default
text context in the device cons only to toss it with a setTextContext I wouldn't
sweat over it. Even if that were necessary can't we pass the result of
SkGpuDevice::getTextContext() to the helper's constructor?

[baranowski, 2012-03-14 21:14:02]

On 2012/03/14 20:41:54, bsalomon wrote:
> If the callsite is in SkGpuDevice why does it need access to a private getter?
> Is it just to lazily init fTextContext? That doesn't seem like a necessity.
> Creating a SkGpuDevice is not a high-frequency operation. If we create a
default
> text context in the device cons only to toss it with a setTextContext I
wouldn't
> sweat over it.

In order to choose the right TextContext subclass, we need to know SkGpuDevice
state. Putting there a private factory that gives us TextContext seems most
natural.

> Even if that were necessary can't we pass the result of
> SkGpuDevice::getTextContext() to the helper's constructor?

Yes, I can do that.

On 2012/03/14 20:36:19, bsalomon wrote:
> The default text context is able to get the outline using the GrFontScaler
> passed in to drawPackedGlyph. It boils down to calling
> SkScalerContext::getPath(), same as SkPaint::getTextPath. Can this be made to
> work for the GLyphy subclass as well?

I'm not aware of any way to make it work. For GLyphy we need outline with a
custom font size and custom transformation matrix. We cannot modify these
parameters of SkScalerContext. AFAIK the transformation matrix is encoded in
platform-specific SkScalerContext subclasses.

> On 2012/03/14 19:16:15, baranowski wrote:
> > On 2012/03/14 19:02:33, bsalomon wrote:
> > > 
> > > What determines mvMatrix?
> > 
> > It is copied from fMVMatrix in the list SkCanvas::fMCRec.fTopLayer. I
couldn't
> > understand what those lists do and how they are handled.

> That should be the same as GrContext::getMatrix(), so I don't think 
> it needs to
> be a separate param.

I'm afraid it might be much more complicated, but I'll remove that argument and
add it later if needed.

[baranowski, 2012-03-14 22:12:43]

Moved helper to GrTextStrike.
Removed mvMatrix argument.
Made init(...) and finish() protected.

[bsalomon, 2012-03-15 15:14:31]

I talked to Mike about SkPaint and getting the outline. He said that
GrFontScaler could provide a point size which would enable the GLyphy subclass
to normalize the outline size. I'd definitely prefer that over pushing a SkPaint
down to the text context. Let's remove SkPaint from the interface and then work
with Mike to get the necessary API on GrFontScaler to enable GLyphy.

http://codereview.appspot.com/5796071/diff/8002/include/gpu/GrTextContext.h
File include/gpu/GrTextContext.h (right):

http://codereview.appspot.com/5796071/diff/8002/include/gpu/GrTextContext.h#n...
include/gpu/GrTextContext.h:32: AutoFinish(GrTextContext* textContext,
GrContext* context,
Should we inline this class's impl to avoid function call overhead?

http://codereview.appspot.com/5796071/diff/8002/include/gpu/GrTextContext.h#n...
include/gpu/GrTextContext.h:88: friend class AutoFinish;
I don't think we need this. As far as I know we're not building on compilers
that haven't been updated to give automatic access to nested classes and we
assume this in other places.

http://codereview.appspot.com/5796071/diff/8002/src/gpu/GrDefaultTextContext.cpp
File src/gpu/GrDefaultTextContext.cpp (right):

http://codereview.appspot.com/5796071/diff/8002/src/gpu/GrDefaultTextContext....
src/gpu/GrDefaultTextContext.cpp:159: this->flush();
Would it be better for the batched base class to do this so that each subclass
doesn't have to remember:

this->INHERITED::finish();
fStrike = NULL;
fContext->setMatrix(...);

http://codereview.appspot.com/5796071/diff/8002/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/5796071/diff/8002/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1606: paint, grPaint, draw.fMVMatrix);
fExtMatrix?

http://codereview.appspot.com/5796071/diff/8002/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1636: paint, grPaint, draw.fMVMatrix);
ditto

[baranowski, 2012-03-15 15:57:11]

Removed SkPaint argument.

http://codereview.appspot.com/5796071/diff/8002/include/gpu/GrTextContext.h
File include/gpu/GrTextContext.h (right):

http://codereview.appspot.com/5796071/diff/8002/include/gpu/GrTextContext.h#n...
include/gpu/GrTextContext.h:32: AutoFinish(GrTextContext* textContext,
GrContext* context,
On 2012/03/15 15:14:31, bsalomon wrote:
> Should we inline this class's impl to avoid function call overhead?

Done in new patch set.

http://codereview.appspot.com/5796071/diff/8002/include/gpu/GrTextContext.h#n...
include/gpu/GrTextContext.h:88: friend class AutoFinish;
On 2012/03/15 15:14:31, bsalomon wrote:
> I don't think we need this. As far as I know we're not building on compilers
> that haven't been updated to give automatic access to nested classes and we
> assume this in other places.

Done in new patch set.

http://codereview.appspot.com/5796071/diff/8002/src/gpu/GrDefaultTextContext.cpp
File src/gpu/GrDefaultTextContext.cpp (right):

http://codereview.appspot.com/5796071/diff/8002/src/gpu/GrDefaultTextContext....
src/gpu/GrDefaultTextContext.cpp:159: this->flush();
On 2012/03/15 15:14:31, bsalomon wrote:
> Would it be better for the batched base class to do this so that each subclass
> doesn't have to remember:
> 
> this->INHERITED::finish();
> fStrike = NULL;
> fContext->setMatrix(...);

I think the cleanest and most maintainable way to do init/finish is to think of
them as of pseudo-constructor/destructor. So the first thing subclass::init()
does is calling INHERITED::init(). And the last thing subclass::finish() does is
calling INHERITED::finish().

And even if we call finish() earlier, calling overridden method from
baseclass::finish() might be error-prone, since it's so different from
constructor/destructor semantics.

http://codereview.appspot.com/5796071/diff/8002/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/5796071/diff/8002/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1606: paint, grPaint, draw.fMVMatrix);
On 2012/03/15 15:14:31, bsalomon wrote:
> fExtMatrix?

Thanks, good catch.

[bsalomon, 2012-03-15 17:33:56]

LGTM. OK if I commit tomorrow morning? This will require a change to chrome's
skia.gyp. I can commit this and start a DEPS roll with the gyp changes first
thing tomorrow.

http://codereview.appspot.com/5796071/diff/8002/src/gpu/GrDefaultTextContext.cpp
File src/gpu/GrDefaultTextContext.cpp (right):

http://codereview.appspot.com/5796071/diff/8002/src/gpu/GrDefaultTextContext....
src/gpu/GrDefaultTextContext.cpp:159: this->flush();
On 2012/03/15 15:57:11, baranowski wrote:
> On 2012/03/15 15:14:31, bsalomon wrote:
> > Would it be better for the batched base class to do this so that each
subclass
> > doesn't have to remember:
> > 
> > this->INHERITED::finish();
> > fStrike = NULL;
> > fContext->setMatrix(...);
> 
> I think the cleanest and most maintainable way to do init/finish is to think
of
> them as of pseudo-constructor/destructor. So the first thing subclass::init()
> does is calling INHERITED::init(). And the last thing subclass::finish() does
is
> calling INHERITED::finish().
> 
> And even if we call finish() earlier, calling overridden method from
> baseclass::finish() might be error-prone, since it's so different from
> constructor/destructor semantics.

Ok.

[baranowski, 2012-03-15 18:59:49]

On 2012/03/15 17:33:56, bsalomon wrote:
> LGTM. OK if I commit tomorrow morning? This will require a change to chrome's
> skia.gyp. I can commit this and start a DEPS roll with the gyp changes first
> thing tomorrow.

Thanks. Yes, committing tomorrow is OK. But if you have something more urgent to
do, there is no rush.

[bsalomon, 2012-03-16 14:10:53]

Landed at r3412. (I added a comment or two, removed the instance count debug
code, and removed a few lines that just contained a semicolon).