internal/atlas: refactoring: ensure ReadPixels to be processed in a frame

This enables to call (*Image).At from HandleInput, which might be
called outside of a frame.

Updates #1704

internal/atlas/func.go

@@ -0,0 +1,91 @@
+// Copyright 2023 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 atlas
+
+import (
+	"sync"
+)
+
+var (
+	deferred []func()
+
+	// deferredM is a mutex for the slice operations. This must not be used for other usages.
+	deferredM sync.Mutex
+)
+
+func appendDeferred(f func()) {
+	deferredM.Lock()
+	defer deferredM.Unlock()
+	deferred = append(deferred, f)
+}
+
+func flushDeferred() {
+	deferredM.Lock()
+	fs := deferred
+	deferred = nil
+	deferredM.Unlock()
+
+	for _, f := range fs {
+		f()
+	}
+}
+
+type funcsInFrame struct {
+	initOnce      sync.Once
+	funcsCh       chan func()
+	funcsAckCh    chan struct{}
+	beginFrameCh  chan struct{}
+	endFrameCh    chan struct{}
+	endFrameAckCh chan struct{}
+}
+
+var theFuncsInFrame funcsInFrame
+
+func (f *funcsInFrame) beginFrame() {
+	f.initOnce.Do(func() {
+		f.funcsCh = make(chan func())
+		f.funcsAckCh = make(chan struct{})
+		f.beginFrameCh = make(chan struct{})
+		f.endFrameCh = make(chan struct{})
+		f.endFrameAckCh = make(chan struct{})
+		go func() {
+			<-f.beginFrameCh
+			for {
+				select {
+				case fn := <-f.funcsCh:
+					fn()
+					f.funcsAckCh <- struct{}{}
+				case <-f.endFrameCh:
+					f.endFrameAckCh <- struct{}{}
+					// Wait for the next frame.
+					<-f.beginFrameCh
+				}
+			}
+		}()
+	})
+
+	f.beginFrameCh <- struct{}{}
+}
+
+func (f *funcsInFrame) endFrame() {
+	f.endFrameCh <- struct{}{}
+	// Ensure that all the queued functions are consumed and the loop is suspended.
+	<-f.endFrameAckCh
+}
+
+func (f *funcsInFrame) runFuncInFrame(fn func()) {
+	f.funcsCh <- fn
+	<-f.funcsAckCh
+}

internal/atlas/image.go

@@ -48,17 +48,6 @@ func min(a, b int) int {
 	return b
 }
 
-func flushDeferred() {
-	deferredM.Lock()
-	fs := deferred
-	deferred = nil
-	deferredM.Unlock()
-
-	for _, f := range fs {
-		f()
-	}
-}
-
 // baseCountToPutOnSourceBackend represents the base time duration when the image can be put onto an atlas.
 // Actual time duration is increased in an exponential way for each usage as a rendering target.
 const baseCountToPutOnSourceBackend = 10
@@ -132,11 +121,6 @@ var (
 	imagesToPutOnSourceBackend smallImageSet
 
 	imagesUsedAsDestination smallImageSet
-
-	deferred []func()
-
-	// deferredM is a mutex for the slice operations. This must not be used for other usages.
-	deferredM sync.Mutex
 )
 
 type ImageType int
