Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

image, image/draw: add interfaces for using RGBA64 directly #44808

Closed
zephyrtronium opened this issue Mar 5, 2021 · 22 comments
Closed

image, image/draw: add interfaces for using RGBA64 directly #44808

zephyrtronium opened this issue Mar 5, 2021 · 22 comments

Comments

@zephyrtronium
Copy link
Contributor

@zephyrtronium zephyrtronium commented Mar 5, 2021

I have an application that involves calculating image data at extremely high resolutions, sometimes on the scale of gigapixels, then resampling the results to smaller sizes. Using package x/image/draw to perform the resampling gives excellent results, but rather slowly. One significant reason for this is that every call to At allocates, because the result type is an interface. A quick benchmark shows that this is responsible for 80% of the allocations in my application.

Package x/image/draw already contains specialized fast paths for many of the raw image types in package image. However, because my images are calculated by histogramming sometimes trillions of iterations of a chaotic process, I cannot use those types to accumulate the image. I also cannot copy to such a type prior to rescaling, because the images I'm working with already occupy most of the memory I have available.

In order to resolve this, I propose to create a standardized interface in package image that allows image processing routines to define fast paths that can avoid unnecessary allocations. The interface would be as follows:

package image

// RGBA64Image is an image with pixels that can be transformed directly to
// color.RGBA64.
type RGBA64Image interface {
	// RGBA64At returns the RGBA64 color of the pixel at (x, y). It is
	// equivalent to calling At(x, y).RGBA() and converting the resulting
	// 32-bit values to color.RGBA64, but it may avoid allocations from
	// converting concrete color types to the color.Color interface type.
	RGBA64At(x, y int) color.RGBA64
	Image
}

Additionally, I propose that a corresponding interface be added to package image/draw (with an alias in x/image/draw):

package draw

// RGBA64Image is an Image with a SetRGBA64 method to set the color of a
// single pixel. It is like Image, but using it may mitigate allocations from
// converting concrete color types to the color.Color interface.
type RGBA64Image interface {
	SetRGBA64(x, y int, c color.RGBA64)
	Image
}

I propose that the image types Alpha, Alpha16, CMYK, Gray, Gray16, NRGBA, NRGBA64, Paletted, and RGBA be extended to implement both of these interfaces (noting that RGBA64 already does), and that NYCbCrA, Uniform, and YCbCr be extended to implement the former.

With these interfaces, image processing algorithms in image/draw, x/image/draw, and elsewhere can implement generic fast paths, rather than only having fallbacks for image.Image that force at least an allocation per pixel. An experiment using a modified version of x/image/draw to implement such a fast path for Kernel.Scale shows that resampling an image from 1280x1280 to 128x128 goes from 3296775 to 32775 allocations per operation, a 100x improvement, with approximately a 3x speed improvement. In particular, the primary culprit method in my application disappears completely from the memory profile.


I originally proposed to add an interface with RGBA64At to x/image/draw specifically, rather than to the standard library, because the interface allocations are most impactful when processing extremely large images, and resampling seems like the most likely operation for them. However, I've since talked with others who have had serious performance issues using image/draw for color transformations on many images, and @nigeltao suggested in a comment that these interfaces be added to the standard library.

@gopherbot gopherbot added this to the Proposal milestone Mar 5, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Mar 10, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 10, 2021

Loading

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Mar 11, 2021

Note that the RGBAAt(x, y int) (r, g, b, a uint32) proposal conflicts with the existing func (p *RGBA) RGBAAt(x, y int) color.RGBA in the standard library's package image.

Perhaps add some interfaces to the standard library (and make its concrete image types implement them):


package image

type ImageV2 interface {
    Image
    // AtDotRGBA(x, y) is equivalent to At(x, y).RGBA() but it can be more
    // efficient, as it does not allocate an intermediate color.Color.
    AtDotRGBA(x, y int) (r, g, b, a uint32)
}

// or perhaps

type ImageV2 interface {
    Image
    // RGBA64At(x, y) is equivalent to At(x, y).RGBA(), after packing RGBA's
    // four uint32 return values into one color.RGBA64, but it can be more
    // efficient, as it does not allocate an intermediate color.Color.
    RGBA64At(x, y int) color.RGBA64
}

package draw

type ImageV2 interface {
    Image
    // SetRGBA64(x, y, c) is equivalent to Set(x, y, c) but it can be more
    // efficient, as it does not allocate an intermediate color.Color.
    SetRGBA64(x, y int, c color.RGBA64)
}

golang.org/x/image/draw obviously gets:

