Odd behaviour of SubImage() methods in package image

[Holger Knauer]

I stumbled upon what I think is a rather odd bevahiour of all the SubImage methods of the concrete image types in package image.
Each of these methods starts by intersecting it's own images bounds with the given rectangle:

r = r.Intersect(p.Rect)

then, this intersection result is directly set as Rect (and therefore effectively Bounds) of the newly returned image.

While this is not without a certain logic, it results in the newly returned subimage's bounds be given in the coordinate spaces of the original image.
E.g: If I create a subimage with r = (25,25)-(75,75) from an image with bounds (0,0)-(100,100), the resulting subimage's bounds will now be (25,25)-(75,75) instead of (0,0)-(50,50) what one would come to expect if one didn't know this image was created via a call to SubImage().

So i believe instead of just the intersected r, the new subimage's Rect should be set to a new Rectangle of (0,0)-(r.Dx(), r.Dy()).

But maybe there is a strong reason to do it the way it's implemented?

[Rob Pike]

It's in the same coordinate system because it's the same pixels. The
pixel at (36, 44) is the same pixel, with the same address, in both
the original and the subimage. This makes sense for a lot of reasons,
particularly for windowing applications but also because of the way it
allows tiling to work literally seamlessly. Pixel-to-address
arithmetic is also simpler.

-rob

[roger peppe]

That is indeed obvious, but it's often not a desirable thing to do, as
any drawing to
a translatedImage will now take the slow path (which can be orders of magnitude
slower).

It would be nice if there was a way to avoid that.

On 9 November 2012 16:58, <benjami...@ai-solutions.com> wrote:
> Apologies if this is so obvious as to be annoying, but I'm always pleased by
> how easy it is to get the behavior one wants with a tiny bit of wrapping:
>
> type translatedImage struct {
> wrapped Image
> newOrigin Point
> }
>
> func (i translatedImage) At(x, y int) color.Color {
> return i.wrapped.At(x+i.newOrigin.x, y+i.newOrigin.y)
> }
>
> func (i translatedImage) Bounds() Rectangle {
> r := i.wrapped.Bounds()
> return Rectangle{r.Min.Sub(i.newOrigin), r.Max.Sub(i.newOrigin)}
> }
>
> func OSubImage(Image i, Rectangle r) Image {
> //cast i to ii, something that has a SubImage method...
> /*
> ...
> */
> return translatedImage(ii.SubImage(r), r.Min)
> }
>
> ... or something like that.
>
>>
> --
>
>

[Holger Knauer]

Thanks for the clarification, Rob. That makes perfect sense especially after I had a closer look at PixOffset() and how it is used whenever a pixel has to be retrieved or set.
I guess it was just a matter of understanding the logic used here initially. Maybe a short addendum to the documentation for image.Image.Bound() might have done that for me:

// Bounds returns the domain for which At can return non-zero color.
// The bounds do not necessarily contain the point (0, 0).

-> "Thus the width and height of an image are returned by Bounds().Dx() and Bounds.Dy() respectively"

But then again I should have understood what's going on after reading the documentation for image.Image.At()

[Nigel Tao]

On Sat, Nov 10, 2012 at 4:14 AM, Holger Knauer <holger...@gmail.com> wrote:
> Maybe a short addendum to the documentation for image.Image.Bound() might
> have done that for me:

The package overview at http://golang.org/pkg/image/ also says: See
"The Go image package" for more details:
http://golang.org/doc/articles/image_package.html

[Nigel Tao]

On Mon, Dec 24, 2012 at 1:53 AM, <ma...@ungerik.net> wrote:

> On Friday, November 9, 2012 5:31:34 PM UTC+1, Rob Pike wrote:
>> It's in the same coordinate system because it's the same pixels. The
>> pixel at (36, 44) is the same pixel, with the same address, in both
>> the original and the subimage.
>

> No, because the sub image is initialized with Pix: p.Pix[i:], where i is the
> byte offset of the sub images origin.
> Subsequently subImage.At(0,0) must be valid, because after slicing the sub
> images data starts at the first byte of its Pix.
>
> But according to the spec subImage.At(originX,originY) should be the first
> byte.
>
> The fix to make it work like the spec would be to simply not slice the sub
> image Pix.

