Add ImageFactory and Buffer to Options by tgrigsby-sc · Pull Request #29 · kolesa-team/go-webp

tgrigsby-sc · 2023-07-11T21:11:18Z

Added an ImagePool and a Buffer as encoder options for improved encoding efficiency:

BenchmarkDecodePooled
BenchmarkDecodePooled-10      	  355448	      3650 ns/op	     581 B/op	       6 allocs/op
BenchmarkDecodeUnPooled
BenchmarkDecodeUnPooled-10    	   99817	     11573 ns/op	   66210 B/op	       8 allocs/op

The ImagePool will be used in the decode process to provide an image of the right dimensions if one already exists, which, once you're done with image, can be put back into the pool. Similarly, passing in the correct size of buffer will prevent multiple mem reallocs while writing.

See the benchmark test for details about usage.

add image pooling

…ate_options_fields exposing private fields

…ate_options_fields whoops

Copilot

Pull request overview

This pull request adds performance optimizations for WebP decoding through object pooling. It introduces an ImagePool for reusing allocated image buffers and a BufferPool for reusing I/O buffers, along with an ImageFactory interface to support custom image allocation strategies.

Changes:

Added ImagePool and BufferPool types for memory pooling
Added ImageFactory interface and DefaultImageFactory implementation to Options
Modified NewDecoder to use ImageFactory and accept a reusable Buffer in Options

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
decoder/pool.go	New file implementing ImagePool and BufferPool for object pooling
decoder/options.go	Added ImageFactory interface and Buffer field to Options struct
decoder/decoder.go	Modified to use ImageFactory and Buffer from Options
decoder/decoder_benchmark_test.go	New benchmark tests comparing pooled vs unpooled decoding
decoder/decoder_test.go	Import reordering (formatting only)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-24T15:27:34Z

decoder/decoder_benchmark_test.go

+			decoder, err := NewDecoder(bytes.NewReader(data), &Options{ImageFactory: imagePool, Buffer: buf})
+			img, err := decoder.Decode()


The error returned by NewDecoder is checked, but if it's non-nil, the benchmark will fatal. However, the subsequent err assignment from decoder.Decode() overwrites the first err variable without checking it. This means if NewDecoder returns an error, it won't be caught until decoder.Decode() is called on a nil decoder, causing a panic. Both errors should be checked separately or assigned to different variables.

Copilot · 2026-01-24T15:27:35Z

decoder/decoder_benchmark_test.go

+			decoder, err := NewDecoder(bytes.NewReader(data), &Options{})
+			img, err := decoder.Decode()


The same error variable issue exists here. The error from NewDecoder is not checked before being overwritten by the error from decoder.Decode(), which could lead to a panic if NewDecoder fails.

Copilot · 2026-01-24T15:27:35Z

decoder/pool.go

+func (n *ImagePool) Get(width, height int) *image.NRGBA {
+	dimPool := n.getPool(width, height)
+
+	img := dimPool.Get().(*image.NRGBA)
+	img.Rect.Max.X = width
+	img.Rect.Max.Y = height
+	return img
+}
+
+func (n *ImagePool) getPool(width int, height int) *sync.Pool {
+	dim := width * height
+
+	n.lock.Lock()
+	dimPool, ok := n.poolMap[dim]
+	if !ok {
+		atomic.AddInt64(&n.Count, 1)
+		dimPool = &sync.Pool{
+			New: func() interface{} {
+				return image.NewNRGBA(image.Rect(0, 0, width, height))
+			},
+		}
+		n.poolMap[dim] = dimPool
+	}
+	n.lock.Unlock()
+	return dimPool
+}
+
+func (n *ImagePool) Put(img *image.NRGBA) {
+	dimPool := n.getPool(img.Rect.Dx(), img.Rect.Dy())
+	dimPool.Put(img)
+}


