From 6b3c51921c89614b225a54f14acde31b1c9876d2 Mon Sep 17 00:00:00 2001 From: Hajime Hoshi Date: Fri, 26 Feb 2021 23:00:10 +0900 Subject: [PATCH] internal/driver: Remove Image.Sync Syncing is no longer needed for Metal, and additionally, OpenGL's sync implementation was mock. Updates #1508 --- internal/driver/graphics.go | 12 ------- internal/graphicscommand/command.go | 32 ------------------- internal/graphicscommand/image.go | 17 ---------- internal/graphicsdriver/metal/graphics.go | 38 +++-------------------- internal/graphicsdriver/metal/mtl/mtl.go | 27 ---------------- internal/graphicsdriver/metal/mtl/mtl.h | 1 - internal/graphicsdriver/metal/mtl/mtl.m | 10 ------ internal/graphicsdriver/opengl/image.go | 7 ----- internal/restorable/image.go | 11 ------- internal/shareable/export_test.go | 3 +- internal/shareable/image.go | 22 ------------- internal/shareable/image_test.go | 2 +- 12 files changed, 6 insertions(+), 176 deletions(-) diff --git a/internal/driver/graphics.go b/internal/driver/graphics.go index 9b05248a1..d512da900 100644 --- a/internal/driver/graphics.go +++ b/internal/driver/graphics.go @@ -68,18 +68,6 @@ type Image interface { ID() ImageID Dispose() IsInvalidated() bool - - // Sync syncs the texture data in CPU and GPU so that Pixels can return the texture data immediately. - // In most cases, Sync doesn't have to be called explicitly. - // Even without Sync, Pixels should does Sync automatically, but this might take long. - // - // Sync returns a channel that is closed when syncing finishes. - // - // Whatever the syncing state is, the other function should work correctly. - // - // TODO: Should the other functions be blocked during syncing? - Sync() <-chan struct{} - Pixels() ([]byte, error) ReplacePixels(args []*ReplacePixelsArgs) } diff --git a/internal/graphicscommand/command.go b/internal/graphicscommand/command.go index 0e97b5176..5ef686864 100644 --- a/internal/graphicscommand/command.go +++ b/internal/graphicscommand/command.go @@ -512,38 +512,6 @@ func (c *replacePixelsCommand) CanMergeWithDrawTrianglesCommand(dst *Image, src return false } -type syncCommand struct { - result <-chan struct{} - img *Image -} - -func (c *syncCommand) Exec(indexOffset int) error { - c.result = c.img.image.Sync() - return nil -} - -func (c *syncCommand) NumVertices() int { - return 0 -} - -func (c *syncCommand) NumIndices() int { - return 0 -} - -func (c *syncCommand) AddNumVertices(n int) { -} - -func (c *syncCommand) AddNumIndices(n int) { -} - -func (c *syncCommand) CanMergeWithDrawTrianglesCommand(dst *Image, src [graphics.ShaderImageNum]*Image, color *affine.ColorM, mode driver.CompositeMode, filter driver.Filter, address driver.Address, dstRegion, srcRegion driver.Region, shader *Shader) bool { - return false -} - -func (c *syncCommand) String() string { - return fmt.Sprintf("sync: image: %d", c.img.id) -} - type pixelsCommand struct { result []byte img *Image diff --git a/internal/graphicscommand/image.go b/internal/graphicscommand/image.go index 85b252387..d39661fdf 100644 --- a/internal/graphicscommand/image.go +++ b/internal/graphicscommand/image.go @@ -163,23 +163,6 @@ func (i *Image) DrawTriangles(srcs [graphics.ShaderImageNum]*Image, offsets [gra theCommandQueue.EnqueueDrawTrianglesCommand(i, srcs, offsets, vertices, indices, clr, mode, filter, address, dstRegion, srcRegion, shader, uniforms) } -// Sync syncs the texture data in CPU and GPU so that Pixels can return the texture data immediately. -// In most cases, Sync doesn't have to be called explicitly. -// Even without Sync, Pixels should does Sync automatically, but this might take long. -// -// Sync returns a channel that is closed when syncing finishes. -func (i *Image) Sync() (<-chan struct{}, error) { - i.resolveBufferedReplacePixels() - c := &syncCommand{ - img: i, - } - theCommandQueue.Enqueue(c) - if err := theCommandQueue.Flush(); err != nil { - return nil, err - } - return c.result, nil -} - // Pixels returns the image's pixels. // Pixels might return nil when OpenGL error happens. func (i *Image) Pixels() ([]byte, error) { diff --git a/internal/graphicsdriver/metal/graphics.go b/internal/graphicsdriver/metal/graphics.go index fca8a5d3a..fc53a056e 100644 --- a/internal/graphicsdriver/metal/graphics.go +++ b/internal/graphicsdriver/metal/graphics.go @@ -690,7 +690,6 @@ func (g *Graphics) draw(rps mtl.RenderPipelineState, dst *Image, dstRegion drive func (g *Graphics) Draw(dstID, srcID driver.ImageID, indexLen int, indexOffset int, mode driver.CompositeMode, colorM *affine.ColorM, filter driver.Filter, address driver.Address, dstRegion, srcRegion driver.Region) error { dst := g.images[dstID] - dst.waitUntilSyncFinishes() srcs := [graphics.ShaderImageNum]*Image{g.images[srcID]} @@ -820,15 +819,6 @@ type Image struct { height int screen bool texture mtl.Texture - sync <-chan struct{} -} - -func (i *Image) waitUntilSyncFinishes() { - if i.sync == nil { - return - } - <-i.sync - i.sync = nil } func (i *Image) ID() driver.ImageID { @@ -843,9 +833,6 @@ func (i *Image) internalSize() (int, int) { } func (i *Image) Dispose() { - // TODO: Is it necessary to wait for syncing? - i.waitUntilSyncFinishes() - if i.texture != (mtl.Texture{}) { i.texture.Release() i.texture = mtl.Texture{} @@ -860,13 +847,7 @@ func (i *Image) IsInvalidated() bool { return false } -func (i *Image) Sync() <-chan struct{} { - if i.sync != nil { - return i.sync - } - - i.graphics.flushIfNeeded(false) - +func (i *Image) syncTexture() { // Calling SynchronizeTexture is ignored on iOS (see mtl.m), but it looks like committing BlitCommandEncoder // is necessary (#1337). if i.graphics.cb != (mtl.CommandBuffer{}) { @@ -878,21 +859,13 @@ func (i *Image) Sync() <-chan struct{} { bce.SynchronizeTexture(i.texture, 0, 0) bce.EndEncoding() - ch := make(chan struct{}) - cb.AddCompletedHandler(func() { - close(ch) - }) cb.Commit() - - i.sync = ch - return i.sync + cb.WaitUntilCompleted() } func (i *Image) Pixels() ([]byte, error) { - // Call Sync just in case when the user doesn't call Sync. - // If the image is already synced, the channel should be closed immediately. - <-i.Sync() - i.sync = nil + i.graphics.flushIfNeeded(false) + i.syncTexture() b := make([]byte, 4*i.width*i.height) i.texture.GetBytes(&b[0], uintptr(4*i.width), mtl.Region{ @@ -902,8 +875,6 @@ func (i *Image) Pixels() ([]byte, error) { } func (i *Image) ReplacePixels(args []*driver.ReplacePixelsArgs) { - i.waitUntilSyncFinishes() - g := i.graphics // Use a temporary texture to send pixels asynchrounsly, whichever the memory is shared (e.g., iOS) or @@ -941,7 +912,6 @@ func (i *Image) ReplacePixels(args []*driver.ReplacePixelsArgs) { func (g *Graphics) DrawShader(dstID driver.ImageID, srcIDs [graphics.ShaderImageNum]driver.ImageID, offsets [graphics.ShaderImageNum - 1][2]float32, shader driver.ShaderID, indexLen int, indexOffset int, dstRegion, srcRegion driver.Region, mode driver.CompositeMode, uniforms []interface{}) error { dst := g.images[dstID] - dst.waitUntilSyncFinishes() var srcs [graphics.ShaderImageNum]*Image for i, srcID := range srcIDs { diff --git a/internal/graphicsdriver/metal/mtl/mtl.go b/internal/graphicsdriver/metal/mtl/mtl.go index 13e91f129..c52ccb2fd 100644 --- a/internal/graphicsdriver/metal/mtl/mtl.go +++ b/internal/graphicsdriver/metal/mtl/mtl.go @@ -27,7 +27,6 @@ package mtl import ( "errors" "fmt" - "sync" "unsafe" ) @@ -569,32 +568,6 @@ func (cb CommandBuffer) WaitUntilCompleted() { C.CommandBuffer_WaitUntilCompleted(cb.commandBuffer) } -var ( - commandBufferCompletedHandlers = map[unsafe.Pointer]func(){} - commandBufferCompletedHandlersM sync.Mutex -) - -// AddCompletedHandler registers a block of code that Metal calls immediately after the GPU finishes executing the commands in the command buffer. -// -// Reference: https://developer.apple.com/documentation/metal/mtlcommandbuffer/1442997-addcompletedhandler -func (cb CommandBuffer) AddCompletedHandler(f func()) { - commandBufferCompletedHandlersM.Lock() - commandBufferCompletedHandlers[cb.commandBuffer] = f - commandBufferCompletedHandlersM.Unlock() - - C.CommandBuffer_AddCompletedHandler(cb.commandBuffer) -} - -//export commandBufferCompletedCallback -func commandBufferCompletedCallback(commandBuffer unsafe.Pointer) { - commandBufferCompletedHandlersM.Lock() - f := commandBufferCompletedHandlers[commandBuffer] - delete(commandBufferCompletedHandlers, commandBuffer) - commandBufferCompletedHandlersM.Unlock() - - f() -} - // MakeRenderCommandEncoder creates an encoder object that can // encode graphics rendering commands into this command buffer. // diff --git a/internal/graphicsdriver/metal/mtl/mtl.h b/internal/graphicsdriver/metal/mtl/mtl.h index 6fbd804eb..c4e6ef40b 100644 --- a/internal/graphicsdriver/metal/mtl/mtl.h +++ b/internal/graphicsdriver/metal/mtl/mtl.h @@ -133,7 +133,6 @@ void CommandBuffer_Release(void *commandBuffer); void CommandBuffer_PresentDrawable(void *commandBuffer, void *drawable); void CommandBuffer_Commit(void *commandBuffer); void CommandBuffer_WaitUntilCompleted(void *commandBuffer); -void CommandBuffer_AddCompletedHandler(void *commandBuffer); void * CommandBuffer_MakeRenderCommandEncoder(void *commandBuffer, struct RenderPassDescriptor descriptor); diff --git a/internal/graphicsdriver/metal/mtl/mtl.m b/internal/graphicsdriver/metal/mtl/mtl.m index a7720ef3d..387438044 100644 --- a/internal/graphicsdriver/metal/mtl/mtl.m +++ b/internal/graphicsdriver/metal/mtl/mtl.m @@ -18,9 +18,6 @@ #import #include -// commandBufferCompletedCallback is an exported function from Go. -void commandBufferCompletedCallback(void *commandBuffer); - struct Device CreateSystemDefaultDevice() { id device = MTLCreateSystemDefaultDevice(); if (!device) { @@ -149,13 +146,6 @@ void CommandBuffer_WaitUntilCompleted(void *commandBuffer) { [(id)commandBuffer waitUntilCompleted]; } -void CommandBuffer_AddCompletedHandler(void *commandBuffer) { - [(id)commandBuffer - addCompletedHandler:^(id cb) { - commandBufferCompletedCallback(cb); - }]; -} - void * CommandBuffer_MakeRenderCommandEncoder(void *commandBuffer, struct RenderPassDescriptor descriptor) { diff --git a/internal/graphicsdriver/opengl/image.go b/internal/graphicsdriver/opengl/image.go index d3abb2594..f00d5a2de 100644 --- a/internal/graphicsdriver/opengl/image.go +++ b/internal/graphicsdriver/opengl/image.go @@ -60,13 +60,6 @@ func (i *Image) setViewport() error { return nil } -func (i *Image) Sync() <-chan struct{} { - // There is no way to sync the textures data on the system memory and GPU. - ch := make(chan struct{}) - close(ch) - return ch -} - func (i *Image) Pixels() ([]byte, error) { if err := i.ensureFramebuffer(); err != nil { return nil, err diff --git a/internal/restorable/image.go b/internal/restorable/image.go index 975ec6e05..a23fc3136 100644 --- a/internal/restorable/image.go +++ b/internal/restorable/image.go @@ -434,17 +434,6 @@ func (i *Image) readPixelsFromGPUIfNeeded() error { return nil } -func (i *Image) Sync() (<-chan struct{}, error) { - if err := graphicscommand.FlushCommands(); err != nil { - return nil, err - } - ch, err := i.image.Sync() - if err != nil { - return nil, err - } - return ch, nil -} - // At returns a color value at (x, y). // // Note that this must not be called until context is available. diff --git a/internal/shareable/export_test.go b/internal/shareable/export_test.go index 302ced95d..3109c1f6a 100644 --- a/internal/shareable/export_test.go +++ b/internal/shareable/export_test.go @@ -15,8 +15,7 @@ package shareable const ( - MaxCountForShare = maxCountForShare - CountForStartSyncing = countForStartSyncing + MaxCountForShare = maxCountForShare ) func MakeImagesSharedForTesting() error { diff --git a/internal/shareable/image.go b/internal/shareable/image.go index dca880076..4707ae114 100644 --- a/internal/shareable/image.go +++ b/internal/shareable/image.go @@ -76,33 +76,15 @@ func resolveDeferred() { // maxCountForShare represents the time duration when the image can become shared. const maxCountForShare = 10 -// countForStartSyncing represents the count that the image starts to sync pixels between GPU and CPU. -const countForStartSyncing = maxCountForShare / 2 - func makeImagesShared() error { for i := range imagesToMakeShared { i.usedAsSourceCount++ - if restorable.NeedsRestoring() && i.usedAsSourceCount >= countForStartSyncing && i.syncing == nil { - // Sync the pixel data on CPU and GPU sides explicitly in order not to block this process. - ch, err := i.backend.restorable.Sync() - if err != nil { - return err - } - i.syncing = ch - } if i.usedAsSourceCount >= maxCountForShare { - if restorable.NeedsRestoring() { - // TODO: Instead of waiting for the channel, use select-case and continue the loop - // if this channel is blocking. However, this might make the tests difficult. - <-i.syncing - } - if err := i.makeShared(); err != nil { return err } i.usedAsSourceCount = 0 delete(imagesToMakeShared, i) - i.syncing = nil } } @@ -201,8 +183,6 @@ type Image struct { // // ReplacePixels doesn't affect this value since ReplacePixels can be done on shared images. usedAsSourceCount int - - syncing <-chan struct{} } func (i *Image) moveTo(dst *Image) { @@ -221,7 +201,6 @@ func (i *Image) isShared() bool { func (i *Image) resetUsedAsSourceCount() { i.usedAsSourceCount = 0 delete(imagesToMakeShared, i) - i.syncing = nil } func (i *Image) ensureNotShared() { @@ -321,7 +300,6 @@ func (i *Image) makeShared() error { newI.moveTo(i) i.usedAsSourceCount = 0 - i.syncing = nil return nil } diff --git a/internal/shareable/image_test.go b/internal/shareable/image_test.go index 349282d4f..0ac6b25d2 100644 --- a/internal/shareable/image_test.go +++ b/internal/shareable/image_test.go @@ -562,7 +562,7 @@ func TestDisposedAndReshared(t *testing.T) { } // Use src as a render source. - for i := 0; i < CountForStartSyncing; i++ { + for i := 0; i < MaxCountForShare/2; i++ { if err := MakeImagesSharedForTesting(); err != nil { t.Fatal(err) }