code review 1240047: Make it possible to draw text using an arbitrary Painter.

Make it possible to draw text using an arbitrary Painter.
This requires that Dx and Dy are in Rasterizer rather than
RGBAPainter. RGBAContext then becomes a specialized version
of the more general Context. Dx and Dy are removed from RGBAPainter
and AlphaPainter as they're now unnecessary.

One other minor change: avoid allocating table when gamma is 1.

BTW the Painter abstraction is lovely - a great example
of composability in Go (I think Russ's suggestion of a nil
ss in Paint to signify the end of the spans is probably
slightly better than the current bool flag though).

[rog, 2010-05-24 16:05:06]

Hello nigeltao_golang (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[nigeltao, 2010-05-24 16:51:08]

On 2010/05/24 16:05:06, rog wrote:
> Hello nigeltao_golang (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change.

Moving the {Dx,Dy} fields from the painter to the rasterizer seems fair enough.

I'm less convinced about the differentiation between an RGBAContext and a
Context, unless there's a pressing need for it. (And FYI, I'm thinking of
renaming "Context" to "Canvas"). If you do feel inclined to do this, please make
it a separate change from this one -- smaller, fine-grained self-contained
changes are generally easier to understand and review, and to roll back in case
of breakage. Also, embedding a Context into an RGBAContext seems to me like an
inheritance-based design. Is it possible for a Context to take a
freetype.Painter, where a freetype.Painter is an interface type that contains
raster.Painter but also SetColor(color image.Color) or SetOffset(dx, dy int) or
whatever else you need?

[nigeltao, 2010-05-24 16:51:25]

http://codereview.appspot.com/1240047/diff/2001/3002
File freetype/raster/paint.go (right):

http://codereview.appspot.com/1240047/diff/2001/3002#newcode215
freetype/raster/paint.go:215: a *[256]uint16
I'd rather keep this the way it was. Before, creating a GammaCorrectionPainter
was only one malloc, whether or not the gamma was 1.0. After, creating a GCP is
one or two mallocs, and looking up the table is a further indirection.

[nigeltao, 2010-05-24 17:01:00]

http://codereview.appspot.com/1240047/diff/2001/3002
File freetype/raster/paint.go (right):

http://codereview.appspot.com/1240047/diff/2001/3002#newcode215
freetype/raster/paint.go:215: a *[256]uint16
On 2010/05/24 16:51:25, nigeltao_golang wrote:
> I'd rather keep this the way it was. Before, creating a GammaCorrectionPainter
> was only one malloc, whether or not the gamma was 1.0. After, creating a GCP
is
> one or two mallocs, and looking up the table is a further indirection.

For future reference, I think the more stylish way to do this is to make GCP.a a
slice rather than a pointer to an array. But I'd just revert this change.

[nigeltao, 2010-05-24 17:04:31]

Oh, please also add yourself to AUTHORS and CONTRIBUTORS.

[rog, 2010-05-24 17:27:53]

On 24 May 2010 17:51,  <nigeltao@golang.org> wrote:
> On 2010/05/24 16:05:06, rog wrote:
>>
>> Hello nigeltao_golang (cc: mailto:golang-dev@googlegroups.com),
>
>> I'd like you to review this change.
>
> Moving the {Dx,Dy} fields from the painter to the rasterizer seems fair
> enough.
>
> I'm less convinced about the differentiation between an RGBAContext and
> a Context, unless there's a pressing need for it. (And FYI, I'm thinking
> of renaming "Context" to "Canvas"). If you do feel inclined to do this,
> please make it a separate change from this one -- smaller, fine-grained
> self-contained changes are generally easier to understand and review,
> and to roll back in case of breakage. Also, embedding a Context into an
> RGBAContext seems to me like an inheritance-based design. Is it possible
> for a Context to take a freetype.Painter, where a freetype.Painter is an
> interface type that contains raster.Painter but also SetColor(color
> image.Color) or SetOffset(dx, dy int) or whatever else you need?

a Context can take anything that satisfies the Painter interface.
so, yes, no problem.

it's not really an inheritance-based design (i have no OO background
at all). it's just an encapsulation. RGBAContext just makes it
a little more convenient to draw text to an RGBA image without
having to know about painters.

my "pressing need" was that i needed to draw through a couple
of different Painters when drawing text. in particular, i can draw
through a painter that finds the exact bounding box of the drawn
text, and i can draw through a painter that clips the text to
a given rectangle. i can imagine all kind of other uses for it
(currently you can't draw to an image.Alpha either)

FWIW, i did see rob's comments on the matter earlier, but
having used the package a little, i think Context is an ok name.
it represents pretty well what it does - it's some precalculated
state that speeds up rendering of characters from some particular
font. it's really not a "canvas", by my understanding of the term anyway.

RGBAContext is less necessary (i left it for backward compatibility);
it could just be a function:

DrawRGBAText(c *Context, m *image.RGBA, color image.Color, gamma float)

except that you'd want to pre-calculate the gamma. maybe
making Gamma a separate entity from GammaCorrectionPainter
might be the way to go. then it'd be:

DrawRGBAText(c *Context, m *image.RGBA, color image.Color, gamma *Gamma)

[rog, 2010-05-24 17:34:54]

i guess i was optimising for the possibly common case of
gamma=1.0. it's true it's two mallocs, but the extra indirection
would be negligible if the indirection was done just once
at the start of Paint.

following on from my suggestion of separating Gamma from
GammaCorrectionPainter, you could use a nil *Gamma
to represent gamma=1, and then there would be no
need for the special case.

no need for GammaCorrectionPainter.SetGamma either - just
make the gamma a public field.

On 24 May 2010 17:51,  <nigeltao@golang.org> wrote:
>
> http://codereview.appspot.com/1240047/diff/2001/3002
> File freetype/raster/paint.go (right):
>
> http://codereview.appspot.com/1240047/diff/2001/3002#newcode215
> freetype/raster/paint.go:215: a *[256]uint16
> I'd rather keep this the way it was. Before, creating a
> GammaCorrectionPainter was only one malloc, whether or not the gamma was
> 1.0. After, creating a GCP is one or two mallocs, and looking up the
> table is a further indirection.
>
> http://codereview.appspot.com/1240047/show
>

[nigeltao_gnome, 2010-05-24 23:22:42]

On 24 May 2010 10:27, roger peppe <rogpeppe@gmail.com> wrote:
> RGBAContext is less necessary (i left it for backward compatibility);
> it could just be a function:
>
> DrawRGBAText(c *Context, m *image.RGBA, color image.Color, gamma float)
>
> except that you'd want to pre-calculate the gamma. maybe
> making Gamma a separate entity from GammaCorrectionPainter
> might be the way to go. then it'd be:
>
> DrawRGBAText(c *Context, m *image.RGBA, color image.Color, gamma *Gamma)

If you do the freetype.Painter thing I suggested, then RGBAContext
could just be a function (instead of its own special type). In
particular, NewRGBAContext(m *image.RGBA) could return a Context that
had the right Painter.

Backwards compatability isn't that important right now -- the Context
API isn't part of the C Freetype library and it's very much subject to
change as we explore the space.


On 24 May 2010 10:34, roger peppe <rogpeppe@gmail.com> wrote:
> i guess i was optimising for the possibly common case of
> gamma=1.0. it's true it's two mallocs, but the extra indirection
> would be negligible if the indirection was done just once
> at the start of Paint.

The extra indirection is probably negligible, but it might not be if
it affects the cache. I haven't verified this myself, but this blog
post [0] says that a Java port of git had poor performance because,
amongst other reasons, "the bytes themselves are in a different area
of memory from the container object".

[0] http://loadcode.blogspot.com/2009/12/go-vs-java.html


> following on from my suggestion of separating Gamma from
> GammaCorrectionPainter, you could use a nil *Gamma
> to represent gamma=1, and then there would be no
> need for the special case.

You could do it that way, but then whenever you set the gamma to non-1
then you need to allocate another GCP rather than re-using the
existing one. An Android colleague tells me that non-1 gamma is
actually pretty common.


> no need for GammaCorrectionPainter.SetGamma either - just
> make the gamma a public field.

I don't think that works. SetGamma doesn't just set the gamma field,
it also calculates the GCP.a lookup table.

[nigeltao_gnome, 2010-05-24 23:28:02]

On 24 May 2010 09:05,  <rogpeppe@gmail.com> wrote:
> BTW the Painter abstraction is lovely - a great example
> of composability in Go (I think Russ's suggestion of a nil
> ss in Paint to signify the end of the spans is probably
> slightly better than the current bool flag though).

I like the explicit "done" bool parameter better. Having nil signify
end-of-paint means that the nil []Span means something different from
a zero-length []Span. It's do-able, but it's subtle, whereas the bool
is pretty clear.

[rog, 2010-05-25 06:25:43]

On 25 May 2010 00:28, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
> On 24 May 2010 09:05,  <rogpeppe@gmail.com> wrote:
>> BTW the Painter abstraction is lovely - a great example
>> of composability in Go (I think Russ's suggestion of a nil
>> ss in Paint to signify the end of the spans is probably
>> slightly better than the current bool flag though).
>
> I like the explicit "done" bool parameter better. Having nil signify
> end-of-paint means that the nil []Span means something different from
> a zero-length []Span. It's do-able, but it's subtle, whereas the bool
> is pretty clear.

it doesn't have to mean something different. there's never
any need to pass a zero-length span to a painter, AFAICS,
so end-of-paint could just as well be signified by a len(ss)==0,
which covers both.

[rog, 2010-05-25 06:53:39]

On 25 May 2010 00:22, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
> If you do the freetype.Painter thing I suggested, then RGBAContext
> could just be a function (instead of its own special type). In
> particular, NewRGBAContext(m *image.RGBA) could return a Context that
> had the right Painter.

yes, you could do that.
but i *think* that rob's original point is that putting drawing
attributes in a context doesn't really buy you much - it's
better to pass them explicitly in a function.

this doesn't apply, i don't think, when the attributes have expensive
calculations needed to make them usable, which is the case for
most of the stuff in Context.

but it does apply to (at least) the destination image and the colour - there's
no precalculation involved, just a simple assignment.

> On 24 May 2010 10:34, roger peppe <rogpeppe@gmail.com> wrote:
>> i guess i was optimising for the possibly common case of
>> gamma=1.0. it's true it's two mallocs, but the extra indirection
>> would be negligible if the indirection was done just once
>> at the start of Paint.
>
> The extra indirection is probably negligible, but it might not be if
> it affects the cache. I haven't verified this myself, but this blog
> post [0] says that a Java port of git had poor performance because,
> amongst other reasons, "the bytes themselves are in a different area
> of memory from the container object".

if it's cache performance you're concerned about, then i'm sure it
would be better to separate out the gamma correction table
from the Context. as it is, if you have several contexts, each for
a different font, each will have its own gamma correction table,
where only one is necessary. also, if you're worried about
indirections, it would be much better to have the inner loop
in GammaCorrectionPainter indirect into the table only once per span
(saving the array bounds checks too).

a separate gamma correction object might potentially be
useful in other places too.

type GammaCorrect [256]uint16

func (g *GammaCorrect) Lookup(a uint32) uint32
func (g *GammaCorrect) Init(gamma float) *GammaCorrect

 if you were concerned about the cost of the call to Lookup,
you could still inline the body of Lookup into the GammaCorrectionPainter
(until the compiler could inline it).

> You could do it that way, but then whenever you set the gamma to non-1
> then you need to allocate another GCP rather than re-using the
> existing one. An Android colleague tells me that non-1 gamma is
> actually pretty common.

a non-1 gamma might be pretty common, but how often does
a screen change gamma? :-)

>> no need for GammaCorrectionPainter.SetGamma either - just
>> make the gamma a public field.
>
> I don't think that works. SetGamma doesn't just set the gamma field,
> it also calculates the GCP.a lookup table.

yes, i realise that. i was in a hurry so not clear, sorry. when i said
gamma above, i meant a gamma correction object, such as
in the interface proposed above.

[nigeltao_gnome, 2010-05-25 18:57:46]

On 24 May 2010 23:25, roger peppe <rogpeppe@gmail.com> wrote:
> it doesn't have to mean something different. there's never
> any need to pass a zero-length span to a painter, AFAICS,
> so end-of-paint could just as well be signified by a len(ss)==0,
> which covers both.

The MonochromePainter takes a []Span, filters out a subset, and passes
a possibly shorter []Span on to its wrapped Painter. Under your
scheme, it would have to do a little check that its shorter []Span
wasn't zero length. That's a subtlety I don't have to deal with when I
use the explicit done bool.

[rog, 2010-05-25 19:27:51]

On 25 May 2010 19:57, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
> On 24 May 2010 23:25, roger peppe <rogpeppe@gmail.com> wrote:
>> it doesn't have to mean something different. there's never
>> any need to pass a zero-length span to a painter, AFAICS,
>> so end-of-paint could just as well be signified by a len(ss)==0,
>> which covers both.
>
> The MonochromePainter takes a []Span, filters out a subset, and passes
> a possibly shorter []Span on to its wrapped Painter. Under your
> scheme, it would have to do a little check that its shorter []Span
> wasn't zero length. That's a subtlety I don't have to deal with when I
> use the explicit done bool.

that is true, but there are other subtleties involved in the current
scheme (like the fact that you can't just bail out at the start
of the function if done==true)

i think the relatively few Painters that eliminate spans can
easily maintain the len(ss) > 0 invariant (it's just one if statement)
and be happy that they're saving cpu cycles by avoiding the
call to the sub-painter.

but i don't mind really. just a mild preference.

[nigeltao, 2010-05-26 18:02:25]

This change does three things:
1. It moves Dx,Dy from the painters to the rasterizer.
2. It makes the gamma correction table lazily allocated.
3. It factors out a Context out of a RGBAContext.

I'm happy with 1, unhappy with 2, and am still thinking about 3. If you update
this change to do only 1 (and the AUTHORS + CONTRIBUTORS edit) then I'll happily
check this in.

[nigeltao, 2010-05-26 18:04:17]

Oh, don't forget to update the change description via "hg change 1240047".

[nigeltao_gnome, 2010-05-26 20:15:40]

On 26 May 2010 11:02,  <nigeltao@golang.org> wrote:
> This change does three things:
> 1. It moves Dx,Dy from the painters to the rasterizer.
> 2. It makes the gamma correction table lazily allocated.
> 3. It factors out a Context out of a RGBAContext.
>
> I'm happy with 1, unhappy with 2, and am still thinking about 3. If you
> update this change to do only 1 (and the AUTHORS + CONTRIBUTORS edit)
> then I'll happily check this in.

Ah, I'm guessing that it's bedtime in your timezone. I'll just do this
myself (http://codereview.appspot.com/1332041).