code review 5229053: graphics/detect: haar cascade object detector

graphics/detect: haar cascade object detector

No trainer yet, just a parser for the OpenCV XML format. Image tests
will follow when I take some headshots. (There are lots of face
libraries, but none of them are free to distribute.)

[crawshaw1, 2011-10-12 01:02:42]

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

I'd like you to review this change to
https://crawshaw%40google.com@code.google.com/p/graphics-go/

[nigeltao, 2011-10-16 11:44:26]

There's a lot of code in this change. I've just made some superficial comments
as a first pass.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/detect.go
File graphics/detect/detect.go (right):

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/detect.go#new...
graphics/detect/detect.go:5: package detect
This package needs a documentation comment. You are declaring a lot of exported
(starts-with-a-capital-letter) types and functions. It is not clear to me how it
all fits together.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/integral.go
File graphics/detect/integral.go (right):

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/integral.go#n...
graphics/detect/integral.go:13: type IntegralColor struct {
Are users of the detect package supposed to use these types? If not, this name
should start with a lower case "i", and similarly below.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/opencv_parser.go
File graphics/detect/opencv_parser.go (right):

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/opencv_parser...
graphics/detect/opencv_parser.go:8: "strings"
Sort imports.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/opencv_parser...
graphics/detect/opencv_parser.go:43: type sliceReader struct {
This should be unnecessary. Use a bytes.Buffer instead.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/opencv_parser...
graphics/detect/opencv_parser.go:58: vals := strings.Split(r, " ")
I think you should be able to use fmt.Sscanf.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/opencv_parser...
graphics/detect/opencv_parser.go:158: cascade, name, err = buildCascade(s)
return buildCascade(s)

[nigeltao, 2011-10-17 04:40:54]

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/integral.go
File graphics/detect/integral.go (right):

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/integral.go#n...
graphics/detect/integral.go:32: type IntegralImage struct {
Another thing: does this type have to implement the image.Image interface? I
feel like it would be easier if you just had:
type integral struct {
  // Same as you have now.
}
and simply dropped the awkward-looking and otherwise-unused (??) IntegralColor
and IntegralColorModel types, and the Bounds, ColorModel, AtC and At methods.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/integral.go#n...
graphics/detect/integral.go:96: Pix:    make([]uint64, b.Max.Y*b.Max.X),
I think that you are assuming that b.Min is (0, 0)

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/integral.go#n...
graphics/detect/integral.go:136: if y > 0 && x > 0 {
Again, you are assuming that b.Min is (0, 0).

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/projector.go
File graphics/detect/projector.go (right):

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/projector.go#...
graphics/detect/projector.go:11: // Projector allows projecting from a source
Rectanlge onto a target Rectangle.
Typo in "Rectangle".

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/projector.go#...
graphics/detect/projector.go:21: func NewProjector(o image.Rectangle, n
image.Rectangle) *Projector {
"o" and "n" are unusual argument names.

[crawshaw1, 2011-10-22 16:02:41]

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/detect.go
File graphics/detect/detect.go (right):

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/detect.go#new...
graphics/detect/detect.go:5: package detect
On 2011/10/16 11:44:26, nigeltao wrote:
> This package needs a documentation comment. You are declaring a lot of
exported
> (starts-with-a-capital-letter) types and functions. It is not clear to me how
it
> all fits together.

doc.go added and interface simplified

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/integral.go
File graphics/detect/integral.go (right):

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/integral.go#n...
graphics/detect/integral.go:13: type IntegralColor struct {
On 2011/10/16 11:44:26, nigeltao wrote:
> Are users of the detect package supposed to use these types? If not, this name
> should start with a lower case "i", and similarly below.

Done.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/integral.go#n...
graphics/detect/integral.go:32: type IntegralImage struct {
On 2011/10/17 04:40:55, nigeltao wrote:
> Another thing: does this type have to implement the image.Image interface? I
> feel like it would be easier if you just had:
> type integral struct {
>   // Same as you have now.
> }
> and simply dropped the awkward-looking and otherwise-unused (??) IntegralColor
> and IntegralColorModel types, and the Bounds, ColorModel, AtC and At methods.

Done.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/integral.go#n...
graphics/detect/integral.go:96: Pix:    make([]uint64, b.Max.Y*b.Max.X),
On 2011/10/17 04:40:55, nigeltao wrote:
> I think that you are assuming that b.Min is (0, 0)

As above: "To support sum over an entire subimage, we do not truncate pix." The
problem is, if you take a subImage of integral, you can't sum the exact bounds
of the subimage unless you keep one extra row of pixels.

To avoid working out the edge cases and having two sets of bounds within an
integral, I cheat, and use a different way of referencing into pix.

This does waste some memory, and I might get around to fixing it at some point.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/integral.go#n...
graphics/detect/integral.go:136: if y > 0 && x > 0 {
On 2011/10/17 04:40:55, nigeltao wrote:
> Again, you are assuming that b.Min is (0, 0).

As above.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/opencv_parser.go
File graphics/detect/opencv_parser.go (right):

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/opencv_parser...
graphics/detect/opencv_parser.go:8: "strings"
On 2011/10/16 11:44:26, nigeltao wrote:
> Sort imports.

Done.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/opencv_parser...
graphics/detect/opencv_parser.go:43: type sliceReader struct {
On 2011/10/16 11:44:26, nigeltao wrote:
> This should be unnecessary. Use a bytes.Buffer instead.

Done.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/opencv_parser...
graphics/detect/opencv_parser.go:58: vals := strings.Split(r, " ")
On 2011/10/16 11:44:26, nigeltao wrote:
> I think you should be able to use fmt.Sscanf.

Done.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/opencv_parser...
graphics/detect/opencv_parser.go:158: cascade, name, err = buildCascade(s)
On 2011/10/16 11:44:26, nigeltao wrote:
> return buildCascade(s)

Done.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/projector.go
File graphics/detect/projector.go (right):

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/projector.go#...
graphics/detect/projector.go:11: // Projector allows projecting from a source
Rectanlge onto a target Rectangle.
On 2011/10/17 04:40:55, nigeltao wrote:
> Typo in "Rectangle".

Done.

http://codereview.appspot.com/5229053/diff/4001/graphics/detect/projector.go#...
graphics/detect/projector.go:21: func NewProjector(o image.Rectangle, n
image.Rectangle) *Projector {
On 2011/10/17 04:40:55, nigeltao wrote:
> "o" and "n" are unusual argument names.

Done.

[nigeltao, 2011-10-24 06:36:28]

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/detect.go
File graphics/detect/detect.go (right):

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/detect.go#ne...
graphics/detect/detect.go:55: for r := (image.Rectangle{image.Pt(0, 0),
s}).Add(b.Min); r.In(b); r = r.Add(ty) {
s/image.Pt(0, 0)/image.ZP/, or s/(image.Rectangle{image.Pt(0, 0),
s})/image.Rect(0, 0, s.X, s.Y)/.

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/detect.go#ne...
graphics/detect/detect.go:104: for i := 0; i < len(c.Feature); i++ {
for _, f := range c.Feature {

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/doc.go
File graphics/detect/doc.go (right):

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/doc.go#newco...
graphics/detect/doc.go:18: classifier, _, err := detect.ParseOpenCV
Append "(r)".

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/integral.go
File graphics/detect/integral.go (right):

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/integral.go#...
graphics/detect/integral.go:27: func (p *integral) subImage(r image.Rectangle)
*integral {
It seems to me that the rect should be a field of the window struct, not the
integral struct, and integral.integrate should take an image.Rectangle argument.
WDYT?

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/integral.go#...
graphics/detect/integral.go:79: b := src.Bounds()
Move this inside the !ok block.

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/integral.go#...
graphics/detect/integral.go:110: srcg := toGray(src)
I might be naive, but it seems expensive to convert to gray twice: once for
newIntegral and again for newSquaredIntegral.

Would it be better for the integral struct to have two []uint64 values called
sum and sumOfSq (or sum and sum2)?

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/projector.go
File graphics/detect/projector.go (right):

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/projector.go...
graphics/detect/projector.go:13: rx float64
Maybe add some comments that rx, ry are scales, dx, dy are translates, and r is
a clip??

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/projector.go...
graphics/detect/projector.go:20: // newProjector creates a Projector with source
o and target n.
"o" and "n" are wrong.

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/projector.go...
graphics/detect/projector.go:40: func (s *projector) Rect(r image.Rectangle)
image.Rectangle {
Why is Rect capitalized but pt not?

http://codereview.appspot.com/5229053/diff/11001/testdata/opencv.xml
File testdata/opencv.xml (right):

http://codereview.appspot.com/5229053/diff/11001/testdata/opencv.xml#newcode1
testdata/opencv.xml:1: <?xml version="1.0"?>
Does this need a copyright header? Additionally, does it need a comment saying
where was it sourced from?

[bsiegert, 2011-10-24 07:26:24]

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/opencv_parse...
File graphics/detect/opencv_parser.go (right):

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/opencv_parse...
graphics/detect/opencv_parser.go:111: // BUG(crawshaw): tag-based parsing
doesn't seem to work with <_>
If this is a bug in package xml, did you file an issue?

[crawshaw1, 2011-11-01 05:15:03]

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/detect.go
File graphics/detect/detect.go (right):

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/detect.go#ne...
graphics/detect/detect.go:55: for r := (image.Rectangle{image.Pt(0, 0),
s}).Add(b.Min); r.In(b); r = r.Add(ty) {
On 2011/10/24 06:36:29, nigeltao wrote:
> s/image.Pt(0, 0)/image.ZP/, or s/(image.Rectangle{image.Pt(0, 0),
> s})/image.Rect(0, 0, s.X, s.Y)/.

Done.

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/detect.go#ne...
graphics/detect/detect.go:104: for i := 0; i < len(c.Feature); i++ {
On 2011/10/24 06:36:29, nigeltao wrote:
> for _, f := range c.Feature {

Done.

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/doc.go
File graphics/detect/doc.go (right):

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/doc.go#newco...
graphics/detect/doc.go:18: classifier, _, err := detect.ParseOpenCV
On 2011/10/24 06:36:29, nigeltao wrote:
> Append "(r)".

Done.

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/integral.go
File graphics/detect/integral.go (right):

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/integral.go#...
graphics/detect/integral.go:27: func (p *integral) subImage(r image.Rectangle)
*integral {
On 2011/10/24 06:36:29, nigeltao wrote:
> It seems to me that the rect should be a field of the window struct, not the
> integral struct, and integral.integrate should take an image.Rectangle
argument.
> WDYT?

Done: the first half of your suggestion. Got that far and realized I did not
need the second half, nor a lot of other code. Thanks!

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/integral.go#...
graphics/detect/integral.go:79: b := src.Bounds()
On 2011/10/24 06:36:29, nigeltao wrote:
> Move this inside the !ok block.

Done.

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/integral.go#...
graphics/detect/integral.go:110: srcg := toGray(src)
On 2011/10/24 06:36:29, nigeltao wrote:
> I might be naive, but it seems expensive to convert to gray twice: once for
> newIntegral and again for newSquaredIntegral.
> 
> Would it be better for the integral struct to have two []uint64 values called
> sum and sumOfSq (or sum and sum2)?

Did something similar. I am trying to maintain the conceptual simplicity of the
integral struct.

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/opencv_parse...
File graphics/detect/opencv_parser.go (right):

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/opencv_parse...
graphics/detect/opencv_parser.go:111: // BUG(crawshaw): tag-based parsing
doesn't seem to work with <_>
On 2011/10/24 07:26:24, bsiegert wrote:
> If this is a bug in package xml, did you file an issue?

I have http://codereview.appspot.com/5298061 out for review.

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/projector.go
File graphics/detect/projector.go (right):

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/projector.go...
graphics/detect/projector.go:13: rx float64
On 2011/10/24 06:36:29, nigeltao wrote:
> Maybe add some comments that rx, ry are scales, dx, dy are translates, and r
is
> a clip??

Done.

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/projector.go...
graphics/detect/projector.go:20: // newProjector creates a Projector with source
o and target n.
On 2011/10/24 06:36:29, nigeltao wrote:
> "o" and "n" are wrong.

Done.

http://codereview.appspot.com/5229053/diff/11001/graphics/detect/projector.go...
graphics/detect/projector.go:40: func (s *projector) Rect(r image.Rectangle)
image.Rectangle {
On 2011/10/24 06:36:29, nigeltao wrote:
> Why is Rect capitalized but pt not?

Done.

http://codereview.appspot.com/5229053/diff/11001/testdata/opencv.xml
File testdata/opencv.xml (right):

http://codereview.appspot.com/5229053/diff/11001/testdata/opencv.xml#newcode1
testdata/opencv.xml:1: <?xml version="1.0"?>
On 2011/10/24 06:36:29, nigeltao wrote:
> Does this need a copyright header? Additionally, does it need a comment saying
> where was it sourced from?

Copyright added. I made this file myself (the actual classifier it contains is
nonsense).

[nigeltao, 2011-11-02 07:45:55]

LGTM.

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/Makefile
File graphics/detect/Makefile (right):

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/Makefile#new...
graphics/detect/Makefile:9: detect.go\
Add doc.go.

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/detect_test.go
File graphics/detect/detect_test.go (right):

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/detect_test....
graphics/detect/detect_test.go:12: _ "image/jpeg"
Unnecessary?

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/detect_test....
graphics/detect/detect_test.go:46: draw.Draw(m, image.Rect(0, 0, 3, 4),
image.White, b.Min, draw.Src)
When the src image is an image.Uniform like image.White or image.Black,
idiomatic is to make the fourth argument image.ZP instead of b.Min.

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/detect_test....
graphics/detect/detect_test.go:47: draw.Draw(m, image.Rect(3, 4, 4, 5),
image.Black, b.Min, draw.Src)
Is drawing a black rectangle doing anything? An image.NewGray starts as all
black.

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/opencv_parse...
File graphics/detect/opencv_parser_test.go (right):

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/opencv_parse...
graphics/detect/opencv_parser_test.go:60: t.Error(err)
You could s/Error/Fatal/ and drop the returns throughout this function.

[crawshaw1, 2011-11-05 22:09:24]

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/Makefile
File graphics/detect/Makefile (right):

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/Makefile#new...
graphics/detect/Makefile:9: detect.go\
On 2011/11/02 07:45:55, nigeltao wrote:
> Add doc.go.

Done.

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/detect_test.go
File graphics/detect/detect_test.go (right):

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/detect_test....
graphics/detect/detect_test.go:12: _ "image/jpeg"
On 2011/11/02 07:45:55, nigeltao wrote:
> Unnecessary?

Done.

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/detect_test....
graphics/detect/detect_test.go:46: draw.Draw(m, image.Rect(0, 0, 3, 4),
image.White, b.Min, draw.Src)
On 2011/11/02 07:45:55, nigeltao wrote:
> When the src image is an image.Uniform like image.White or image.Black,
> idiomatic is to make the fourth argument image.ZP instead of b.Min.

Done.

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/detect_test....
graphics/detect/detect_test.go:47: draw.Draw(m, image.Rect(3, 4, 4, 5),
image.Black, b.Min, draw.Src)
On 2011/11/02 07:45:55, nigeltao wrote:
> Is drawing a black rectangle doing anything? An image.NewGray starts as all
> black.

Done.

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/opencv_parse...
File graphics/detect/opencv_parser_test.go (right):

http://codereview.appspot.com/5229053/diff/23001/graphics/detect/opencv_parse...
graphics/detect/opencv_parser_test.go:60: t.Error(err)
On 2011/11/02 07:45:55, nigeltao wrote:
> You could s/Error/Fatal/ and drop the returns throughout this function.

Done.

[crawshaw1, 2011-11-05 22:10:40]

*** Submitted as 1e7422ec8285 ***

graphics/detect: haar cascade object detector

No trainer yet, just a parser for the OpenCV XML format. Image tests
will follow when I take some headshots. (There are lots of face
libraries, but none of them are free to distribute.)

R=nigeltao, bsiegert
CC=golang-dev
http://codereview.appspot.com/5229053