-
Notifications
You must be signed in to change notification settings - Fork 39
Add ImageFactory and Buffer to Options #29
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
base: master
Are you sure you want to change the base?
Changes from all commits
129e827
1d3590d
9d9f7c0
75612fb
8bb1218
26d0161
21239f1
2776c42
7f99873
a88289c
bde705c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ package decoder | |
| */ | ||
| import "C" | ||
| import ( | ||
| "bytes" | ||
| "errors" | ||
| "fmt" | ||
| "image" | ||
|
|
@@ -47,22 +48,26 @@ type Decoder struct { | |
| sPtr C.size_t | ||
| } | ||
|
|
||
| // NewDecoder return new decoder instance | ||
| func NewDecoder(r io.Reader, options *Options) (d *Decoder, err error) { | ||
| var data []byte | ||
| if options == nil { | ||
| options = &Options{} | ||
| } | ||
|
|
||
| if options.ImageFactory == nil { | ||
| options.ImageFactory = &DefaultImageFactory{} | ||
| } | ||
|
|
||
| if data, err = io.ReadAll(r); err != nil { | ||
| buf := bytes.NewBuffer(options.Buffer) | ||
|
|
||
| if _, err = io.Copy(buf, r); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if len(data) == 0 { | ||
| if len(buf.Bytes()) == 0 { | ||
| return nil, errors.New("data is empty") | ||
| } | ||
|
|
||
| if options == nil { | ||
| options = &Options{} | ||
| } | ||
| d = &Decoder{data: data, options: options} | ||
| d = &Decoder{data: buf.Bytes(), options: options} | ||
|
Comment on lines
+60
to
+70
|
||
|
|
||
| if d.config, err = d.options.GetConfig(); err != nil { | ||
| return nil, err | ||
|
|
@@ -87,10 +92,7 @@ func (d *Decoder) Decode() (image.Image, error) { | |
| d.config.output.colorspace = C.MODE_RGBA | ||
| d.config.output.is_external_memory = 1 | ||
|
|
||
| img := image.NewNRGBA(image.Rectangle{Max: image.Point{ | ||
| X: int(d.config.output.width), | ||
| Y: int(d.config.output.height), | ||
| }}) | ||
| img := d.options.ImageFactory.Get(int(d.config.output.width), int(d.config.output.height)) | ||
|
|
||
| buff := (*C.WebPRGBABuffer)(unsafe.Pointer(&d.config.output.u[0])) | ||
| buff.stride = C.int(img.Stride) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| package decoder | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "image" | ||
| "os" | ||
| "testing" | ||
| ) | ||
|
|
||
| func loadImage(b *testing.B) []byte { | ||
| filename := "../test_data/images/100x150_lossless.webp" | ||
| data, err := os.ReadFile(filename) | ||
| if err != nil { | ||
| b.Fatal(err) | ||
| } | ||
| return data | ||
| } | ||
|
|
||
| func BenchmarkDecodePooled(b *testing.B) { | ||
| b.ResetTimer() | ||
| b.ReportAllocs() | ||
|
|
||
| data := loadImage(b) | ||
|
|
||
| imagePool := NewImagePool() | ||
| bufferPool := NewBufferPool() | ||
|
|
||
| b.RunParallel(func(pb *testing.PB) { | ||
| for pb.Next() { | ||
| buf := bufferPool.Get() | ||
| decoder, err := NewDecoder(bytes.NewReader(data), &Options{ImageFactory: imagePool, Buffer: buf}) | ||
| img, err := decoder.Decode() | ||
|
Comment on lines
+31
to
+32
|
||
| if err != nil { | ||
| b.Fatal(err) | ||
| } | ||
|
|
||
| // put everything back | ||
| imagePool.Put(img.(*image.NRGBA)) | ||
| bufferPool.Put(buf) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func BenchmarkDecodeUnPooled(b *testing.B) { | ||
| b.ResetTimer() | ||
| b.ReportAllocs() | ||
|
|
||
| data := loadImage(b) | ||
|
|
||
| b.RunParallel(func(pb *testing.PB) { | ||
| for pb.Next() { | ||
| decoder, err := NewDecoder(bytes.NewReader(data), &Options{}) | ||
| img, err := decoder.Decode() | ||
|
Comment on lines
+52
to
+53
|
||
| if err != nil { | ||
| b.Fatal(err) | ||
| } | ||
| _ = img | ||
| } | ||
| }) | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,75 @@ | ||||||||||||
| package decoder | ||||||||||||
|
|
||||||||||||
| import ( | ||||||||||||
| "image" | ||||||||||||
| "sync" | ||||||||||||
| "sync/atomic" | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| type ImagePool struct { | ||||||||||||
| poolMap map[int]*sync.Pool | ||||||||||||
| lock *sync.Mutex | ||||||||||||
| Count int64 | ||||||||||||
|
||||||||||||
| 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
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the product of width and height as the pool key can cause incorrect pooling when dimensions differ but have the same product (e.g., 100x150 and 150x100 both equal 15000). This would return images with incorrect dimensions, leading to data corruption or rendering errors. The key should incorporate both dimensions separately, such as using a composite key or encoding both values uniquely.
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lock is released after the pool is retrieved, but before it's used to Get an image. This creates a race condition where the pool's New function might be called concurrently with map modifications. While sync.Pool is safe for concurrent access, the issue is that the New function captures width and height from the first allocation, which might not match subsequent requests with the same dimension product but different width/height combinations.
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.