From 71b7cedc5b9907f4b0e8838e71688c9895d0997c Mon Sep 17 00:00:00 2001
From: Hajime Hoshi <hajimehoshi@gmail.com>
Date: Fri, 12 Jul 2024 10:53:54 +0900
Subject: [PATCH] internal/graphicscommand: bug fix: buffered write pixel args
 might never be released

Closes #3036
---
 internal/graphics/bytes.go              |  9 +++++
 internal/graphicscommand/export_test.go | 21 ++++++++++
 internal/graphicscommand/image.go       | 18 ++++++++-
 internal/graphicscommand/image_test.go  | 51 +++++++++++++++++++++++++
 4 files changed, 98 insertions(+), 1 deletion(-)
 create mode 100644 internal/graphicscommand/export_test.go

diff --git a/internal/graphics/bytes.go b/internal/graphics/bytes.go
index a92e299ac..6bd9a6b2b 100644
--- a/internal/graphics/bytes.go
+++ b/internal/graphics/bytes.go
@@ -59,6 +59,15 @@ func (m *ManagedBytes) GetAndRelease() ([]byte, func()) {
 	}
 }
 
+// Release releases the underlying byte slice.
+//
+// After Release is called, the underlying byte slice is no longer available.
+func (m *ManagedBytes) Release() {
+	m.pool.put(m.bytes)
+	m.bytes = nil
+	runtime.SetFinalizer(m, nil)
+}
+
 // NewManagedBytes returns a managed byte slice initialized by the given constructor f.
 //
 // The byte slice is not zero-cleared at the constructor.
diff --git a/internal/graphicscommand/export_test.go b/internal/graphicscommand/export_test.go
new file mode 100644
index 000000000..016c4569f
--- /dev/null
+++ b/internal/graphicscommand/export_test.go
@@ -0,0 +1,21 @@
+// Copyright 2024 The Ebitengine Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package graphicscommand
+
+type WritePixelsCommandArgs = writePixelsCommandArgs
+
+func (i *Image) BufferedWritePixelsArgsForTesting() []WritePixelsCommandArgs {
+	return i.bufferedWritePixelsArgs
+}
diff --git a/internal/graphicscommand/image.go b/internal/graphicscommand/image.go
index 64ff0abbd..4daa6cbc3 100644
--- a/internal/graphicscommand/image.go
+++ b/internal/graphicscommand/image.go
@@ -161,7 +161,23 @@ func (i *Image) ReadPixels(graphicsDriver graphicsdriver.Graphics, args []graphi
 }
 
 func (i *Image) WritePixels(pixels *graphics.ManagedBytes, region image.Rectangle) {
-	i.bufferedWritePixelsArgs = append(i.bufferedWritePixelsArgs, writePixelsCommandArgs{
+	// Release the previous pixels if the region is included by the new region.
+	// Successive WritePixels calls might accumulate the pixels and never release,
+	// especially when the image is unmanaged (#3036).
+	var cur int
+	for idx := 0; idx < len(i.bufferedWritePixelsArgs); idx++ {
+		arg := i.bufferedWritePixelsArgs[idx]
+		if arg.region.In(region) {
+			arg.pixels.Release()
+			continue
+		}
+		i.bufferedWritePixelsArgs[cur] = arg
+		cur++
+	}
+	for idx := cur; idx < len(i.bufferedWritePixelsArgs); idx++ {
+		i.bufferedWritePixelsArgs[idx] = writePixelsCommandArgs{}
+	}
+	i.bufferedWritePixelsArgs = append(i.bufferedWritePixelsArgs[:cur], writePixelsCommandArgs{
 		pixels: pixels,
 		region: region,
 	})
diff --git a/internal/graphicscommand/image_test.go b/internal/graphicscommand/image_test.go
index e8d08e032..9840a68be 100644
--- a/internal/graphicscommand/image_test.go
+++ b/internal/graphicscommand/image_test.go
@@ -135,3 +135,54 @@ func TestShader(t *testing.T) {
 		}
 	}
 }
+
+// Issue #3036
+func TestSuccessiveWritePixels(t *testing.T) {
+	const w, h = 32, 32
+	dst := graphicscommand.NewImage(w, h, false)
+
+	dst.WritePixels(graphics.NewManagedBytes(4, func(bs []byte) {
+		for i := range bs {
+			bs[i] = 0
+		}
+	}), image.Rect(0, 0, 1, 1))
+	if got, want := len(dst.BufferedWritePixelsArgsForTesting()), 1; got != want {
+		t.Errorf("len(dst.BufferedWritePixelsArgsForTesting()): got %d, want: %d", got, want)
+	}
+
+	dst.WritePixels(graphics.NewManagedBytes(4, func(bs []byte) {
+		for i := range bs {
+			bs[i] = 0
+		}
+	}), image.Rect(1, 1, 2, 2))
+	if got, want := len(dst.BufferedWritePixelsArgsForTesting()), 2; got != want {
+		t.Errorf("len(dst.BufferedWritePixelsArgsForTesting()): got %d, want: %d", got, want)
+	}
+
+	dst.WritePixels(graphics.NewManagedBytes(4, func(bs []byte) {
+		for i := range bs {
+			bs[i] = 0
+		}
+	}), image.Rect(0, 0, 1, 1))
+	if got, want := len(dst.BufferedWritePixelsArgsForTesting()), 2; got != want {
+		t.Errorf("len(dst.BufferedWritePixelsArgsForTesting()): got %d, want: %d", got, want)
+	}
+
+	dst.WritePixels(graphics.NewManagedBytes(4, func(bs []byte) {
+		for i := range bs {
+			bs[i] = 0
+		}
+	}), image.Rect(0, 0, 1, 1))
+	if got, want := len(dst.BufferedWritePixelsArgsForTesting()), 2; got != want {
+		t.Errorf("len(dst.BufferedWritePixelsArgsForTesting()): got %d, want: %d", got, want)
+	}
+
+	dst.WritePixels(graphics.NewManagedBytes(4, func(bs []byte) {
+		for i := range bs {
+			bs[i] = 0
+		}
+	}), image.Rect(0, 0, 2, 2))
+	if got, want := len(dst.BufferedWritePixelsArgsForTesting()), 1; got != want {
+		t.Errorf("len(dst.BufferedWritePixelsArgsForTesting()): got %d, want: %d", got, want)
+	}
+}