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

@@ -203,7 +203,7 @@ func (i *Image) DrawImage(img *Image, options *DrawImageOptions) {
 	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}
@@ -327,6 +327,7 @@ func (i *Image) DrawTriangles(vertices []Vertex, indices []uint16, img *Image, o
 
 	filter := driver.Filter(options.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
@@ -566,7 +567,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/atlas/image.go

@@ -301,7 +301,7 @@ func (i *Image) putOnAtlas() error {
 		// If the underlying graphics driver doesn't require restoring from the context lost, just a regular
 		// rendering works.
 		w, h := float32(i.width), float32(i.height)
-		vs := graphics.QuadVertices(0, 0, w, h, 1, 0, 0, 1, 0, 0, 1, 1, 1, 1, false)
+		vs := graphics.QuadVertices(0, 0, w, h, 1, 0, 0, 1, 0, 0, 1, 1, 1, 1)
 		is := graphics.QuadIndices()
 		dr := driver.Region{
 			X:      0,

internal/graphics/vertex.go

@@ -14,6 +14,10 @@
 
 package graphics
 
+import (
+	"sync"
+)
+
 const (
 	ShaderImageNum = 4
 
@@ -50,46 +54,45 @@ 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 on browsers, 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 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)
 
 	// Use the vertex backend instead of calling make to reduce GCs (#1521).
-	vs := theVerticesBackend.slice(4, last)
+	vs := theVerticesBackend.slice(4)
 
 	// This function is very performance-sensitive and implement in a very dumb way.
 	_ = vs[:32]

internal/mipmap/mipmap.go

@@ -187,7 +187,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)
@@ -197,7 +197,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), 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
 	default:
 		panic(fmt.Sprintf("ebiten: invalid level: %d", level))

internal/restorable/image.go

@@ -400,15 +400,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

@@ -861,58 +861,3 @@ func TestClearPixels(t *testing.T) {
 	// After clearing, the regions will be available again.
 	img.ReplacePixels(make([]byte, 4*8*4), 0, 0, 8, 4)
 }
-
-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())
-	dr := driver.Region{
-		X:      0,
-		Y:      0,
-		Width:  w,
-		Height: h,
-	}
-	dst.DrawTriangles([graphics.ShaderImageNum]*Image{src}, [graphics.ShaderImageNum - 1][2]float32{}, vs, is, nil, driver.CompositeModeSourceOver, driver.FilterNearest, driver.AddressUnsafe, dr, 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)
-			}
-		}
-	}
-}