@@ -342,11 +326,9 @@ func (i *Image) DrawTriangles(srcs [graphics.ShaderImageCount]*Image, vertices [
 		us := make([]uint32, len(uniforms))
 		copy(us, uniforms)
 
-		deferredM.Lock()
+		appendDeferred(func() {
-		deferred = append(deferred, func() {
 			i.drawTriangles(srcs, vs, is, blend, dstRegion, srcRegions, shader, us, evenOdd, false)
 		})
-		deferredM.Unlock()
 		return
 	}
 
@@ -463,11 +445,9 @@ func (i *Image) WritePixels(pix []byte, region image.Rectangle) {
 		copied := make([]byte, len(pix))
 		copy(copied, pix)
 
-		deferredM.Lock()
+		appendDeferred(func() {
-		deferred = append(deferred, func() {
 			i.writePixels(copied, region)
 		})
-		deferredM.Unlock()
 		return
 	}
 
@@ -539,26 +519,30 @@ func (i *Image) writePixels(pix []byte, region image.Rectangle) {
 	i.backend.restorable.WritePixels(pixb, r)
 }
 
-func (i *Image) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte, region image.Rectangle) (ok bool, err error) {
+// ReadPixels reads pixels on the given region to the given slice pixels.
+//
+// ReadPixels blocks until BeginFrame is called if necessary in order to ensure this is called in a frame (between BeginFrame and EndFrame).
+// Be careful not to cause a deadlock by blocking a BeginFrame call by this ReadPixels call.
+func (i *Image) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte, region image.Rectangle) error {
+	var err error
+	theFuncsInFrame.runFuncInFrame(func() {
+		err = i.readPixels(graphicsDriver, pixels, region)
+	})
+	return err
+}
+
+func (i *Image) readPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte, region image.Rectangle) error {
 	backendsM.Lock()
 	defer backendsM.Unlock()
 
 	if !inFrame {
-		// Not ready to read pixels. Try this later.
+		panic("atlas: inFrame must be true in readPixels")
-		return false, nil
 	}
 
 	// In the tests, BeginFrame might not be called often and then images might not be disposed (#2292).
 	// To prevent memory leaks, flush the deferred functions here.
 	flushDeferred()
 
-	if err := i.readPixels(graphicsDriver, pixels, region); err != nil {
-		return false, err
-	}
-	return true, nil
-}
-
-func (i *Image) readPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte, region image.Rectangle) error {
 	if i.backend == nil || i.backend.restorable == nil {
 		for i := range pixels {
 			pixels[i] = 0
@@ -577,11 +561,9 @@ func (i *Image) readPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte
 // Defer this operation until it becomes safe. (#913)
 func (i *Image) MarkDisposed() {
 	// As MarkDisposed can be invoked from finalizers, backendsM should not be used.
-	deferredM.Lock()
+	appendDeferred(func() {
-	deferred = append(deferred, func() {
 		i.dispose(true)
 	})
-	deferredM.Unlock()
 }
 
 func (i *Image) dispose(markDisposed bool) {
@@ -757,6 +739,9 @@ func (i *Image) DumpScreenshot(graphicsDriver graphicsdriver.Graphics, path stri
 }
 
 func EndFrame() error {
+	// endFrame must be called outside of backendsM.
+	theFuncsInFrame.endFrame()
+
 	backendsM.Lock()
 	defer backendsM.Unlock()
 	defer func() {
@@ -802,6 +787,9 @@ func floorPowerOf2(x int) int {
 }
 
 func BeginFrame(graphicsDriver graphicsdriver.Graphics) error {
+	// beginFrame must be called outside of backendsM.
+	defer theFuncsInFrame.beginFrame()
+
 	backendsM.Lock()
 	defer backendsM.Unlock()
 

internal/atlas/image_test.go

@@ -117,13 +117,9 @@ func TestEnsureIsolatedFromSourceBackend(t *testing.T) {
 	}
 
 	pix = make([]byte, 4*size*size)
-	ok, err := img4.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size))
+	if err := img4.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size)); err != nil {
-	if err != nil {
 		t.Fatal(err)
 	}
-	if !ok {
-		t.Fatal("ReadPixels failed")
-	}
 	for j := 0; j < size; j++ {
 		for i := 0; i < size; i++ {
 			r := pix[4*(size*j+i)]
@@ -216,13 +212,9 @@ func TestReputOnSourceBackend(t *testing.T) {
 	atlas.PutImagesOnSourceBackendForTesting(ui.Get().GraphicsDriverForTesting())
 
 	pix = make([]byte, 4*size*size)
-	ok, err := img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size))
+	if err := img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size)); err != nil {
-	if err != nil {
 		t.Fatal(err)
 	}
-	if !ok {
-		t.Fatal("ReadPixels failed")
-	}
 	for j := 0; j < size; j++ {
 		for i := 0; i < size; i++ {
 			want := color.RGBA{R: byte(i + j), G: byte(i + j), B: byte(i + j), A: byte(i + j)}
@@ -244,13 +236,9 @@ func TestReputOnSourceBackend(t *testing.T) {
 	}
 
 	pix = make([]byte, 4*size*size)
-	ok, err = img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size))
+	if err := img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size)); err != nil {
-	if err != nil {
 		t.Fatal(err)
 	}
-	if !ok {
-		t.Fatal("ReadPixels failed")
-	}
 	for j := 0; j < size; j++ {
 		for i := 0; i < size; i++ {
 			want := color.RGBA{R: byte(i + j), G: byte(i + j), B: byte(i + j), A: byte(i + j)}
@@ -336,13 +324,9 @@ func TestExtend(t *testing.T) {
 	img1.WritePixels(p1, image.Rect(0, 0, w1, h1))
 
 	pix0 := make([]byte, 4*w0*h0)
-	ok, err := img0.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix0, image.Rect(0, 0, w0, h0))
+	if err := img0.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix0, image.Rect(0, 0, w0, h0)); err != nil {
-	if err != nil {
 		t.Fatal(err)
 	}
-	if !ok {
-		t.Fatal("ReadPixels failed")
-	}
 	for j := 0; j < h0; j++ {
 		for i := 0; i < w0; i++ {
 			r := pix0[4*(w0*j+i)]
@@ -359,13 +343,9 @@ func TestExtend(t *testing.T) {
 	}
 
 	pix1 := make([]byte, 4*w1*h1)
-	ok, err = img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix1, image.Rect(0, 0, w1, h1))
+	if err := img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix1, image.Rect(0, 0, w1, h1)); err != nil {
-	if err != nil {
 		t.Fatal(err)
 	}
-	if !ok {
-		t.Fatal("ReadPixels failed")
-	}
 	for j := 0; j < h1; j++ {
 		for i := 0; i < w1; i++ {
 			r := pix1[4*(w1*j+i)]
@@ -405,13 +385,9 @@ func TestWritePixelsAfterDrawTriangles(t *testing.T) {
 	dst.WritePixels(pix, image.Rect(0, 0, w, h))
 
 	pix = make([]byte, 4*w*h)
-	ok, err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h))
+	if err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h)); err != nil {
-	if err != nil {
 		t.Fatal(err)
 	}
-	if !ok {
-		t.Fatal("ReadPixels failed")
-	}
 	for j := 0; j < h; j++ {
 		for i := 0; i < w; i++ {
 			r := pix[4*(w*j+i)]
@@ -451,13 +427,9 @@ func TestSmallImages(t *testing.T) {
 	dst.DrawTriangles([graphics.ShaderImageCount]*atlas.Image{src}, vs, is, graphicsdriver.BlendSourceOver, dr, [graphics.ShaderImageCount]image.Rectangle{}, atlas.NearestFilterShader, nil, false)
 
 	pix = make([]byte, 4*w*h)
-	ok, err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h))
+	if err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h)); err != nil {
-	if err != nil {
 		t.Fatal(err)
 	}
-	if !ok {
-		t.Fatal("ReadPixels failed")
-	}
 	for j := 0; j < h; j++ {
 		for i := 0; i < w; i++ {
 			r := pix[4*(w*j+i)]
@@ -498,13 +470,9 @@ func TestLongImages(t *testing.T) {
 	dst.DrawTriangles([graphics.ShaderImageCount]*atlas.Image{src}, vs, is, graphicsdriver.BlendSourceOver, dr, [graphics.ShaderImageCount]image.Rectangle{}, atlas.NearestFilterShader, nil, false)
 
 	pix = make([]byte, 4*dstW*dstH)
-	ok, err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, dstW, dstH))
+	if err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, dstW, dstH)); err != nil {
-	if err != nil {
 		t.Fatal(err)
 	}
-	if !ok {
-		t.Fatal("ReadPixels failed")
-	}
 	for j := 0; j < h; j++ {
 		for i := 0; i < w*scale; i++ {
 			r := pix[4*(dstW*j+i)]
@@ -716,13 +684,9 @@ func TestImageWritePixelsModify(t *testing.T) {
 
 		// Check the pixels are the original ones.
 		pix = make([]byte, 4*size*size)
-		ok, err := img.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size))
+		if err := img.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size)); err != nil {
-		if err != nil {
 			t.Fatal(err)
 		}
-		if !ok {
-			t.Fatal("ReadPixels failed")
-		}
 		for j := 0; j < size; j++ {
 			for i := 0; i < size; i++ {
 				want := color.RGBA{R: byte(i + j), G: byte(i + j), B: byte(i + j), A: byte(i + j)}

internal/atlas/shader_test.go

@@ -44,13 +44,9 @@ func TestShaderFillTwice(t *testing.T) {
 	dst.DrawTriangles([graphics.ShaderImageCount]*atlas.Image{}, vs, is, graphicsdriver.BlendCopy, dr, [graphics.ShaderImageCount]image.Rectangle{}, s1, nil, false)
 
 	pix := make([]byte, 4*w*h)
-	ok, err := dst.ReadPixels(g, pix, image.Rect(0, 0, w, h))
+	if err := dst.ReadPixels(g, pix, image.Rect(0, 0, w, h)); err != nil {
-	if err != nil {
 		t.Error(err)
 	}
-	if !ok {
-		t.Fatal("ReadPixels failed")
-	}
 	if got, want := (color.RGBA{R: pix[0], G: pix[1], B: pix[2], A: pix[3]}), (color.RGBA{R: 0x80, G: 0x80, B: 0x80, A: 0xff}); got != want {
 		t.Errorf("got: %v, want: %v", got, want)
 	}
@@ -75,13 +71,9 @@ func TestImageDrawTwice(t *testing.T) {
 	dst.DrawTriangles([graphics.ShaderImageCount]*atlas.Image{src1}, vs, is, graphicsdriver.BlendCopy, dr, [graphics.ShaderImageCount]image.Rectangle{}, atlas.NearestFilterShader, nil, false)
 
 	pix := make([]byte, 4*w*h)
-	ok, err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h))
+	if err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h)); err != nil {
-	if err != nil {
 		t.Error(err)
 	}
-	if !ok {
-		t.Fatal("ReadPixels failed")
-	}
 	if got, want := (color.RGBA{R: pix[0], G: pix[1], B: pix[2], A: pix[3]}), (color.RGBA{R: 0x80, G: 0x80, B: 0x80, A: 0xff}); got != want {
 		t.Errorf("got: %v, want: %v", got, want)
 	}

internal/buffered/image.go

@@ -49,24 +49,19 @@ func (i *Image) MarkDisposed() {
 	i.img.MarkDisposed()
 }
 
-func (i *Image) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte, region image.Rectangle) (ok bool, err error) {
+func (i *Image) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte, region image.Rectangle) error {
 	// If restorable.AlwaysReadPixelsFromGPU() returns false, the pixel data is cached in the restorable package.
 	if !restorable.AlwaysReadPixelsFromGPU() {
-		ok, err := i.img.ReadPixels(graphicsDriver, pixels, region)
+		if err := i.img.ReadPixels(graphicsDriver, pixels, region); err != nil {
-		if err != nil {
+			return err
-			return false, err
 		}
-		return ok, nil
+		return nil
 	}
 
 	if i.pixels == nil {
 		pix := make([]byte, 4*i.width*i.height)
-		ok, err := i.img.ReadPixels(graphicsDriver, pix, image.Rect(0, 0, i.width, i.height))
+		if err := i.img.ReadPixels(graphicsDriver, pix, image.Rect(0, 0, i.width, i.height)); err != nil {
-		if err != nil {
+			return err
-			return false, err
-		}
-		if !ok {
-			return false, nil
 		}
 		i.pixels = pix
 	}
@@ -77,7 +72,7 @@ func (i *Image) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte
 		srcX := 4 * ((region.Min.Y+j)*i.width + region.Min.X)
 		copy(pixels[dstX:dstX+lineWidth], i.pixels[srcX:srcX+lineWidth])
 	}
-	return true, nil
+	return nil
 }
 
 func (i *Image) DumpScreenshot(graphicsDriver graphicsdriver.Graphics, name string, blackbg bool) (string, error) {

internal/mipmap/mipmap.go

@@ -61,7 +61,7 @@ func (m *Mipmap) WritePixels(pix []byte, region image.Rectangle) {
 	m.disposeMipmaps()
 }
 
-func (m *Mipmap) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte, region image.Rectangle) (ok bool, err error) {
+func (m *Mipmap) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte, region image.Rectangle) error {
 	return m.orig.ReadPixels(graphicsDriver, pixels, region)
 }
 

internal/ui/context.go

@@ -58,14 +58,11 @@ type context struct {
 	skipCount int
 
 	setContextOnce sync.Once
-
-	funcsInFrameCh chan func()
 }
 
 func newContext(game Game) *context {
 	return &context{
-		game:           game,
+		game: game,
-		funcsInFrameCh: make(chan func()),
 	}
 }
 
@@ -117,11 +114,6 @@ func (c *context) updateFrameImpl(graphicsDriver graphicsdriver.Graphics, update
 		}
 	}()
 
-	// Flush deferred functions, like reading pixels from GPU.
-	if err := c.processFuncsInFrame(ui); err != nil {
-		return err
-	}
-
 	// ForceUpdate can be invoked even if the context is not initialized yet (#1591).
 	if w, h := c.layoutGame(outsideWidth, outsideHeight, deviceScaleFactor); w == 0 || h == 0 {
 		return nil
@@ -296,32 +288,3 @@ func (c *context) screenScaleAndOffsets() (scale, offsetX, offsetY float64) {
 func (u *UserInterface) LogicalPositionToClientPosition(x, y float64) (float64, float64) {
 	return u.context.logicalPositionToClientPosition(x, y, u.DeviceScaleFactor())
 }
-
-func (c *context) runInFrame(f func()) {
-	ch := make(chan struct{})
-	c.funcsInFrameCh <- func() {
-		defer close(ch)
-		f()
-	}
-	<-ch
-	return
-}
-
-func (c *context) processFuncsInFrame(ui *UserInterface) error {
-	var processed bool
-	for {
-		select {
-		case f := <-c.funcsInFrameCh:
-			f()
-			processed = true
-		default:
-			if processed {
-				// Catch the error that happened at (*Image).At.
-				if err := ui.error(); err != nil {
-					return err
-				}
-			}
-			return nil
-		}
-	}
-}

internal/ui/ui.go

@@ -120,35 +120,7 @@ func newUserInterface() (*UserInterface, error) {
 }
 
 func (u *UserInterface) readPixels(mipmap *mipmap.Mipmap, pixels []byte, region image.Rectangle) error {
-	ok, err := mipmap.ReadPixels(u.graphicsDriver, pixels, region)
+	return mipmap.ReadPixels(u.graphicsDriver, pixels, region)
-	if err != nil {
-		return err
-	}
-
-	// ReadPixels failed since this was called in between two frames.
-	// Try this again at the next frame.
-	if !ok {
-		// If this function is called from the same sequence as a game's Update and Draw,
-		// this causes a dead lock.
-		// This never happens so far, but if handling inputs after EndFrame is implemented,
-		// this might be possible (#1704).
-
-		var err1 error
-		u.context.runInFrame(func() {
-			ok, err := mipmap.ReadPixels(u.graphicsDriver, pixels, region)
-			if err != nil {
-				err1 = err
-				return
-			}
-			if !ok {
-				// This never reaches since this function must be called in a frame.
-				panic("ui: ReadPixels unexpectedly failed")
-			}
-		})
-		return err1
-	}
-
-	return nil
 }
 
 func (u *UserInterface) dumpScreenshot(mipmap *mipmap.Mipmap, name string, blackbg bool) (string, error) {