Rob is right: subImage.At(0, 0) is not necessarily valid.


> When we are at it, could we include SubImage() into the Image interface? The
> only type that's currently not supporting SubImage it is Uniform. I think
> simply returning the same Uniform as SubImage would do no harm.

Adding a method to the image.Image interface will be backwards
incompatible with any existing Go code that says

type myImage struct{
// myImage fields
}

// myImage methods

var m image.Image = myImage{}

Not all image.Image implementations live in package image. For
example, the Go Tour (http://tour.golang.org/#57) discusses writing
your own implementation.

[Nigel Tao]

Sorry, I hit "Send" too soon.

On Mon, Dec 24, 2012 at 3:43 PM, Nigel Tao <nige...@golang.org> wrote:
> On Mon, Dec 24, 2012 at 1:53 AM, <ma...@ungerik.net> wrote:
>> On Friday, November 9, 2012 5:31:34 PM UTC+1, Rob Pike wrote:
>>> It's in the same coordinate system because it's the same pixels. The
>>> pixel at (36, 44) is the same pixel, with the same address, in both
>>> the original and the subimage.
>>
>> No, because the sub image is initialized with Pix: p.Pix[i:], where i is the
>> byte offset of the sub images origin.
>> Subsequently subImage.At(0,0) must be valid, because after slicing the sub
>> images data starts at the first byte of its Pix.
>>
>> But according to the spec subImage.At(originX,originY) should be the first
>> byte.
>>
>> The fix to make it work like the spec would be to simply not slice the sub
>> image Pix.
>
> Rob is right: subImage.At(0, 0) is not necessarily valid.

An image's bounds have a top-left corner, called Rect.Min for the
concrete implementations in package image. The first byte of Pix
correspond to this corner but the bounds do not necessarily contain
the origin (0, 0). A sub-image's Pix bytes for (3, 4) have the same
memory address (but not the same Pix offset) as the super-image's Pix
bytes for (3, 4).

http://golang.org/doc/articles/image_package.html has more details
(and some pictures). It also says, "A common mistake is assuming that
an Image's bounds start at (0, 0). For example, an animated GIF
contains a sequence of Images, and each Image after the first
typically only holds pixel data for the area that changed, and that
area doesn't necessarily start at (0, 0). The correct way to iterate
over an Image m's pixels looks like:

b := m.Bounds()
for y := b.Min.Y; y < b.Max.Y; y++ {
for x := b.Min.X; y < b.Max.X; x++ {
doStuffWith(m.At(x, y))
}
}

[Erik Unger]

On Mon, Dec 24, 2012 at 5:43 AM, Nigel Tao <nige...@golang.org> wrote:

> The fix to make it work like the spec would be to simply not slice the sub
> image Pix.

Rob is right: subImage.At(0, 0) is not necessarily valid.

I didn't say that. I said that the implementation doesn't follow the specification that Rob is referring to.

At() uses PixOffset(). But so does SubImage() to create a slice of Pix.

If At() is used then on the sub-image, it creates an offset into the _already_ offset Pix slice, effectively doubling the offset.

Because of the offset of the sub-image Pix slice, the only way to get the min coordinate pixel with the current implementation is to use At(0,0), which is wrong according to the spec. So either the spec is wrong or the implementations of SubImage are wrong.

-Erik

[Nigel Tao]

On Tue, Dec 25, 2012 at 3:27 AM, Erik Unger <er...@erikunger.com> wrote:
> On Mon, Dec 24, 2012 at 5:43 AM, Nigel Tao <nige...@golang.org> wrote:
> If At() is used then on the sub-image, it creates an offset into the
> _already_ offset Pix slice, effectively doubling the offset.

I don't see any double counting: http://play.golang.org/p/uQMgihJkEG

[Erik Unger]

You are right: http://play.golang.org/p/b71OVRB7ed

I had an error somewhere else in my code.