From fc867d4b0b3650d8dd883970c37585bad5ca5fbc Mon Sep 17 00:00:00 2001 From: Hajime Hoshi Date: Sat, 20 Mar 2021 16:18:50 +0900 Subject: [PATCH] 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 9cb631e30f3cf7471d53ed9aa76afb42846b0918. Closes #1546 --- image.go | 5 +-- internal/graphics/vertex.go | 41 +++++++++++++------------ internal/mipmap/mipmap.go | 8 ++--- internal/restorable/image.go | 7 +++-- internal/restorable/images_test.go | 49 ------------------------------ 5 files changed, 32 insertions(+), 78 deletions(-) diff --git a/image.go b/image.go index ee7a63f5a..4d3c06448 100644 --- a/image.go +++ b/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 diff --git a/internal/graphics/vertex.go b/internal/graphics/vertex.go index e027284d9..354ab44b2 100644 --- a/internal/graphics/vertex.go +++ b/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 diff --git a/internal/mipmap/mipmap.go b/internal/mipmap/mipmap.go index 8f687e2b2..6771b07fd 100644 --- a/internal/mipmap/mipmap.go +++ b/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)) diff --git a/internal/restorable/image.go b/internal/restorable/image.go index 16b0a4216..b3e7e435d 100644 --- a/internal/restorable/image.go +++ b/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, diff --git a/internal/restorable/images_test.go b/internal/restorable/images_test.go index 9366d5db4..b34c9eee6 100644 --- a/internal/restorable/images_test.go +++ b/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) - } - } - } -}