From ad51e22252f089d00f9d250183f84ec8fe82574c Mon Sep 17 00:00:00 2001 From: Hajime Hoshi Date: Thu, 19 Oct 2023 03:56:24 +0900 Subject: [PATCH] internal/atlas: bug fix: ReadPixels crashed when inFrame was false This change fixes the issue by letting ReadPixels return a channel and executing this asynchronously when needed. Closes #2815 --- internal/atlas/image.go | 16 ++++- internal/atlas/image_test.go | 18 +++--- internal/atlas/shader_test.go | 4 +- internal/buffered/image.go | 9 ++- internal/processtest/testdata/issue2815.go | 72 ++++++++++++++++++++++ 5 files changed, 104 insertions(+), 15 deletions(-) create mode 100644 internal/processtest/testdata/issue2815.go diff --git a/internal/atlas/image.go b/internal/atlas/image.go index 8872cffab..912b4b602 100644 --- a/internal/atlas/image.go +++ b/internal/atlas/image.go @@ -539,18 +539,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) error { +func (i *Image) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte, region image.Rectangle) chan error { backendsM.Lock() defer backendsM.Unlock() if !inFrame { - panic("atlas: ReadPixels must be called in between BeginFrame and EndFrame") + ch := make(chan error) + deferredM.Lock() + deferred = append(deferred, func() { + ch <- i.readPixels(graphicsDriver, pixels, region) + }) + deferredM.Unlock() + return ch } // 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() + ch := make(chan error, 1) + ch <- i.readPixels(graphicsDriver, pixels, region) + return ch +} + +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 diff --git a/internal/atlas/image_test.go b/internal/atlas/image_test.go index f0efd545d..767c3d307 100644 --- a/internal/atlas/image_test.go +++ b/internal/atlas/image_test.go @@ -117,7 +117,7 @@ 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 { + if err := <-img4.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size)); err != nil { t.Fatal(err) } for j := 0; j < size; j++ { @@ -212,7 +212,7 @@ 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 { + if err := <-img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size)); err != nil { t.Fatal(err) } for j := 0; j < size; j++ { @@ -236,7 +236,7 @@ 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 { + if err := <-img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size)); err != nil { t.Fatal(err) } for j := 0; j < size; j++ { @@ -324,7 +324,7 @@ 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 { + if err := <-img0.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix0, image.Rect(0, 0, w0, h0)); err != nil { t.Fatal(err) } for j := 0; j < h0; j++ { @@ -343,7 +343,7 @@ 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 { + if err := <-img1.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix1, image.Rect(0, 0, w1, h1)); err != nil { t.Fatal(err) } for j := 0; j < h1; j++ { @@ -385,7 +385,7 @@ 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 { + if err := <-dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h)); err != nil { t.Fatal(err) } for j := 0; j < h; j++ { @@ -427,7 +427,7 @@ 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) - if err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h)); err != nil { + if err := <-dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h)); err != nil { t.Fatal(err) } for j := 0; j < h; j++ { @@ -470,7 +470,7 @@ 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) - if err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, dstW, dstH)); err != nil { + if err := <-dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, dstW, dstH)); err != nil { t.Fatal(err) } for j := 0; j < h; j++ { @@ -684,7 +684,7 @@ 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 { + if err := <-img.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, size, size)); err != nil { t.Fatal(err) } for j := 0; j < size; j++ { diff --git a/internal/atlas/shader_test.go b/internal/atlas/shader_test.go index dd3966d12..82ef163cd 100644 --- a/internal/atlas/shader_test.go +++ b/internal/atlas/shader_test.go @@ -44,7 +44,7 @@ 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) - if err := dst.ReadPixels(g, pix, image.Rect(0, 0, w, h)); err != nil { + if err := <-dst.ReadPixels(g, pix, image.Rect(0, 0, w, h)); err != nil { t.Error(err) } 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 { @@ -71,7 +71,7 @@ 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) - if err := dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h)); err != nil { + if err := <-dst.ReadPixels(ui.Get().GraphicsDriverForTesting(), pix, image.Rect(0, 0, w, h)); err != nil { t.Error(err) } 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 { diff --git a/internal/buffered/image.go b/internal/buffered/image.go index 9e5244d28..00cc744ac 100644 --- a/internal/buffered/image.go +++ b/internal/buffered/image.go @@ -58,9 +58,14 @@ func (i *Image) MarkDisposed() { } func (i *Image) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte, region image.Rectangle) error { + // If this function is called from the game (Update/Draw) goroutine, and after EndFrame and before BeginFrame, + // (*atlas.Image).ReadPixels's channel never returns a value and causes a dead lock. + // It is assumed that this never happens so far, but if handling inputs after EndFrame is implemented, + // this might be possible (#1704). + // If restorable.AlwaysReadPixelsFromGPU() returns false, the pixel data is cached in the restorable package. if !restorable.AlwaysReadPixelsFromGPU() { - if err := i.img.ReadPixels(graphicsDriver, pixels, region); err != nil { + if err := <-i.img.ReadPixels(graphicsDriver, pixels, region); err != nil { return err } return nil @@ -68,7 +73,7 @@ func (i *Image) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte 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 { + if err := <-i.img.ReadPixels(graphicsDriver, pix, image.Rect(0, 0, i.width, i.height)); err != nil { return err } i.pixels = pix diff --git a/internal/processtest/testdata/issue2815.go b/internal/processtest/testdata/issue2815.go new file mode 100644 index 000000000..bc7043f53 --- /dev/null +++ b/internal/processtest/testdata/issue2815.go @@ -0,0 +1,72 @@ +// 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. + +//go:build ignore + +package main + +import ( + "time" + + "github.com/hajimehoshi/ebiten/v2" +) + +type Game struct { + init bool + count int + end0 chan struct{} + end1 chan struct{} +} + +func (g *Game) Update() error { + if !g.init { + g.end0 = make(chan struct{}) + g.end1 = make(chan struct{}) + img := ebiten.NewImage(1, 1) + go func() { + t := time.Tick(time.Microsecond) + loop: + for { + select { + case <-t: + img.At(0, 0) + case <-g.end0: + close(g.end1) + break loop + } + } + }() + g.init = true + } + g.count++ + if g.count >= 60 { + close(g.end0) + <-g.end1 + return ebiten.Termination + } + return nil +} + +func (g *Game) Draw(screen *ebiten.Image) { +} + +func (g *Game) Layout(w, h int) (int, int) { + return 320, 240 +} + +func main() { + if err := ebiten.RunGame(&Game{}); err != nil { + panic(err) + } +}