type ImageV2 = draw.ImageV2

Yeah, the names are horrible and inconsistent, but we're constrained by Go 1 backwards compatibility...

Loading

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Mar 11, 2021

Loading

@zephyrtronium
Copy link
Contributor Author

@zephyrtronium zephyrtronium commented Mar 11, 2021

@nigeltao
I had forgotten about image.RGBA.RGBAAt. Considering this, the RGBA64At(x, y int) color.RGBA64 interface does seem more appropriate.

Reading through it again, package image also has somewhat of a convention that would suggest a better name. The concrete image types are RGBA, Gray16, &c. whereas the interface types are Image and PalettedImage. So, it would be natural to add RGBA64Image.

Unless there are objections, tomorrow I will retitle and change this proposal to adding

// RGBA64Image is an image with pixels that can be transformed directly to
// RGBA64.
type RGBA64Image interface {
	// RGBA64At returns the RGBA64 color of the pixel at (x, y). It is
	// equivalent to calling At(x, y).RGBA() and converting the resulting
	// 32-bit channels to color.RGBA64, but it may avoid allocations from
	// converting concrete color types to the color.Color interface type.
	RGBA64At(x, y int) color.RGBA64
	Image
}

to package image, updating the concrete image types to implement it, adding a corresponding RGBA64Image interface to image/draw and x/image/draw, updating the concrete image types in package image to implement these interfaces, and implementing fast paths in image/draw and x/image/draw using these types.

Loading

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Mar 11, 2021

Sounds good to me.

With the new fast paths, I'd only bother with:

  • "image.RGBA64Image on draw.RGBA64Image"

and not e.g.

  • "image.Image on draw.RGBA64Image" or
  • "image.RGBA64Image on draw.Image"

Loading

@zephyrtronium zephyrtronium changed the title proposal: x/image/draw: add interface for providing pixel RGBA directly proposal: image, image/draw: add interfaces for using RGBA64 directly Mar 12, 2021
@naisuuuu
Copy link

@naisuuuu naisuuuu commented Apr 12, 2021

I was about to submit a separate proposal to add additional fast paths to x/image/draw for more resampling destination types. This proposal would solve that and many more similar problems in a much more elegant way. Really hoping to see it come to fruition!

Loading

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Apr 16, 2021

@ianlancetaylor as above (March 12), this proposal (affecting the std image package) Sounds Good To Me.

What's the formal process from here? Do I upgrade this issue from Proposals/Incoming to Proposals/Active?

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 18, 2021

Change https://golang.org/cl/311129 mentions this issue: image: add RGBA64Image interface

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 18, 2021

@nigeltao No, that move will be done when the proposal review committee gets to this issue.

I'll add it to the list to consider in the next meeting.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Apr 21, 2021

Seems OK. Adding to minutes.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Apr 21, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

Loading

@rsc rsc moved this from Incoming to Active in Proposals Apr 21, 2021
@rsc rsc moved this from Active to Likely Accept in Proposals Apr 28, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Apr 28, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

Loading

@rsc
Copy link
Contributor

@rsc rsc commented May 5, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Loading

@rsc rsc moved this from Likely Accept to Accepted in Proposals May 5, 2021
@rsc rsc changed the title proposal: image, image/draw: add interfaces for using RGBA64 directly image, image/draw: add interfaces for using RGBA64 directly May 5, 2021
@rsc rsc removed this from the Proposal milestone May 5, 2021
@rsc rsc added this to the Backlog milestone May 5, 2021
@changkun
Copy link
Contributor

@changkun changkun commented Jun 16, 2021

The CL was sent before the freeze. The proposal was also accepted roughly on the border of dev and freeze. Although the beta1 is out for few days, is there still a chance to get this in in the 1.17 release? So that we won't have to wait another 6 months?

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 16, 2021

Someone will have to review the CL.

Once that is done, you'll have to ask for a freeze exception. See https://groups.google.com/g/golang-dev/c/VNJFUxHWLHo/m/PBmGdYqoAAAJ .

Loading

@changkun

This comment has been hidden.

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Jun 17, 2021

The reviewers have since responded. I filed #46811 to discuss the freeze exception.

Loading

@dmitshur dmitshur removed this from the Backlog milestone Jun 18, 2021
@dmitshur dmitshur added this to the Go1.17 milestone Jun 18, 2021
@gopherbot gopherbot closed this in 86743e7 Jun 18, 2021
@dmitshur dmitshur mentioned this issue Jun 28, 2021
83 tasks
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Jun 28, 2021

2 questions via #46688.

