From 48408cba1185fa2131e5d2d3513447b8c300882f Mon Sep 17 00:00:00 2001 From: Hajime Hoshi Date: Sun, 11 Mar 2018 00:28:30 +0900 Subject: [PATCH] shareable: Bug fix: Protect critical sections --- image_test.go | 3 ++- internal/packing/packing.go | 21 ++--------------- internal/shareable/shareable.go | 41 +++++++++++++++++++++++++++------ 3 files changed, 38 insertions(+), 27 deletions(-) diff --git a/image_test.go b/image_test.go index da5a4ca94..55e7fdf7d 100644 --- a/image_test.go +++ b/image_test.go @@ -238,6 +238,7 @@ func TestImageScale(t *testing.T) { } op := &DrawImageOptions{} op.GeoM.Scale(float64(scale), float64(scale)) + if err := img1.DrawImage(img0, op); err != nil { t.Fatal(err) return @@ -317,7 +318,7 @@ func TestImageDotByDotInversion(t *testing.T) { } } -func TestReplacePixels(t *testing.T) { +func TestImageReplacePixels(t *testing.T) { // Create a dummy image so that the shared texture is used and origImg's position is shfited. dummyImg, _ := NewImageFromImage(image.NewRGBA(image.Rect(0, 0, 16, 16)), FilterDefault) defer dummyImg.Dispose() diff --git a/internal/packing/packing.go b/internal/packing/packing.go index bc8cb6b53..53eb095e7 100644 --- a/internal/packing/packing.go +++ b/internal/packing/packing.go @@ -15,10 +15,6 @@ // Package packing offers a packing algorithm in 2D space. package packing -import ( - "github.com/hajimehoshi/ebiten/internal/sync" -) - const ( minSize = 1 ) @@ -26,7 +22,6 @@ const ( type Page struct { root *Node maxSize int - m sync.Mutex } func NewPage(maxSize int) *Page { @@ -36,14 +31,10 @@ func NewPage(maxSize int) *Page { } func (p *Page) IsEmpty() bool { - p.m.Lock() if p.root == nil { - p.m.Unlock() return true } - r := !p.root.used && p.root.child0 == nil && p.root.child1 == nil - p.m.Unlock() - return r + return !p.root.used && p.root.child0 == nil && p.root.child1 == nil } type Node struct { @@ -145,7 +136,6 @@ func (p *Page) alloc(n *Node, width, height int) *Node { } func (p *Page) Alloc(width, height int) *Node { - p.m.Lock() if width <= 0 || height <= 0 { panic("bsp: width and height must > 0") } @@ -162,17 +152,10 @@ func (p *Page) Alloc(width, height int) *Node { height = minSize } n := p.alloc(p.root, width, height) - p.m.Unlock() return n } func (p *Page) Free(node *Node) { - p.m.Lock() - p.free(node) - p.m.Unlock() -} - -func (p *Page) free(node *Node) { if node.child0 != nil || node.child1 != nil { panic("bsp: can't free the node including children") } @@ -186,6 +169,6 @@ func (p *Page) free(node *Node) { if node.parent.child0.canFree() && node.parent.child1.canFree() { node.parent.child0 = nil node.parent.child1 = nil - p.free(node.parent) + p.Free(node.parent) } } diff --git a/internal/shareable/shareable.go b/internal/shareable/shareable.go index ff48f9a2f..ce7b2a768 100644 --- a/internal/shareable/shareable.go +++ b/internal/shareable/shareable.go @@ -33,6 +33,9 @@ type backend struct { } var ( + // backendsM is a mutex for critical sections of the backend and packing.Node objects. + backendsM sync.Mutex + // theBackends is a set of actually shared images. theBackends = []*backend{} ) @@ -53,7 +56,7 @@ func (s *Image) ensureNotShared() { newImg := restorable.NewImage(w, h, false) newImg.DrawImage(s.backend.restorable, x, y, w, h, nil, nil, opengl.CompositeModeCopy, graphics.FilterNearest) - s.Dispose() + s.dispose() s.backend = &backend{ restorable: newImg, } @@ -68,11 +71,19 @@ func (s *Image) region() (x, y, width, height int) { } func (s *Image) Size() (width, height int) { + backendsM.Lock() _, _, w, h := s.region() + backendsM.Unlock() return w, h } func (s *Image) DrawImage(img *Image, sx0, sy0, sx1, sy1 int, geom *affine.GeoM, colorm *affine.ColorM, mode opengl.CompositeMode, filter graphics.Filter) { + backendsM.Lock() + defer backendsM.Unlock() + s.drawImage(img, sx0, sy0, sx1, sy1, geom, colorm, mode, filter) +} + +func (s *Image) drawImage(img *Image, sx0, sy0, sx1, sy1 int, geom *affine.GeoM, colorm *affine.ColorM, mode opengl.CompositeMode, filter graphics.Filter) { s.ensureNotShared() // Compare i and img after ensuring i is not shared, or @@ -90,6 +101,9 @@ func (s *Image) DrawImage(img *Image, sx0, sy0, sx1, sy1 int, geom *affine.GeoM, } func (s *Image) ReplacePixels(p []byte) { + backendsM.Lock() + defer backendsM.Unlock() + x, y, w, h := s.region() if l := 4 * w * h; len(p) != l { panic(fmt.Sprintf("shareable: len(p) was %d but must be %d", len(p), l)) @@ -98,11 +112,17 @@ func (s *Image) ReplacePixels(p []byte) { } func (s *Image) At(x, y int) (color.Color, error) { + backendsM.Lock() + ox, oy, w, h := s.region() if x < 0 || y < 0 || x >= w || y >= h { + backendsM.Unlock() return color.RGBA{}, nil } - return s.backend.restorable.At(x+ox, y+oy) + + clr, err := s.backend.restorable.At(x+ox, y+oy) + backendsM.Unlock() + return clr, err } func (s *Image) isDisposed() bool { @@ -110,6 +130,12 @@ func (s *Image) isDisposed() bool { } func (s *Image) Dispose() { + backendsM.Lock() + defer backendsM.Unlock() + s.dispose() +} + +func (s *Image) dispose() { if s.isDisposed() { return } @@ -144,16 +170,17 @@ func (s *Image) Dispose() { } func (s *Image) IsInvalidated() (bool, error) { - return s.backend.restorable.IsInvalidated() + backendsM.Lock() + v, err := s.backend.restorable.IsInvalidated() + backendsM.Unlock() + return v, err } -var shareableImageLock sync.Mutex - func NewImage(width, height int) *Image { const maxSize = 2048 - shareableImageLock.Lock() - defer shareableImageLock.Unlock() + backendsM.Lock() + defer backendsM.Unlock() if width > maxSize || height > maxSize { s := &backend{