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

This reverts commit 55702a7c28.

Reason: This didn't work with the single-thread mode.

Updates #1704
Closes #2939

internal/atlas/func.go

@@ -1,91 +0,0 @@
-// 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

@@ -60,6 +60,23 @@ func quadVertices(dx0, dy0, dx1, dy1, sx0, sy0, sx1, sy1, cr, cg, cb, ca float32
 	}
 }
 
+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()
+	}
+}
+
 // 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
@@ -203,6 +220,11 @@ var (
 	imagesUsedAsDestination smallImageSet
 
 	graphicsDriverInitialized bool
+
+	deferred []func()
+
+	// deferredM is a mutex for the slice operations. This must not be used for other usages.
+	deferredM sync.Mutex
 )
 
 type ImageType int
@@ -593,30 +615,26 @@ func (i *Image) writePixels(pix []byte, region image.Rectangle) {
 	i.backend.writePixels(pixb, r)
 }
 
-// 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 {
+func (i *Image) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte, region image.Rectangle) (ok bool, err error) {
 	backendsM.Lock()
 	defer backendsM.Unlock()
 
 	if !inFrame {
-		panic("atlas: inFrame must be true in readPixels")
+		// Not ready to read pixels. Try this later.
+		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.image == nil {
 		for i := range pixels {
 			pixels[i] = 0
@@ -835,9 +853,6 @@ 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() {
@@ -890,9 +905,6 @@ 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

@@ -119,9 +119,13 @@ func TestEnsureIsolatedFromSourceBackend(t *testing.T) {
 	}
 
 	pix = make([]byte, 4*size*size)
-	if err := img4.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size)); err != nil {
+	ok, err := img4.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size))
+	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)]
@@ -214,9 +218,13 @@ func TestReputOnSourceBackend(t *testing.T) {
 	atlas.PutImagesOnSourceBackendForTesting(ui.Get().GraphicsDriverForTesting())
 
 	pix = make([]byte, 4*size*size)
-	if err := img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size)); err != nil {
+	ok, err := img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size))
+	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)}
@@ -238,9 +246,13 @@ func TestReputOnSourceBackend(t *testing.T) {
 	}
 
 	pix = make([]byte, 4*size*size)
-	if err := img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size)); err != nil {
+	ok, err = img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size))
+	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)}
@@ -326,9 +338,13 @@ func TestExtend(t *testing.T) {
 	img1.WritePixels(p1, image.Rect(0, 0, w1, h1))
 
 	pix0 := make([]byte, 4*w0*h0)
-	if err := img0.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix0, image.Rect(0, 0, w0, h0)); err != nil {
+	ok, err := img0.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix0, image.Rect(0, 0, w0, h0))
+	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)]
@@ -345,9 +361,13 @@ func TestExtend(t *testing.T) {
 	}
 
 	pix1 := make([]byte, 4*w1*h1)
-	if err := img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix1, image.Rect(0, 0, w1, h1)); err != nil {
+	ok, err = img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix1, image.Rect(0, 0, w1, h1))
+	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)]
@@ -387,9 +407,13 @@ func TestWritePixelsAfterDrawTriangles(t *testing.T) {
 	dst.WritePixels(pix, image.Rect(0, 0, w, h))
 
 	pix = make([]byte, 4*w*h)
-	if err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h)); err != nil {
+	ok, err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h))
+	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)]
@@ -429,9 +453,13 @@ func TestSmallImages(t *testing.T) {
 	dst.DrawTriangles([graphics.ShaderImageCount]*atlas.Image{src}, vs, is, graphicsdriver.BlendSourceOver, dr, [graphics.ShaderImageCount]image.Rectangle{}, atlas.NearestFilterShader, nil, graphicsdriver.FillAll)
 
 	pix = make([]byte, 4*w*h)
-	if err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h)); err != nil {
+	ok, err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h))
+	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)]
@@ -472,9 +500,13 @@ func TestLongImages(t *testing.T) {
 	dst.DrawTriangles([graphics.ShaderImageCount]*atlas.Image{src}, vs, is, graphicsdriver.BlendSourceOver, dr, [graphics.ShaderImageCount]image.Rectangle{}, atlas.NearestFilterShader, nil, graphicsdriver.FillAll)
 
 	pix = make([]byte, 4*dstW*dstH)
-	if err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, dstW, dstH)); err != nil {
+	ok, err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, dstW, dstH))
+	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)]
@@ -683,9 +715,13 @@ func TestImageWritePixelsModify(t *testing.T) {
 
 		// Check the pixels are the original ones.
 		pix = make([]byte, 4*size*size)
-		if err := img.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size)); err != nil {
+		ok, err := img.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size))
+		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

@@ -45,9 +45,13 @@ func TestShaderFillTwice(t *testing.T) {
 	dst.DrawTriangles([graphics.ShaderImageCount]*atlas.Image{}, vs, is, graphicsdriver.BlendCopy, dr, [graphics.ShaderImageCount]image.Rectangle{}, s1, nil, graphicsdriver.FillAll)
 
 	pix := make([]byte, 4*w*h)
-	if err := dst.ReadPixels(g, pix, image.Rect(0, 0, w, h)); err != nil {
+	ok, err := dst.ReadPixels(g, pix, image.Rect(0, 0, w, h))
+	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)
 	}
@@ -72,9 +76,13 @@ func TestImageDrawTwice(t *testing.T) {
 	dst.DrawTriangles([graphics.ShaderImageCount]*atlas.Image{src1}, vs, is, graphicsdriver.BlendCopy, dr, [graphics.ShaderImageCount]image.Rectangle{}, atlas.NearestFilterShader, nil, graphicsdriver.FillAll)
 
 	pix := make([]byte, 4*w*h)
-	if err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h)); err != nil {
+	ok, err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h))
+	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

@@ -67,21 +67,25 @@ func (i *Image) Deallocate() {
 	i.pixelsUnsynced = false
 }
 
-func (i *Image) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte, region image.Rectangle) error {
+func (i *Image) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte, region image.Rectangle) (bool, error) {
 	// Do not call flushDotsBufferIfNeeded here. This would slow (image/draw).Draw.
 	// See ebiten.TestImageDrawOver.
 
 	if region.Dx() == 1 && region.Dy() == 1 {
 		if c, ok := i.dotsBuffer[region.Min]; ok {
 			copy(pixels, c[:])
-			return nil
+			return true, nil
 		}
 	}
 
 	if i.pixels == nil {
 		pix := make([]byte, 4*i.width*i.height)
-		if err := i.img.ReadPixels(graphicsDriver, pix, image.Rect(0, 0, i.width, i.height)); err != nil {
-			return err
+		ok, err := i.img.ReadPixels(graphicsDriver, pix, image.Rect(0, 0, i.width, i.height))
+		if err != nil {
+			return false, err
+		}
+		if !ok {
+			return false, nil
 		}
 		i.pixels = pix
 	}
@@ -105,7 +109,7 @@ func (i *Image) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte
 		copy(pixels[dstX:dstX+lineWidth], i.pixels[srcX:srcX+lineWidth])
 	}
 
-	return nil
+	return true, nil
 }
 
 func (i *Image) DumpScreenshot(graphicsDriver graphicsdriver.Graphics, name string, blackbg bool) (string, error) {

internal/buffered/image_test.go

@@ -38,9 +38,13 @@ func TestUnsyncedPixels(t *testing.T) {
 
 	// Merge the entry into the cached pixels.
 	// The entry for dotsBuffer is now gone in the current implementation.
-	if err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), make([]byte, 4*16*16), image.Rect(0, 0, 16, 16)); err != nil {
+	ok, err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), make([]byte, 4*16*16), image.Rect(0, 0, 16, 16))
+	if err != nil {
 		t.Fatal(err)
 	}
+	if !ok {
+		t.Fatal("ReadPixels failed")
+	}
 
 	// Call WritePixels with the outside region of (0, 0).
 	dst.WritePixels(make([]byte, 4*2*2), image.Rect(1, 1, 3, 3))
@@ -56,9 +60,13 @@ func TestUnsyncedPixels(t *testing.T) {
 
 	// Check the result is correct.
 	var got [4]byte
-	if err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), got[:], image.Rect(0, 0, 1, 1)); err != nil {
+	ok, err = dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), got[:], image.Rect(0, 0, 1, 1))
+	if err != nil {
 		t.Fatal(err)
 	}
+	if !ok {
+		t.Fatal("ReadPixels failed")
+	}
 	want := [4]byte{0xff, 0xff, 0xff, 0xff}
 	if got != want {
 		t.Errorf("got: %v, want: %v", got, want)

internal/mipmap/mipmap.go

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

internal/ui/context.go

@@ -57,11 +57,14 @@ type context struct {
 	lastDrawTime        time.Time
 
 	skipCount int
+
+	funcsInFrameCh chan func()
 }
 
 func newContext(game Game) *context {
 	return &context{
-		game: game,
+		game:           game,
+		funcsInFrameCh: make(chan func()),
 	}
 }
 
@@ -110,6 +113,11 @@ 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
@@ -292,3 +300,32 @@ func (u *UserInterface) LogicalPositionToClientPositionInNativePixels(x, y float
 	y = dipToNativePixels(y, s)
 	return x, y
 }
+
+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

@@ -127,7 +127,35 @@ func newUserInterface() (*UserInterface, error) {
 }
 
 func (u *UserInterface) readPixels(mipmap *mipmap.Mipmap, pixels []byte, region image.Rectangle) error {
-	return mipmap.ReadPixels(u.graphicsDriver, pixels, region)
+	ok, err := 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) {