The receiver variable name 'n' for ImagePool is unconventional. Go convention typically uses the first letter(s) of the type name, so 'ip' or 'p' would be more idiomatic. This should be consistent across all ImagePool methods.

Copilot · 2026-01-24T15:27:35Z

decoder/pool.go

+type ImagePool struct {
+	poolMap map[int]*sync.Pool
+	lock    *sync.Mutex
+	Count   int64


The Count field is exported but lacks documentation. It appears to track the number of different dimension pools created. This field should either have a documentation comment explaining its purpose and whether it's safe to read/modify concurrently, or it should be unexported if it's only for internal use or debugging.

Suggested change

Count int64

// Count tracks the number of distinct dimension pools created. It is incremented

// atomically by ImagePool and may be read concurrently, but callers should treat it

// as read-only and must not modify it directly.

Count int64

Copilot · 2026-01-24T15:27:36Z

decoder/pool.go

+func (n *ImagePool) Get(width, height int) *image.NRGBA {
+	dimPool := n.getPool(width, height)
+
+	img := dimPool.Get().(*image.NRGBA)


This line will panic if the pool contains nil or a value that is not *image.NRGBA. Since sync.Pool doesn't guarantee the type of values returned, this type assertion should be checked to prevent runtime panics. Consider using a comma-ok idiom or ensuring the New function is always called for empty pools.

Copilot · 2026-01-24T15:27:37Z

decoder/pool.go

+type ImagePool struct {
+	poolMap map[int]*sync.Pool
+	lock    *sync.Mutex
+	Count   int64
+}
+
+func NewImagePool() *ImagePool {
+	return &ImagePool{
+		poolMap: make(map[int]*sync.Pool),
+		lock:    &sync.Mutex{},
+	}
+}
+
+func (n *ImagePool) Get(width, height int) *image.NRGBA {
+	dimPool := n.getPool(width, height)
+
+	img := dimPool.Get().(*image.NRGBA)
+	img.Rect.Max.X = width
+	img.Rect.Max.Y = height
+	return img
+}
+
+func (n *ImagePool) getPool(width int, height int) *sync.Pool {
+	dim := width * height
+
+	n.lock.Lock()
+	dimPool, ok := n.poolMap[dim]
+	if !ok {
+		atomic.AddInt64(&n.Count, 1)
+		dimPool = &sync.Pool{
+			New: func() interface{} {
+				return image.NewNRGBA(image.Rect(0, 0, width, height))
+			},
+		}
+		n.poolMap[dim] = dimPool
+	}
+	n.lock.Unlock()
+	return dimPool
+}
+
+func (n *ImagePool) Put(img *image.NRGBA) {
+	dimPool := n.getPool(img.Rect.Dx(), img.Rect.Dy())
+	dimPool.Put(img)
+}
+
+type BufferPool struct {
+	pool *sync.Pool // pointer because noCopy
+}
+
+func NewBufferPool() *BufferPool {
+	return &BufferPool{
+		pool: &sync.Pool{
+			New: func() interface{} {
+				return make([]byte, 0, 1024)
+			},
+		},
+	}
+}
+
+func (b *BufferPool) Get() []byte {
+	return b.pool.Get().([]byte)
+}
+
+func (b *BufferPool) Put(buf []byte) {
+	buf = buf[:0]
+	b.pool.Put(buf)
+}


The ImagePool and BufferPool types lack test coverage. Given that the decoder package has existing test coverage (see decoder_test.go), tests should be added to verify the pool implementations work correctly, including edge cases like concurrent access, proper reuse of pooled objects, and correct handling of different image dimensions.

Copilot · 2026-01-24T15:27:37Z

decoder/pool.go

+}
+
+type BufferPool struct {
+	pool *sync.Pool // pointer because noCopy


The comment states "pointer because noCopy", but there's no noCopy field or implementation in the BufferPool struct. If the intention is to prevent copying of the BufferPool, a noCopy field should be added (sync.Pool itself is not supposed to be copied, but Go doesn't enforce this without an explicit noCopy marker). If the comment is incorrect, it should be removed or clarified.

Copilot · 2026-01-24T15:27:37Z

decoder/decoder.go

@@ -47,22 +48,26 @@ type Decoder struct {
 	sPtr    C.size_t
 }



The function documentation comment for NewDecoder was removed. Public functions should have documentation comments that start with the function name. The comment should be restored and potentially updated to document the new Options.ImageFactory and Options.Buffer fields and their usage patterns.

Suggested change

// NewDecoder creates a Decoder that reads all WebP data from r using the provided options.

//

// If options is nil, a zero-value Options is used. If options.ImageFactory is nil,

// it defaults to DefaultImageFactory.

//

// If options.Buffer is non-nil, it is used as the initial contents and capacity of the

// internal buffer before copying data from r. This allows callers to reuse a buffer to

// reduce allocations. The buffer contents are overwritten with the data read from r.

Copilot · 2026-01-24T15:27:37Z

decoder/pool.go

+	img.Rect.Max.X = width
+	img.Rect.Max.Y = height


Modifying the image rectangle after retrieval from the pool can lead to inconsistent state. If an image is retrieved from the pool with different dimensions than originally allocated, the Pix slice capacity might not match the new dimensions, potentially causing incorrect image data or index out of bounds errors when the image is used. The pool should only return images with the exact dimensions requested, not resize them after retrieval.

Copilot · 2026-01-24T15:27:37Z

decoder/pool.go

+func (b *BufferPool) Put(buf []byte) {
+	buf = buf[:0]
+	b.pool.Put(buf)


The buffer is truncated before being returned to the pool, which is good practice. However, there's no upper limit on buffer size before returning to the pool. If a buffer grows very large during use, it will remain large in the pool and consume excessive memory. Consider checking the buffer's capacity and only returning buffers below a certain threshold to the pool to prevent memory bloat.

tgrigsby-sc and others added 11 commits March 30, 2023 13:19

adding image pool and buffer pool to options

129e827

simplifying after some more testing

1d3590d

moving some things around

9d9f7c0

adding some comments

75612fb

moving things around

8bb1218

woops

26d0161

Merge pull request #1 from travisgrigsby/travisg_20230330-image_pooling

21239f1

add image pooling

exposing private fields

2776c42

Merge pull request #2 from travisgrigsby/travisg_20230330-expose_priv…

7f99873

…ate_options_fields exposing private fields

whoops

a88289c

Merge pull request #3 from travisgrigsby/travisg_20230330-expose_priv…

bde705c

…ate_options_fields whoops

lEx0 requested a review from Copilot January 24, 2026 15:22

Copilot started reviewing on behalf of lEx0 January 24, 2026 15:23 View session

Copilot AI reviewed Jan 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ImageFactory and Buffer to Options#29

Add ImageFactory and Buffer to Options#29
tgrigsby-sc wants to merge 11 commits intokolesa-team:masterfrom
tgrigsby-sc:master

tgrigsby-sc commented Jul 11, 2023

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		decoder, err := NewDecoder(bytes.NewReader(data), &Options{ImageFactory: imagePool, Buffer: buf})
		img, err := decoder.Decode()

-	Count   int64
+	// Count tracks the number of distinct dimension pools created. It is incremented
+	// atomically by ImagePool and may be read concurrently, but callers should treat it
+	// as read-only and must not modify it directly.
+	Count int64

+// NewDecoder creates a Decoder that reads all WebP data from r using the provided options.
+//
+// If options is nil, a zero-value Options is used. If options.ImageFactory is nil,
+// it defaults to DefaultImageFactory.
+//
+// If options.Buffer is non-nil, it is used as the initial contents and capacity of the
+// internal buffer before copying data from r. This allows callers to reuse a buffer to
+// reduce allocations. The buffer contents are overwritten with the data read from r.

Conversation

tgrigsby-sc commented Jul 11, 2023

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants