code review 904041: Freetype-Go: batch Spans when painting.

Freetype-Go: batch Spans when painting.
Introduce MonochromePainter and GammaCorrectionPainter.

[nigeltao, 2010-04-08 13:39:47]

Hello r@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[nigeltao, 2010-04-08 13:56:34]

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

http://codereview.appspot.com/904041/diff/2001/3004#newcode136
freetype/raster/paint.go:136: p, q := ss[i].A/M, (ss[i].A%M)>>9
The image.Color interface's RGBA method currently returns four uint32s, where
fully opaque is represented by alpha == 1<<32-1. raster.Painter follows this
convention.

I'm beginning to think that it would be better (for both image.Color and
raster.Painter) for the type to remain uint32 but for fully opaque to be
1<<16-1. Using 16-bit alpha lets you multiply two values without overflowing a
uint32, and I don't think the loss in precision will be missed.

If you really need high dynamic range imaging, then perhaps a HDRColor interface
should define a HDRRGBA method that returns four float64s and fully opaque means
alpha == 1.0.

[r, 2010-04-08 17:53:06]

http://codereview.appspot.com/904041/diff/2001/3002
File example/gamma/main.go (right):

http://codereview.appspot.com/904041/diff/2001/3002#newcode69
example/gamma/main.go:69: return
if this is a test, why not os.Exit(1) so it's easier to script it?

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

http://codereview.appspot.com/904041/diff/2001/3004#newcode24
freetype/raster/paint.go:24: // value Span will occur only once per
rasterization, not one per Paint call.
s/one/once/

http://codereview.appspot.com/904041/diff/2001/3004#newcode136
freetype/raster/paint.go:136: p, q := ss[i].A/M, (ss[i].A%M)>>9
this is a decision to be made by professionals. what is the web wisdom? (i
realize those may be contradictory remarks.)

[rsc1, 2010-04-08 18:22:06]

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

http://codereview.appspot.com/904041/diff/2001/3004#newcode136
freetype/raster/paint.go:136: p, q := ss[i].A/M, (ss[i].A%M)>>9
Would that help here?  You're scaling p down to uint8, not uint16,
so it looks like this code wouldn't change much.

Are you proposing to scale down all the values (R,G,B) or just A?

[nigeltao, 2010-04-08 23:04:11]

http://codereview.appspot.com/904041/diff/2001/3002
File example/gamma/main.go (right):

http://codereview.appspot.com/904041/diff/2001/3002#newcode27
example/gamma/main.go:27: m.Pixel[y][x] = image.AlphaColor{0}
On a separate note, would it be worth having a zero([]T) built-in, where zero :
copy :: memset : memcpy?

http://codereview.appspot.com/904041/diff/2001/3002#newcode69
example/gamma/main.go:69: return
On 2010/04/08 17:53:06, r wrote:
> if this is a test, why not os.Exit(1) so it's easier to script it?

Done.

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

http://codereview.appspot.com/904041/diff/2001/3004#newcode24
freetype/raster/paint.go:24: // value Span will occur only once per
rasterization, not one per Paint call.
On 2010/04/08 17:53:06, r wrote:
> s/one/once/

Done.

http://codereview.appspot.com/904041/diff/2001/3004#newcode136
freetype/raster/paint.go:136: p, q := ss[i].A/M, (ss[i].A%M)>>9
On 2010/04/08 18:22:06, rsc1 wrote:
> Would that help here?  You're scaling p down to uint8, not uint16,
> so it looks like this code wouldn't change much.

p becomes 8-bit, but that leaves q at 25-bit, so I have to shift q to use it to
blend 16-bit alphas.

The exp/draw code also shifts 32-bit RGBA values to 16-bit when blending.


> Are you proposing to scale down all the values (R,G,B) or just A?

I'm proposing to scale down all values.

http://codereview.appspot.com/904041/diff/2001/3004#newcode136
freetype/raster/paint.go:136: p, q := ss[i].A/M, (ss[i].A%M)>>9
On 2010/04/08 17:53:06, r wrote:
> this is a decision to be made by professionals. what is the web wisdom? (i
> realize those may be contradictory remarks.)

One data point is that Mac OS X's CoreImage uses float32 everywhere. So says the
web:
http://arstechnica.com/apple/reviews/2005/04/macosx-10-4.ars/15

If we want to offer similar quality in all-software image processing, then I
think we need a separate HDRColor interface. If we have that, then we don't need
the uint32 values of Color.RGBA to be as general as possible.

I'll try to see what the web says Adobe does...

[r, 2010-04-12 19:53:15]

LGTM

we can address the bigger questions in another round

[nigeltao, 2010-04-13 12:30:01]

*** Submitted as
http://code.google.com/p/freetype-go/source/detail?r=74c3a050dfcc ***

Freetype-Go: batch Spans when painting.
Introduce MonochromePainter and GammaCorrectionPainter.

R=r, rsc
CC=golang-dev
http://codereview.appspot.com/904041