code review 1849041: image: change image representation from slice-of-slices...

image: change image representation from slice-of-slices to linear buffer,
stride and rect.

[nigeltao, 2010-07-15 06:34:01]

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

I'd like you to review this change.

[nigeltao, 2010-07-15 06:40:28]

CC Roger Peppe.

[rog, 2010-07-15 08:24:22]

is there a reason why Rect and Point can't be defined outside
exp/draw? then image could use Rect rather
than X0, X1, Y0, Y1 etc. and Image.At could be Image.At(Point)
rather than Image.At(int, int).

BTW, not to denigrate this change, but it does not provide the same
functionality
as the plan 9 images, which allowed the *external* coordinates
of an image to be an arbitrary rectangle.
the Image interface presupposes that all addressable
pixels lie inside ((0, 0), (width, height))

to remove that restriction, Image would need to be defined something like this:

type Image interface {
        ColorModel() ColorModel
        Rect() (x0, x1, y0, y1 int)
        At(x, y) Color
}

or if Rectangle was defined outside exp/draw (perhaps in image itself):

type Image interface {
        ColorModel() ColorModel
        Rect() Rectangle
        At(Point) Color
}

[nigeltao_gnome, 2010-07-15 11:56:52]

On 15 July 2010 18:24, roger peppe <rogpeppe@gmail.com> wrote:
> BTW, not to denigrate this change, but it does not provide the same
> functionality
> as the plan 9 images, which allowed the *external* coordinates
> of an image to be an arbitrary rectangle.
> the Image interface presupposes that all addressable
> pixels lie inside ((0, 0), (width, height))

Three things:

I'm not sure what you mean exactly by *external* coordinates. Can you elaborate?

There is a difference between the Image interface and the RGBA type.
You can copy an RGBA and/or fiddle with its X0, Y0, X1, and Y1 fields
to reslice (in two dimensions) an RGBA.

A struct isn't restricted to implementing only one interface. Perhaps
not all Images are subsettable, but you could declare:
type SubImage interface {
    Image
    Rect() (x0, y0, x1, y1 int)
}
and some Images could also be SubImages.

[rog, 2010-07-15 15:43:53]

On 15 July 2010 12:56, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
> I'm not sure what you mean exactly by *external* coordinates. Can you
elaborate?

by external coordinates, i mean the coordinates as
passed to Image.At.

currently, the top left pixel of an image is always at (0, 0)
and the bottom right at (i.Width()-1, i.Height()-1).

it is useful to be able to break that invariant; for
instance so that the image being drawn to
stores pixels for only a small subset of a virtual
image. it's convenient way to implement scrolling.
it can also be used along with subsettable images to ensure
that drawing is clipped to a given rectangle.

subsettable images don't cater for this functionality AFAICS.

[r2, 2010-07-15 16:35:10]

On Jul 15, 2010, at 8:43 AM, roger peppe wrote:

> On 15 July 2010 12:56, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
>> I'm not sure what you mean exactly by *external* coordinates. Can  
>> you elaborate?
>
> by external coordinates, i mean the coordinates as
> passed to Image.At.
>
> currently, the top left pixel of an image is always at (0, 0)
> and the bottom right at (i.Width()-1, i.Height()-1).
>
> it is useful to be able to break that invariant; for
> instance so that the image being drawn to
> stores pixels for only a small subset of a virtual
> image. it's convenient way to implement scrolling.
> it can also be used along with subsettable images to ensure
> that drawing is clipped to a given rectangle.
>
> subsettable images don't cater for this functionality AFAICS.

nigel, i mentioned this issue in our chat the other day. i can provide  
more detail in a higher-bandwidth medium.

-rob

[r, 2010-08-10 02:00:03]

http://codereview.appspot.com/1849041/diff/17001/10007
File src/pkg/image/image.go (right):

http://codereview.appspot.com/1849041/diff/17001/10007#newcode20
src/pkg/image/image.go:20: // Buf[0] is the origin of the image's backing store.
It is the top-left
backing store is a windowing term. this is just storage.

http://codereview.appspot.com/1849041/diff/17001/10007#newcode27
src/pkg/image/image.go:27: // dimensional slice of the backing store that makes
up this image.
slice of pixels
s/makes/make/

http://codereview.appspot.com/1849041/diff/17001/10007#newcode91
src/pkg/image/image.go:91: // The image's pixels are elements 0:p.Width() of
that row, not p.X0:p.X1.
s/that row/the return value/

http://codereview.appspot.com/1849041/diff/17001/10007#newcode118
src/pkg/image/image.go:118: // Buf[0] is the origin of the image's backing
store. It is the top-left
ditto etc. check everywhere

http://codereview.appspot.com/1849041/diff/17001/10007#newcode125
src/pkg/image/image.go:125: // dimensional slice of the backing store that makes
up this image.
ditto

http://codereview.appspot.com/1849041/diff/17001/10007#newcode140
src/pkg/image/image.go:140: // The image's pixels are elements 0:p.Width() of
that row, not p.X0:p.X1.
ditto

http://codereview.appspot.com/1849041/diff/17001/10007#newcode167
src/pkg/image/image.go:167: // Buf[0] is the origin of the image's backing
store. It is the top-left
and so on.

[nigeltao_gnome, 2010-08-10 05:16:54]

Sorry, that CL wasn't quite ready for review before. It is now.

Please take a fresh look.

[r, 2010-08-10 06:13:46]

LGTM

[nigeltao, 2010-08-10 06:35:01]

*** Submitted as http://code.google.com/p/go/source/detail?r=31e80f0b0959 ***

image: change image representation from slice-of-slices to linear buffer,
stride and rect.

R=r
CC=golang-dev, rog
http://codereview.appspot.com/1849041