graphics: Bug fix: the flag and the enqueueing operation must be protected by a same mutex

It was theoretically possible that an item was enqueued even
though the flag said it should not.
This commit is contained in:
Hajime Hoshi 2019-08-25 20:57:49 +09:00
parent fc939fabb8
commit 3646e7930d
3 changed files with 54 additions and 28 deletions

View File

@ -20,7 +20,6 @@ import (
"image/color" "image/color"
"math" "math"
"sync" "sync"
"sync/atomic"
"github.com/hajimehoshi/ebiten/internal/driver" "github.com/hajimehoshi/ebiten/internal/driver"
"github.com/hajimehoshi/ebiten/internal/graphics" "github.com/hajimehoshi/ebiten/internal/graphics"
@ -31,21 +30,27 @@ var (
// imageQueue represents a queue for image operations that are ordered before the game starts (BeginFrame). // imageQueue represents a queue for image operations that are ordered before the game starts (BeginFrame).
// Before the game starts, the package shareable doesn't determine the minimum/maximum texture sizes (#879). // Before the game starts, the package shareable doesn't determine the minimum/maximum texture sizes (#879).
// Instead of accessing the package shareable, defer the image operations until the game starts (#921). // Instead of accessing the package shareable, defer the image operations until the game starts (#921).
imageQueue []func() imageQueue []func()
imageQueueM sync.Mutex imageQueueM sync.Mutex
needsEnqueueImageOps = true
) )
func enqueueImageOp(f func()) { func flushImageOpsIfNeeded() {
imageQueueM.Lock() imageQueueM.Lock()
defer imageQueueM.Unlock()
imageQueue = append(imageQueue, f) if !needsEnqueueImageOps {
} if len(imageQueue) > 0 {
panic("ebiten: len(imageQueue) must be 0 after the game starts")
}
imageQueueM.Unlock()
return
}
func flushImageOps() { // Set this flag false first, or the image operations will be queued again.
imageQueueM.Lock() needsEnqueueImageOps = false
defer imageQueueM.Unlock() imageQueueM.Unlock()
// As a new item will not be enqueued any longer, mutex does not have to, or should not be used.
for _, f := range imageQueue { for _, f := range imageQueue {
f() f()
} }
@ -112,9 +117,10 @@ func (i *Image) Clear() error {
func (i *Image) Fill(clr color.Color) error { func (i *Image) Fill(clr color.Color) error {
i.copyCheck() i.copyCheck()
if atomic.LoadInt32(&isImageAvailable) == 0 { imageQueueM.Lock()
if needsEnqueueImageOps {
r, g, b, a := clr.RGBA() r, g, b, a := clr.RGBA()
enqueueImageOp(func() { imageQueue = append(imageQueue, func() {
i.Fill(color.RGBA64{ i.Fill(color.RGBA64{
R: uint16(r), R: uint16(r),
G: uint16(g), G: uint16(g),
@ -122,8 +128,10 @@ func (i *Image) Fill(clr color.Color) error {
A: uint16(a), A: uint16(a),
}) })
}) })
imageQueueM.Unlock()
return nil return nil
} }
imageQueueM.Unlock()
if i.isDisposed() { if i.isDisposed() {
return nil return nil
@ -188,13 +196,16 @@ func (i *Image) disposeMipmaps() {
func (i *Image) DrawImage(img *Image, options *DrawImageOptions) error { func (i *Image) DrawImage(img *Image, options *DrawImageOptions) error {
i.copyCheck() i.copyCheck()
if atomic.LoadInt32(&isImageAvailable) == 0 { imageQueueM.Lock()
if needsEnqueueImageOps {
op := *options op := *options
enqueueImageOp(func() { imageQueue = append(imageQueue, func() {
i.DrawImage(img, &op) i.DrawImage(img, &op)
}) })
imageQueueM.Unlock()
return nil return nil
} }
imageQueueM.Unlock()
if img.isDisposed() { if img.isDisposed() {
panic("ebiten: the given image to DrawImage must not be disposed") panic("ebiten: the given image to DrawImage must not be disposed")
@ -407,17 +418,20 @@ const MaxIndicesNum = graphics.IndicesNum
func (i *Image) DrawTriangles(vertices []Vertex, indices []uint16, img *Image, options *DrawTrianglesOptions) { func (i *Image) DrawTriangles(vertices []Vertex, indices []uint16, img *Image, options *DrawTrianglesOptions) {
i.copyCheck() i.copyCheck()
if atomic.LoadInt32(&isImageAvailable) == 0 { imageQueueM.Lock()
if needsEnqueueImageOps {
vs := make([]Vertex, len(vertices)) vs := make([]Vertex, len(vertices))
copy(vs, vertices) copy(vs, vertices)
is := make([]uint16, len(indices)) is := make([]uint16, len(indices))
copy(is, indices) copy(is, indices)
op := *options op := *options
enqueueImageOp(func() { imageQueue = append(imageQueue, func() {
i.DrawTriangles(vs, is, img, &op) i.DrawTriangles(vs, is, img, &op)
}) })
imageQueueM.Unlock()
return return
} }
imageQueueM.Unlock()
if i.isDisposed() { if i.isDisposed() {
return return
@ -528,7 +542,10 @@ func (i *Image) ColorModel() color.Model {
// //
// At can't be called outside the main loop (ebiten.Run's updating function) starts (as of version 1.4.0-alpha). // At can't be called outside the main loop (ebiten.Run's updating function) starts (as of version 1.4.0-alpha).
func (i *Image) At(x, y int) color.Color { func (i *Image) At(x, y int) color.Color {
if atomic.LoadInt32(&isImageAvailable) == 0 { imageQueueM.Lock()
n := needsEnqueueImageOps
imageQueueM.Unlock()
if n {
panic("ebiten: (*Image).At is not available outside the main loop so far") panic("ebiten: (*Image).At is not available outside the main loop so far")
} }
@ -551,7 +568,10 @@ func (i *Image) At(x, y int) color.Color {
// //
// If the image is disposed, Set does nothing. // If the image is disposed, Set does nothing.
func (img *Image) Set(x, y int, clr color.Color) { func (img *Image) Set(x, y int, clr color.Color) {
if atomic.LoadInt32(&isImageAvailable) == 0 { imageQueueM.Lock()
n := needsEnqueueImageOps
imageQueueM.Unlock()
if n {
panic("ebiten: (*Image).Set is not available outside the main loop so far") panic("ebiten: (*Image).Set is not available outside the main loop so far")
} }
@ -619,12 +639,15 @@ func (i *Image) resolvePendingPixels(draw bool) {
func (i *Image) Dispose() error { func (i *Image) Dispose() error {
i.copyCheck() i.copyCheck()
if atomic.LoadInt32(&isImageAvailable) == 0 { imageQueueM.Lock()
enqueueImageOp(func() { if needsEnqueueImageOps {
imageQueue = append(imageQueue, func() {
i.Dispose() i.Dispose()
}) })
imageQueueM.Unlock()
return nil return nil
} }
imageQueueM.Unlock()
if i.isDisposed() { if i.isDisposed() {
return nil return nil
@ -651,14 +674,17 @@ func (i *Image) Dispose() error {
func (i *Image) ReplacePixels(p []byte) error { func (i *Image) ReplacePixels(p []byte) error {
i.copyCheck() i.copyCheck()
if atomic.LoadInt32(&isImageAvailable) == 0 { imageQueueM.Lock()
if needsEnqueueImageOps {
px := make([]byte, len(p)) px := make([]byte, len(p))
copy(px, p) copy(px, p)
enqueueImageOp(func() { imageQueue = append(imageQueue, func() {
i.ReplacePixels(px) i.ReplacePixels(px)
}) })
imageQueueM.Unlock()
return nil return nil
} }
imageQueueM.Unlock()
if i.isDisposed() { if i.isDisposed() {
return nil return nil
@ -743,12 +769,15 @@ func NewImage(width, height int, filter Filter) (*Image, error) {
// //
// When the image is disposed, makeVolatile does nothing. // When the image is disposed, makeVolatile does nothing.
func (i *Image) makeVolatile() { func (i *Image) makeVolatile() {
if atomic.LoadInt32(&isImageAvailable) == 0 { imageQueueM.Lock()
enqueueImageOp(func() { if needsEnqueueImageOps {
imageQueue = append(imageQueue, func() {
i.makeVolatile() i.makeVolatile()
}) })
imageQueueM.Unlock()
return return
} }
imageQueueM.Unlock()
if i.isDisposed() { if i.isDisposed() {
return return

1
run.go
View File

@ -46,7 +46,6 @@ func CurrentFPS() float64 {
var ( var (
isDrawingSkipped = int32(0) isDrawingSkipped = int32(0)
currentMaxTPS = int32(DefaultTPS) currentMaxTPS = int32(DefaultTPS)
isImageAvailable = int32(0)
) )
func setDrawingSkipped(skipped bool) { func setDrawingSkipped(skipped bool) {

View File

@ -17,7 +17,6 @@ package ebiten
import ( import (
"fmt" "fmt"
"math" "math"
"sync/atomic"
"github.com/hajimehoshi/ebiten/internal/clock" "github.com/hajimehoshi/ebiten/internal/clock"
"github.com/hajimehoshi/ebiten/internal/driver" "github.com/hajimehoshi/ebiten/internal/driver"
@ -85,8 +84,7 @@ func (c *uiContext) Update(afterFrameUpdate func()) error {
} }
// Images are available after shareable is initialized. // Images are available after shareable is initialized.
atomic.StoreInt32(&isImageAvailable, 1) flushImageOpsIfNeeded()
flushImageOps()
for i := 0; i < updateCount; i++ { for i := 0; i < updateCount; i++ {
c.offscreen.Clear() c.offscreen.Clear()