Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert using a separate draw thread #4656

Merged
merged 6 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions internal/animation/animation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ import (
"fyne.io/fyne/v2"
)

func tick(run *Runner) {
time.Sleep(time.Millisecond * 5) // wait long enough that we are not at 0 time
run.TickAnimations()
}

func TestGLDriver_StartAnimation(t *testing.T) {
done := make(chan float32)
run := &Runner{}
Expand All @@ -22,6 +27,7 @@ func TestGLDriver_StartAnimation(t *testing.T) {
}}

run.Start(a)
go tick(run) // simulate a graphics draw loop
select {
case d := <-done:
assert.Greater(t, d, float32(0))
Expand All @@ -40,6 +46,7 @@ func TestGLDriver_StopAnimation(t *testing.T) {
}}

run.Start(a)
go tick(run) // simulate a graphics draw loop
select {
case d := <-done:
assert.Greater(t, d, float32(0))
Expand All @@ -63,8 +70,12 @@ func TestGLDriver_StopAnimationImmediatelyAndInsideTick(t *testing.T) {
Tick: func(f float32) {},
}
run.Start(a)
go tick(run) // simulate a graphics draw loop
run.Stop(a)

run = &Runner{}
wg = sync.WaitGroup{}

// stopping animation inside tick function
for i := 0; i < 10; i++ {
wg.Add(1)
Expand All @@ -78,14 +89,20 @@ func TestGLDriver_StopAnimationImmediatelyAndInsideTick(t *testing.T) {
run.Start(b)
}

run = &Runner{}
wg = sync.WaitGroup{}

// Similar to first part, but in this time this animation should be added and then removed
// from pendingAnimation slice.
c := &fyne.Animation{
Duration: time.Second,
Tick: func(f float32) {},
}
run.Start(c)
go tick(run) // simulate a graphics draw loop

run.Stop(c)
go tick(run) // simulate a graphics draw loop

wg.Wait()
// animations stopped inside tick are really stopped in the next runner cycle
Expand Down
21 changes: 11 additions & 10 deletions internal/animation/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ func (r *Runner) Start(a *fyne.Animation) {
r.animations = make([]*anim, 0, 16)
}
r.animations = append(r.animations, newAnim(a))
r.runAnimations()
} else {
if r.pendingAnimations == nil {
// initialize with excess capacity to avoid re-allocations
Expand Down Expand Up @@ -85,18 +84,20 @@ func (r *Runner) Stop(a *fyne.Animation) {
r.pendingAnimations = newList
}

func (r *Runner) runAnimations() {
draw := time.NewTicker(time.Second / 60)
go func() {
for done := false; !done; {
<-draw.C
done = r.runOneFrame()
}
// TickAnimations progresses all running animations by one tick.
// This will be called from the driver to update objects immediately before next paint.
func (r *Runner) TickAnimations() {
if !r.runnerStarted {
return
}

done := r.runOneFrame()

if done {
r.animationMutex.Lock()
r.runnerStarted = false
r.animationMutex.Unlock()
draw.Stop()
}()
}
}

func (r *Runner) runOneFrame() (done bool) {
Expand Down
2 changes: 1 addition & 1 deletion internal/driver/glfw/canvas.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type glCanvas struct {

func (c *glCanvas) Capture() image.Image {
var img image.Image
runOnDraw(c.context.(*window), func() {
runOnMainWithContext(c.context.(*window), func() {
img = c.Painter().Capture(c)
})
return img
Expand Down
5 changes: 0 additions & 5 deletions internal/driver/glfw/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,10 @@ var curWindow *window
// Declare conformity with Driver
var _ fyne.Driver = (*gLDriver)(nil)

// A workaround on Apple M1/M2, just use 1 thread until fixed upstream.
const drawOnMainThread bool = runtime.GOOS == "darwin" && runtime.GOARCH == "arm64"

type gLDriver struct {
windowLock sync.RWMutex
windows []fyne.Window
done chan struct{}
drawDone chan struct{}
waitForStart chan struct{}

animation animation.Runner
Expand Down Expand Up @@ -179,7 +175,6 @@ func NewGLDriver() *gLDriver {

return &gLDriver{
done: make(chan struct{}),
drawDone: make(chan struct{}),
waitForStart: make(chan struct{}),
}
}
2 changes: 1 addition & 1 deletion internal/driver/glfw/glfw_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func repaintWindow(w *window) {
// If we try to paint windows before the context is created, we will end up on the wrong thread.
<-w.driver.waitForStart

runOnDraw(w, func() {
runOnMainWithContext(w, func() {
d.repaintWindow(w)
})

Expand Down
79 changes: 20 additions & 59 deletions internal/driver/glfw/loop.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,8 @@ type funcData struct {
done chan struct{} // Zero allocation signalling channel
}

type drawData struct {
f func()
win *window
done chan struct{} // Zero allocation signalling channel
}

// channel for queuing functions on the main thread
var funcQueue = make(chan funcData)
var drawFuncQueue = make(chan drawData)
var running atomic.Bool
var initOnce = &sync.Once{}

Expand Down Expand Up @@ -55,16 +48,8 @@ func runOnMain(f func()) {
}

// force a function f to run on the draw thread
func runOnDraw(w *window, f func()) {
if drawOnMainThread {
runOnMain(func() { w.RunWithContext(f) })
return
}
done := common.DonePool.Get()
defer common.DonePool.Put(done)

drawFuncQueue <- drawData{f: f, win: w, done: done}
<-done
func runOnMainWithContext(w *window, f func()) {
runOnMain(func() { w.RunWithContext(f) }) // TODO remove this completely
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a TODO for an upcoming PR or should it be removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is part of the future refactoring. Lots more to be done as mentioned above and in the other tickets.

There are 3 different ways to queue onto what is essentially the same thread but I didn't want to rip out too much code until the direction is established :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Just wanted to check. Still approved from my end :)

}

// Preallocate to avoid allocations on every drawSingleFrame.
Expand Down Expand Up @@ -117,12 +102,15 @@ func (d *gLDriver) runGL() {
if f := fyne.CurrentApp().Lifecycle().(*app.Lifecycle).OnStarted(); f != nil {
go f() // don't block main, we don't have window event queue
}

settingsChange := make(chan fyne.Settings)
fyne.CurrentApp().Settings().AddChangeListener(settingsChange)

eventTick := time.NewTicker(time.Second / 60)
for {
select {
case <-d.done:
eventTick.Stop()
d.drawDone <- struct{}{} // wait for draw thread to stop
d.Terminate()
l := fyne.CurrentApp().Lifecycle().(*app.Lifecycle)
if f := l.OnStopped(); f != nil {
Expand Down Expand Up @@ -163,9 +151,8 @@ func (d *gLDriver) runGL() {
}
}

if drawOnMainThread {
d.drawSingleFrame()
}
d.animation.TickAnimations()
d.drawSingleFrame()
}
if windowsToRemove > 0 {
oldWindows := d.windowList()
Expand Down Expand Up @@ -200,6 +187,18 @@ func (d *gLDriver) runGL() {
d.Quit()
}
}
case set := <-settingsChange:
painter.ClearFontCache()
cache.ResetThemeCaches()
app.ApplySettingsWithCallback(set, fyne.CurrentApp(), func(w fyne.Window) {
c, ok := w.Canvas().(*glCanvas)
if !ok {
return
}
c.applyThemeOutOfTreeObjects()
go c.reloadScale()
})

}
}
}
Expand Down Expand Up @@ -228,44 +227,6 @@ func (d *gLDriver) repaintWindow(w *window) {
})
}

func (d *gLDriver) startDrawThread() {
settingsChange := make(chan fyne.Settings)
fyne.CurrentApp().Settings().AddChangeListener(settingsChange)
var drawCh <-chan time.Time
if drawOnMainThread {
drawCh = make(chan time.Time) // don't tick when on M1
} else {
drawCh = time.NewTicker(time.Second / 60).C
}

go func() {
runtime.LockOSThread()

for {
select {
case <-d.drawDone:
return
case f := <-drawFuncQueue:
f.win.RunWithContext(f.f)
f.done <- struct{}{}
case set := <-settingsChange:
painter.ClearFontCache()
cache.ResetThemeCaches()
app.ApplySettingsWithCallback(set, fyne.CurrentApp(), func(w fyne.Window) {
c, ok := w.Canvas().(*glCanvas)
if !ok {
return
}
c.applyThemeOutOfTreeObjects()
go c.reloadScale()
})
case <-drawCh:
d.drawSingleFrame()
}
}
}()
}

// refreshWindow requests that the specified window be redrawn
func refreshWindow(w *window) {
w.canvas.SetDirty()
Expand Down
1 change: 0 additions & 1 deletion internal/driver/glfw/loop_desktop.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ func (d *gLDriver) initGLFW() {
}

initCursors()
d.startDrawThread()
})
}

Expand Down
2 changes: 0 additions & 2 deletions internal/driver/glfw/loop_goxjs.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ func (d *gLDriver) initGLFW() {
fyne.LogError("failed to initialise GLFW", err)
return
}

d.startDrawThread()
})
}

Expand Down
2 changes: 1 addition & 1 deletion internal/driver/glfw/loop_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,6 @@ func BenchmarkRunOnDraw(b *testing.B) {
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
runOnDraw(w, f)
runOnMainWithContext(w, f)
}
}
4 changes: 2 additions & 2 deletions internal/driver/glfw/window.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ func (w *window) doShow() {
if content := w.canvas.Content(); content != nil {
content.Show()

runOnDraw(w, func() {
runOnMainWithContext(w, func() {
w.driver.repaintWindow(w)
})
}
Expand Down Expand Up @@ -214,7 +214,7 @@ func (w *window) Close() {
}

// set w.closing flag inside draw thread to ensure we can free textures
runOnDraw(w, func() {
runOnMainWithContext(w, func() {
w.viewLock.Lock()
w.closing = true
w.viewLock.Unlock()
Expand Down
2 changes: 1 addition & 1 deletion internal/driver/glfw/window_desktop.go
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,7 @@ func (w *window) create() {
}

// run the GL init on the draw thread
runOnDraw(w, func() {
runOnMainWithContext(w, func() {
w.canvas.SetPainter(gl.NewPainter(w.canvas, w))
w.canvas.Painter().Init()
})
Expand Down
11 changes: 2 additions & 9 deletions internal/driver/glfw/window_notxdg.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,6 @@ func (w *window) platformResize(canvasSize fyne.Size) {
return
}

if drawOnMainThread {
w.canvas.Resize(canvasSize)
d.repaintWindow(w)
} else {
runOnDraw(w, func() {
w.canvas.Resize(canvasSize)
d.repaintWindow(w)
})
}
w.canvas.Resize(canvasSize)
d.repaintWindow(w)
}
2 changes: 1 addition & 1 deletion internal/driver/glfw/window_wasm.go
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ func (w *window) create() {
}

// run the GL init on the draw thread
runOnDraw(w, func() {
runOnMainWithContext(w, func() {
w.canvas.SetPainter(gl.NewPainter(w.canvas, w))
w.canvas.Painter().Init()
})
Expand Down
1 change: 1 addition & 0 deletions internal/driver/mobile/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ func (d *driver) handlePaint(e paint.Event, w *window) {
c.Painter().Init() // we cannot init until the context is set above
}

d.animation.TickAnimations()
canvasNeedRefresh := c.FreeDirtyTextures() > 0 || c.CheckDirtyAndClear()
if canvasNeedRefresh {
newSize := fyne.NewSize(float32(d.currentSize.WidthPx)/c.scale, float32(d.currentSize.HeightPx)/c.scale)
Expand Down
Loading