In the original proposal, it was suggested that:

I propose that the image types Alpha, Alpha16, CMYK, Gray, Gray16, NRGBA, NRGBA64, Paletted, and RGBA be extended to implement both of these interfaces (noting that RGBA64 already does), and that NYCbCrA, Uniform, and YCbCr be extended to implement the former.

CL 311129 implemented all of that, with the exception of image.Uniform.

@nigeltao Can you confirm leaving out Uniform is intentional and okay? (Asking this question for #46688.)

Also, the image/draw.RGBA64Image interface suggested in the proposal was:

type RGBA64Image interface {
	SetRGBA64(x, y int, c color.RGBA64)
	Image
}

CL 311129 implemented a slightly different one, which includes the RGBA64At(x, y int) color.RGBA64 method:

type RGBA64Image interface {
	image.RGBA64Image
	Set(x, y int, c color.Color)
	SetRGBA64(x, y int, c color.RGBA64)
}

I just want to point it out because it's a difference, and ask @nigeltao to confirm this is intentional and okay for purposes of #46688. Thanks.

(Everything else matches.)

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 29, 2021

Change https://golang.org/cl/331570 mentions this issue: image: add Uniform.RGBA64At and Rectangle.RGBA64At

Loading

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Jun 29, 2021

@nigeltao Can you confirm leaving out Uniform is intentional and okay?

Leaving out image.Uniform in CL 311129 was unintentional.

On further thought, leaving out image.Rectangle (which implements image.Image) in both the CL and this proposal issue was an additional oversight.

My guiding principle is that, if a concrete image.Foo type previously implemented the image.Image interface, it should now also implement the image.RGBA64Image interface. Similarly, if it previously implemented the draw.Image interface, it should now also implement the draw.RGBA64Image interface.

I have sent out CL 331570 to address image.Uniform and image.Rectangle.

I just want to point it out [the interface definitions] because it's a difference, and ask @nigeltao to confirm this is intentional and okay

Yes, it's intentional that each draw.Foo interface is a superset of image.Foo (and for bar.QuxImage to be a superset of bar.Image). Sorry for the oversight in the proposal. Thanks for catching it. No further action required.

Loading

gopherbot pushed a commit that referenced this issue Jun 30, 2021
These types already implemented the Image interface. They should also
implement the RGBA64Image interface (new in Go 1.17)

Updates #44808

Change-Id: I9a2b13e305997088ae874efb95ad9e1648f94812
Reviewed-on: https://go-review.googlesource.com/c/go/+/331570
Trust: Nigel Tao <nigeltao@golang.org>
Run-TryBot: Nigel Tao <nigeltao@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 29, 2021

Change https://golang.org/cl/340049 mentions this issue: image/draw: add RGBA64Image fast path

Loading

gopherbot pushed a commit that referenced this issue Sep 3, 2021
name               old time/op  new time/op  delta
GenericOver-4      15.0ms ± 1%   2.9ms ± 1%  -80.56%  (p=0.008 n=5+5)
GenericMaskOver-4  7.82ms ± 4%  1.69ms ± 2%  -78.38%  (p=0.008 n=5+5)
GenericSrc-4       6.13ms ± 3%  1.66ms ± 1%  -72.90%  (p=0.008 n=5+5)
GenericMaskSrc-4   11.5ms ± 1%   2.0ms ± 0%  -82.77%  (p=0.008 n=5+5)

Updates #44808.

Change-Id: I131cf6fad01708540390a8012d8f2a21e849fe9d
Reviewed-on: https://go-review.googlesource.com/c/go/+/340049
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Nigel Tao <nigeltao@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 24, 2021

Change https://golang.org/cl/351852 mentions this issue: image/draw: add RGBA64Image fast path for RGBA dst

Loading

gopherbot pushed a commit that referenced this issue Sep 27, 2021
This should have been part of https://golang.org/cl/340049 but I
overlooked it. That commit added fast path code when the destination
image was *not* an *image.RGBA. This commit edits func drawRGBA.

name               old time/op  new time/op  delta
RGBA1-4            5.11ms ± 1%  1.12ms ± 1%  -78.01%  (p=0.008 n=5+5)
RGBA2-4            8.69ms ± 1%  2.98ms ± 1%  -65.77%  (p=0.008 n=5+5)

Updates #44808.
Updates #46395.

Change-Id: I899d46d985634fc81ea47ff4f0d436630e8a961c
Reviewed-on: https://go-review.googlesource.com/c/go/+/351852
Trust: Nigel Tao <nigeltao@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants