From c31cc4ecffca2657444e50716d638f3ebbf84ad3 Mon Sep 17 00:00:00 2001 From: Hajime Hoshi Date: Sun, 3 Jul 2022 13:53:59 +0900 Subject: [PATCH] internal/graphicscommand: bug fix: using an image just after ReplacePixels might fail on Metal Inserting a dummy rendering command after ReplacePixels and before DrawTriangles solved the issue. This is a very dirty hack, but there seems no other better way. Closes #2154 --- internal/graphicscommand/image.go | 42 +++++- .../graphicsdriver/metal/graphics_darwin.go | 1 - internal/processtest/testdata/issue2154.go | 121 ++++++++++++++++++ 3 files changed, 158 insertions(+), 6 deletions(-) create mode 100644 internal/processtest/testdata/issue2154.go diff --git a/internal/graphicscommand/image.go b/internal/graphicscommand/image.go index 67fdb780a..d2ee4914f 100644 --- a/internal/graphicscommand/image.go +++ b/internal/graphicscommand/image.go @@ -18,6 +18,7 @@ import ( "fmt" "image" "os" + "runtime" "sort" "strings" @@ -54,6 +55,11 @@ func genNextID() int { return id } +var ( + dummyDst *Image + dummySrc *Image +) + // NewImage returns a new image. // // Note that the image is not initialized yet. @@ -74,9 +80,9 @@ func NewImage(width, height int, screenFramebuffer bool) *Image { return i } -func (i *Image) resolveBufferedReplacePixels() { +func (i *Image) resolveBufferedReplacePixels() bool { if len(i.bufferedRP) == 0 { - return + return false } c := &replacePixelsCommand{ dst: i, @@ -84,6 +90,8 @@ func (i *Image) resolveBufferedReplacePixels() { } theCommandQueue.Enqueue(c) i.bufferedRP = nil + + return true } func (i *Image) Dispose() { @@ -128,13 +136,16 @@ func (i *Image) InternalSize() (int, int) { // If the source image is not specified, i.e., src is nil and there is no image in the uniform variables, the // elements for the source image are not used. func (i *Image) DrawTriangles(srcs [graphics.ShaderImageNum]*Image, offsets [graphics.ShaderImageNum - 1][2]float32, vertices []float32, indices []uint16, clr affine.ColorM, mode graphicsdriver.CompositeMode, filter graphicsdriver.Filter, address graphicsdriver.Address, dstRegion, srcRegion graphicsdriver.Region, shader *Shader, uniforms [][]float32, evenOdd bool) { + var needDummyRendering bool if shader == nil { // Fast path for rendering without a shader (#1355). img := srcs[0] if img.screen { panic("graphicscommand: the screen image cannot be the rendering source") } - img.resolveBufferedReplacePixels() + if img.resolveBufferedReplacePixels() { + needDummyRendering = true + } } else { for _, src := range srcs { if src == nil { @@ -143,10 +154,31 @@ func (i *Image) DrawTriangles(srcs [graphics.ShaderImageNum]*Image, offsets [gra if src.screen { panic("graphicscommand: the screen image cannot be the rendering source") } - src.resolveBufferedReplacePixels() + if src.resolveBufferedReplacePixels() { + needDummyRendering = true + } } } - i.resolveBufferedReplacePixels() + if i.resolveBufferedReplacePixels() { + needDummyRendering = true + } + + // On Metal, using an image just after ReplacePixel might fail (#2154). + // This is a very dirty hack but there seemed no other way. + // In order to reproduce the issue, run: + // + // go test ./internal/processtest/ -run=/issue2154.go -count=100 + // + // TODO: Detect the graphics library and apply this logic only when the graphics library is Metal. + if runtime.GOOS == "darwin" && needDummyRendering { + if dummyDst == nil { + dummyDst = NewImage(1, 1, false) + } + if dummySrc == nil { + dummySrc = NewImage(1, 1, false) + } + dummyDst.DrawTriangles([graphics.ShaderImageNum]*Image{dummySrc}, [graphics.ShaderImageNum - 1][2]float32{}, make([]float32, 8*3), []uint16{0, 1, 2}, affine.ColorMIdentity{}, graphicsdriver.CompositeModeSourceOver, graphicsdriver.FilterNearest, graphicsdriver.AddressUnsafe, graphicsdriver.Region{}, graphicsdriver.Region{}, nil, nil, false) + } theCommandQueue.EnqueueDrawTrianglesCommand(i, srcs, offsets, vertices, indices, clr, mode, filter, address, dstRegion, srcRegion, shader, uniforms, evenOdd) } diff --git a/internal/graphicsdriver/metal/graphics_darwin.go b/internal/graphicsdriver/metal/graphics_darwin.go index f5624edeb..e6443f028 100644 --- a/internal/graphicsdriver/metal/graphics_darwin.go +++ b/internal/graphicsdriver/metal/graphics_darwin.go @@ -1185,7 +1185,6 @@ func (i *Image) syncTexture() { bce.EndEncoding() cb.Commit() - // TODO: Are fences available here? cb.WaitUntilCompleted() } diff --git a/internal/processtest/testdata/issue2154.go b/internal/processtest/testdata/issue2154.go new file mode 100644 index 000000000..cee3c6168 --- /dev/null +++ b/internal/processtest/testdata/issue2154.go @@ -0,0 +1,121 @@ +// Copyright 2022 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 +// +build ignore + +package main + +import ( + "errors" + "fmt" + "image/color" + + "github.com/hajimehoshi/ebiten/v2" +) + +var regularTermination = errors.New("regular termination") + +var srcInit *ebiten.Image + +func init() { + const ( + w = 2 + h = 2 + ) + + //src2 := ebiten.NewImage(1, 1) + + src0 := ebiten.NewImage(w, h) + src0.Fill(color.RGBA{0xff, 0xff, 0xff, 0xff}) + src0.Set(0, 0, color.RGBA{0, 0, 0, 0xff}) + src0.Set(0, 1, color.RGBA{0, 0, 0, 0xff}) + src0.Set(1, 0, color.RGBA{0, 0, 0, 0xff}) + + src1 := ebiten.NewImage(w, h) + // Using the image as a source just after Set caused troubles on Metal. + // For example, inserting src1.Fill(color.RGBA{0, 0xff, 0, 0xff}) here hid the error. + src1.DrawImage(src0, nil) + + srcInit = src1 +} + +type Game struct { + count int + dst *ebiten.Image +} + +func (g *Game) Update() error { + g.count++ + if g.count == 16 { + return regularTermination + } + return nil +} + +func (g *Game) Draw(screen *ebiten.Image) { + screen.Fill(color.RGBA{0xff, 0xff, 0xff, 0xff}) + screen.DrawImage(srcInit, nil) + + if g.dst == nil { + g.dst = ebiten.NewImage(screen.Size()) + return + } + + g.dst.DrawImage(screen, nil) + if got, want := g.dst.At(0, 0), (color.RGBA{0, 0, 0, 0xff}); got != want { + panic(fmt.Sprintf("count: %d, got: %v, want: %v", g.count, got, want)) + } + if got, want := g.dst.At(1, 1), (color.RGBA{0xff, 0xff, 0xff, 0xff}); got != want { + panic(fmt.Sprintf("count: %d, got: %v, want: %v", g.count, got, want)) + } + g.dst.Clear() + + const ( + w = 2 + h = 2 + ) + + src0 := ebiten.NewImage(w, h) + defer src0.Dispose() + src0.Fill(color.RGBA{0xff, 0xff, 0xff, 0xff}) + src0.Set(0, 0, color.RGBA{0, 0, 0, 0xff}) + src0.Set(0, 1, color.RGBA{0, 0, 0, 0xff}) + src0.Set(1, 0, color.RGBA{0, 0, 0, 0xff}) + + src1 := ebiten.NewImage(w, h) + defer src1.Dispose() + src1.DrawImage(src0, nil) + + screen.Fill(color.RGBA{0xff, 0xff, 0xff, 0xff}) + screen.DrawImage(src1, nil) + + g.dst.DrawImage(screen, nil) + if got, want := g.dst.At(0, 0), (color.RGBA{0, 0, 0, 0xff}); got != want { + panic(fmt.Sprintf("count: %d, got: %v, want: %v", g.count, got, want)) + } + if got, want := g.dst.At(1, 1), (color.RGBA{0xff, 0xff, 0xff, 0xff}); got != want { + panic(fmt.Sprintf("count: %d, got: %v, want: %v", g.count, got, want)) + } +} + +func (g *Game) Layout(width, height int) (int, int) { + return width, height +} + +func main() { + if err := ebiten.RunGame(&Game{}); err != nil && err != regularTermination { + panic(err) + } +}