internal/graphics: Bug fix: Race condition at QuadVertices

QuadVertices or verticesBackend.slice reused its backend slice.
This caused a race condition. QuadVertices can be accessed from
multiple goroutines, and resetting the head and copying the data
at internal/graphicscommand might not be synced.

This change fixes this issue by basically reverting
9cb631e30f.

Closes #1546

image.go

@@ -256,7 +256,7 @@ func (i *Image) DrawImage(img *Image, options *DrawImageOptions) error {
 	sy0 := float32(bounds.Min.Y)
 	sx1 := float32(bounds.Max.X)
 	sy1 := float32(bounds.Max.Y)
-	vs := graphics.QuadVertices(sx0, sy0, sx1, sy1, a, b, c, d, tx, ty, 1, 1, 1, 1, i.screen)
+	vs := graphics.QuadVertices(sx0, sy0, sx1, sy1, a, b, c, d, tx, ty, 1, 1, 1, 1)
 	is := graphics.QuadIndices()
 
 	srcs := [graphics.ShaderImageNum]*mipmap.Mipmap{img.mipmap}
@@ -382,6 +382,7 @@ func (i *Image) DrawTriangles(vertices []Vertex, indices []uint16, img *Image, o
 		filter = driver.Filter(img.filter)
 	}
 
+	// TODO: Use the same backend as graphics.QuadVertices.
 	vs := make([]float32, len(vertices)*graphics.VertexFloatNum)
 	for i, v := range vertices {
 		vs[i*graphics.VertexFloatNum] = v.DstX
@@ -614,7 +615,7 @@ func (i *Image) DrawRectShader(width, height int, shader *Shader, options *DrawR
 	}
 
 	a, b, c, d, tx, ty := options.GeoM.elements32()
-	vs := graphics.QuadVertices(sx, sy, sx+float32(width), sy+float32(height), a, b, c, d, tx, ty, 1, 1, 1, 1, false)
+	vs := graphics.QuadVertices(sx, sy, sx+float32(width), sy+float32(height), a, b, c, d, tx, ty, 1, 1, 1, 1)
 	is := graphics.QuadIndices()
 
 	var sr driver.Region

internal/graphics/vertex.go

@@ -15,6 +15,8 @@
 package graphics
 
 import (
+	"sync"
+
 	"github.com/hajimehoshi/ebiten/internal/web"
 )
 
@@ -50,54 +52,53 @@ func QuadIndices() []uint16 {
 }
 
 var (
-	theVerticesBackend = &verticesBackend{
-		backend: make([]float32, VertexFloatNum*1024),
-	}
+	theVerticesBackend = &verticesBackend{}
 )
 
 type verticesBackend struct {
 	backend []float32
 	head    int
+	m       sync.Mutex
 }
 
-func (v *verticesBackend) slice(n int, last bool) []float32 {
-	// As this is called only from GopherJS, mutex is not required.
+func (v *verticesBackend) slice(n int) []float32 {
+	v.m.Lock()
+	defer v.m.Unlock()
 
 	need := n * VertexFloatNum
-	if l := len(v.backend); v.head+need > l {
-		for v.head+need > l {
-			l *= 2
-		}
-		v.backend = make([]float32, l)
+	if v.head+need > len(v.backend) {
+		v.backend = nil
 		v.head = 0
 	}
+	if v.backend == nil {
+		l := 1024
+		if n > l {
+			l = n
+		}
+		v.backend = make([]float32, VertexFloatNum*l)
+	}
 
 	s := v.backend[v.head : v.head+need]
-	if last {
-		// If last is true, the vertices backend is sent to GPU and it is fine to reuse the slice.
-		v.head = 0
-	} else {
-		v.head += need
-	}
+	v.head += need
 	return s
 }
 
-func vertexSlice(n int, last bool) []float32 {
+func vertexSlice(n int) []float32 {
 	if web.IsBrowser() {
 		// In GopherJS and Wasm, allocating memory by make is expensive. Use the backend instead.
-		return theVerticesBackend.slice(n, last)
+		return theVerticesBackend.slice(n)
 	}
 	return make([]float32, n*VertexFloatNum)
 }
 
-func QuadVertices(sx0, sy0, sx1, sy1 float32, a, b, c, d, tx, ty float32, cr, cg, cb, ca float32, last bool) []float32 {
+func QuadVertices(sx0, sy0, sx1, sy1 float32, a, b, c, d, tx, ty float32, cr, cg, cb, ca float32) []float32 {
 	x := sx1 - sx0
 	y := sy1 - sy0
 	ax, by, cx, dy := a*x, b*y, c*x, d*y
 	u0, v0, u1, v1 := float32(sx0), float32(sy0), float32(sx1), float32(sy1)
 
 	// This function is very performance-sensitive and implement in a very dumb way.
-	vs := vertexSlice(4, last)
+	vs := vertexSlice(4)
 	_ = vs[:32]
 
 	vs[0] = tx

internal/mipmap/mipmap.go

@@ -199,7 +199,7 @@ func (m *Mipmap) level(level int) *buffered.Image {
 	switch {
 	case level == 1:
 		src = m.orig
-		vs = graphics.QuadVertices(0, 0, float32(m.width), float32(m.height), 0.5, 0, 0, 0.5, 0, 0, 1, 1, 1, 1, false)
+		vs = graphics.QuadVertices(0, 0, float32(m.width), float32(m.height), 0.5, 0, 0, 0.5, 0, 0, 1, 1, 1, 1)
 		filter = driver.FilterLinear
 	case level > 1:
 		src = m.level(level - 1)
@@ -209,11 +209,11 @@ func (m *Mipmap) level(level int) *buffered.Image {
 		}
 		w := sizeForLevel(m.width, level-1)
 		h := sizeForLevel(m.height, level-1)
-		vs = graphics.QuadVertices(0, 0, float32(w), float32(h), 0.5, 0, 0, 0.5, 0, 0, 1, 1, 1, 1, false)
+		vs = graphics.QuadVertices(0, 0, float32(w), float32(h), 0.5, 0, 0, 0.5, 0, 0, 1, 1, 1, 1)
 		filter = driver.FilterLinear
 	case level == -1:
 		src = m.orig
-		vs = graphics.QuadVertices(0, 0, float32(m.width), float32(m.height), 2, 0, 0, 2, 0, 0, 1, 1, 1, 1, false)
+		vs = graphics.QuadVertices(0, 0, float32(m.width), float32(m.height), 2, 0, 0, 2, 0, 0, 1, 1, 1, 1)
 		filter = driver.FilterNearest
 	case level < -1:
 		src = m.level(level + 1)
@@ -223,7 +223,7 @@ func (m *Mipmap) level(level int) *buffered.Image {
 		}
 		w := sizeForLevel(m.width, level-1)
 		h := sizeForLevel(m.height, level-1)
-		vs = graphics.QuadVertices(0, 0, float32(w), float32(h), 2, 0, 0, 2, 0, 0, 1, 1, 1, 1, false)
+		vs = graphics.QuadVertices(0, 0, float32(w), float32(h), 2, 0, 0, 2, 0, 0, 1, 1, 1, 1)
 		filter = driver.FilterNearest
 	default:
 		panic(fmt.Sprintf("ebiten: invalid level: %d", level))

internal/restorable/image.go

@@ -421,15 +421,16 @@ func (i *Image) appendDrawTrianglesHistory(srcs [graphics.ShaderImageNum]*Image,
 	// All images must be resolved and not stale each after frame.
 	// So we don't have to care if image is stale or not here.
 
-	vs := make([]float32, len(vertices))
-	copy(vs, vertices)
+	// vertices is generated at ebiten package and doesn't have to be copied so far.
+	// This depends on the implementation of graphics.QuadVertices.
+
 	is := make([]uint16, len(indices))
 	copy(is, indices)
 
 	item := &drawTrianglesHistoryItem{
 		images:       srcs,
 		offsets:      offsets,
-		vertices:     vs,
+		vertices:     vertices,
 		indices:      is,
 		colorm:       colorm,
 		mode:         mode,

internal/restorable/images_test.go

@@ -852,52 +852,3 @@ func TestFill2(t *testing.T) {
 		}
 	}
 }
-
-func TestMutateSlices(t *testing.T) {
-	const w, h = 16, 16
-	dst := NewImage(w, h)
-	src := NewImage(w, h)
-	pix := make([]byte, 4*w*h)
-	for i := 0; i < w*h; i++ {
-		pix[4*i] = byte(i)
-		pix[4*i+1] = byte(i)
-		pix[4*i+2] = byte(i)
-		pix[4*i+3] = 0xff
-	}
-	src.ReplacePixels(pix, 0, 0, w, h)
-
-	vs := quadVertices(w, h, 0, 0)
-	is := make([]uint16, len(graphics.QuadIndices()))
-	copy(is, graphics.QuadIndices())
-	dst.DrawTriangles([graphics.ShaderImageNum]*Image{src}, [graphics.ShaderImageNum - 1][2]float32{}, vs, is, nil, driver.CompositeModeSourceOver, driver.FilterNearest, driver.AddressUnsafe, driver.Region{}, nil, nil)
-	for i := range vs {
-		vs[i] = 0
-	}
-	for i := range is {
-		is[i] = 0
-	}
-	if err := ResolveStaleImages(); err != nil {
-		t.Fatal(err)
-	}
-	if err := RestoreIfNeeded(); err != nil {
-		t.Fatal(err)
-	}
-
-	for j := 0; j < h; j++ {
-		for i := 0; i < w; i++ {
-			r, g, b, a, err := src.At(i, j)
-			if err != nil {
-				t.Fatal(err)
-			}
-			want := color.RGBA{r, g, b, a}
-			r, g, b, a, err = dst.At(i, j)
-			if err != nil {
-				t.Fatal(err)
-			}
-			got := color.RGBA{r, g, b, a}
-			if !sameColors(got, want, 1) {
-				t.Errorf("(%d, %d): got %v, want %v", i, j, got, want)
-			}
-		}
-	}
-}