From 3c2d5629674c47f5c389ea05128addf155b4e586 Mon Sep 17 00:00:00 2001 From: Hajime Hoshi Date: Sun, 13 Jun 2021 21:43:23 +0900 Subject: [PATCH] internal/glfw: Bug fix: NewCallbackCDecl objects were leaked Callbacks created by NewCallbackCDecl were never released and then they are leaked, especially when the window size was changed by SetWindowSize on Windows. This change defines new callback ID types with uintptr, and reuse the callbacks. Closes #1672 --- internal/glfw/callback_notwindows.go | 82 +++++++++++++++++++++ internal/glfw/callback_windows.go | 61 ++++++++++++++++ internal/glfw/glfw_notwindows.go | 38 ++-------- internal/glfw/glfw_windows.go | 44 ++--------- internal/glfw/type.go | 8 +- internal/uidriver/glfw/input.go | 8 +- internal/uidriver/glfw/ui.go | 105 +++++++++++++++------------ 7 files changed, 223 insertions(+), 123 deletions(-) create mode 100644 internal/glfw/callback_notwindows.go create mode 100644 internal/glfw/callback_windows.go diff --git a/internal/glfw/callback_notwindows.go b/internal/glfw/callback_notwindows.go new file mode 100644 index 000000000..686027892 --- /dev/null +++ b/internal/glfw/callback_notwindows.go @@ -0,0 +1,82 @@ +// Copyright 2021 The Ebiten 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 !windows +// +build !windows + +package glfw + +import ( + "github.com/go-gl/glfw/v3.3/glfw" +) + +var ( + /*charModsCallbacks = map[CharModsCallback]func(window *Window, char rune, mods ModifierKey){} + framebufferSizeCallbacks = map[FramebufferSizeCallback]func(window *Window, width int, height int){} + scrollCallbacks = map[ScrollCallback]func(window *Window, xoff float64, yoff float64){} + sizeCallbacks = map[SizeCallback]func(window *Window, width int, height int){}*/ + + charModsCallbacks = map[CharModsCallback]glfw.CharModsCallback{} + framebufferSizeCallbacks = map[FramebufferSizeCallback]glfw.FramebufferSizeCallback{} + scrollCallbacks = map[ScrollCallback]glfw.ScrollCallback{} + sizeCallbacks = map[SizeCallback]glfw.SizeCallback{} +) + +func ToCharModsCallback(cb func(window *Window, char rune, mods ModifierKey)) CharModsCallback { + if cb == nil { + return 0 + } + id := CharModsCallback(len(charModsCallbacks) + 1) + var gcb glfw.CharModsCallback = func(window *glfw.Window, char rune, mods glfw.ModifierKey) { + cb(theWindows.get(window), char, ModifierKey(mods)) + } + charModsCallbacks[id] = gcb + return id +} + +func ToFramebufferSizeCallback(cb func(window *Window, width int, height int)) FramebufferSizeCallback { + if cb == nil { + return 0 + } + id := FramebufferSizeCallback(len(framebufferSizeCallbacks) + 1) + var gcb glfw.FramebufferSizeCallback = func(window *glfw.Window, width int, height int) { + cb(theWindows.get(window), width, height) + } + framebufferSizeCallbacks[id] = gcb + return id +} + +func ToScrollCallback(cb func(window *Window, xoff float64, yoff float64)) ScrollCallback { + if cb == nil { + return 0 + } + id := ScrollCallback(len(scrollCallbacks) + 1) + var gcb glfw.ScrollCallback = func(window *glfw.Window, xoff float64, yoff float64) { + cb(theWindows.get(window), xoff, yoff) + } + scrollCallbacks[id] = gcb + return id +} + +func ToSizeCallback(cb func(window *Window, width int, height int)) SizeCallback { + if cb == nil { + return 0 + } + id := SizeCallback(len(sizeCallbacks) + 1) + var gcb glfw.SizeCallback = func(window *glfw.Window, width, height int) { + cb(theWindows.get(window), width, height) + } + sizeCallbacks[id] = gcb + return id +} diff --git a/internal/glfw/callback_windows.go b/internal/glfw/callback_windows.go new file mode 100644 index 000000000..30679ed44 --- /dev/null +++ b/internal/glfw/callback_windows.go @@ -0,0 +1,61 @@ +// Copyright 2021 The Ebiten 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. + +package glfw + +import ( + "golang.org/x/sys/windows" +) + +func ToCharModsCallback(cb func(window *Window, char rune, mods ModifierKey)) CharModsCallback { + if cb == nil { + return 0 + } + return CharModsCallback(windows.NewCallbackCDecl(func(window uintptr, char rune, mods ModifierKey) uintptr { + cb(theGLFWWindows.get(window), char, mods) + return 0 + })) +} + +func ToFramebufferSizeCallback(cb func(window *Window, width int, height int)) FramebufferSizeCallback { + if cb == nil { + return 0 + } + return FramebufferSizeCallback(windows.NewCallbackCDecl(func(window uintptr, width int, height int) uintptr { + cb(theGLFWWindows.get(window), width, height) + return 0 + })) +} + +func ToScrollCallback(cb func(window *Window, xoff float64, yoff float64)) ScrollCallback { + if cb == nil { + return 0 + } + return ScrollCallback(windows.NewCallbackCDecl(func(window uintptr, xoff *float64, yoff *float64) uintptr { + // xoff and yoff were originally float64, but there is no good way to pass them on 32bit + // machines via NewCallback. We've fixed GLFW side to use pointer values. + cb(theGLFWWindows.get(window), *xoff, *yoff) + return 0 + })) +} + +func ToSizeCallback(cb func(window *Window, width int, height int)) SizeCallback { + if cb == nil { + return 0 + } + return SizeCallback(windows.NewCallbackCDecl(func(window uintptr, width int, height int) uintptr { + cb(theGLFWWindows.get(window), width, height) + return 0 + })) +} diff --git a/internal/glfw/glfw_notwindows.go b/internal/glfw/glfw_notwindows.go index dcec6bc8b..66bd68eb6 100644 --- a/internal/glfw/glfw_notwindows.go +++ b/internal/glfw/glfw_notwindows.go @@ -162,14 +162,8 @@ func (w *Window) SetAttrib(attrib Hint, value int) { } func (w *Window) SetCharModsCallback(cbfun CharModsCallback) (previous CharModsCallback) { - var gcb glfw.CharModsCallback - if cbfun != nil { - gcb = func(window *glfw.Window, char rune, mods glfw.ModifierKey) { - cbfun(theWindows.get(window), char, ModifierKey(mods)) - } - } - w.w.SetCharModsCallback(gcb) - return nil // TODO + w.w.SetCharModsCallback(charModsCallbacks[cbfun]) + return 0 // TODO } func (w *Window) SetCursor(cursor *Cursor) { @@ -181,35 +175,17 @@ func (w *Window) SetCursor(cursor *Cursor) { } func (w *Window) SetFramebufferSizeCallback(cbfun FramebufferSizeCallback) (previous FramebufferSizeCallback) { - var gcb glfw.FramebufferSizeCallback - if cbfun != nil { - gcb = func(window *glfw.Window, width int, height int) { - cbfun(theWindows.get(window), width, height) - } - } - w.w.SetFramebufferSizeCallback(gcb) - return nil // TODO + w.w.SetFramebufferSizeCallback(framebufferSizeCallbacks[cbfun]) + return 0 // TODO } func (w *Window) SetScrollCallback(cbfun ScrollCallback) (previous ScrollCallback) { - var gcb glfw.ScrollCallback - if cbfun != nil { - gcb = func(window *glfw.Window, xoff float64, yoff float64) { - cbfun(theWindows.get(window), xoff, yoff) - } - } - w.w.SetScrollCallback(gcb) - return nil // TODO + w.w.SetScrollCallback(scrollCallbacks[cbfun]) + return 0 // TODO } func (w *Window) SetSizeCallback(cbfun SizeCallback) (previous SizeCallback) { - var gcb glfw.SizeCallback - if cbfun != nil { - gcb = func(window *glfw.Window, width, height int) { - cbfun(theWindows.get(window), width, height) - } - } - w.w.SetSizeCallback(gcb) + w.w.SetSizeCallback(sizeCallbacks[cbfun]) prev := w.prevSizeCallback w.prevSizeCallback = cbfun return prev diff --git a/internal/glfw/glfw_windows.go b/internal/glfw/glfw_windows.go index bcecd536a..379e18b9b 100644 --- a/internal/glfw/glfw_windows.go +++ b/internal/glfw/glfw_windows.go @@ -196,16 +196,9 @@ func (w *Window) Restore() { } func (w *Window) SetCharModsCallback(cbfun CharModsCallback) (previous CharModsCallback) { - var gcb uintptr - if cbfun != nil { - gcb = windows.NewCallbackCDecl(func(window uintptr, char rune, mods ModifierKey) uintptr { - cbfun(theGLFWWindows.get(window), char, mods) - return 0 - }) - } - glfwDLL.call("glfwSetCharModsCallback", w.w, gcb) + glfwDLL.call("glfwSetCharModsCallback", w.w, uintptr(cbfun)) panicError() - return nil // TODO + return ToCharModsCallback(nil) // TODO } func (w *Window) SetCursor(cursor *Cursor) { @@ -217,42 +210,19 @@ func (w *Window) SetCursor(cursor *Cursor) { } func (w *Window) SetFramebufferSizeCallback(cbfun FramebufferSizeCallback) (previous FramebufferSizeCallback) { - var gcb uintptr - if cbfun != nil { - gcb = windows.NewCallbackCDecl(func(window uintptr, width int, height int) uintptr { - cbfun(theGLFWWindows.get(window), width, height) - return 0 - }) - } - glfwDLL.call("glfwSetFramebufferSizeCallback", w.w, gcb) + glfwDLL.call("glfwSetFramebufferSizeCallback", w.w, uintptr(cbfun)) panicError() - return nil // TODO + return ToFramebufferSizeCallback(nil) // TODO } func (w *Window) SetScrollCallback(cbfun ScrollCallback) (previous ScrollCallback) { - var gcb uintptr - if cbfun != nil { - gcb = windows.NewCallbackCDecl(func(window uintptr, xoff *float64, yoff *float64) uintptr { - // xoff and yoff were originally float64, but there is no good way to pass them on 32bit - // machines via NewCallback. We've fixed GLFW side to use pointer values. - cbfun(theGLFWWindows.get(window), *xoff, *yoff) - return 0 - }) - } - glfwDLL.call("glfwSetScrollCallback", w.w, gcb) + glfwDLL.call("glfwSetScrollCallback", w.w, uintptr(cbfun)) panicError() - return nil // TODO + return ToScrollCallback(nil) // TODO } func (w *Window) SetSizeCallback(cbfun SizeCallback) (previous SizeCallback) { - var gcb uintptr - if cbfun != nil { - gcb = windows.NewCallbackCDecl(func(window uintptr, width int, height int) uintptr { - cbfun(theGLFWWindows.get(window), width, height) - return 0 - }) - } - glfwDLL.call("glfwSetWindowSizeCallback", w.w, gcb) + glfwDLL.call("glfwSetWindowSizeCallback", w.w, uintptr(cbfun)) panicError() prev := w.prevSizeCallback w.prevSizeCallback = cbfun diff --git a/internal/glfw/type.go b/internal/glfw/type.go index 3f45e3463..c7c767dba 100644 --- a/internal/glfw/type.go +++ b/internal/glfw/type.go @@ -18,10 +18,10 @@ package glfw type ( - CharModsCallback func(w *Window, char rune, mods ModifierKey) - FramebufferSizeCallback func(w *Window, width int, height int) - ScrollCallback func(w *Window, xoff float64, yoff float64) - SizeCallback func(w *Window, width int, height int) + CharModsCallback uintptr + FramebufferSizeCallback uintptr + ScrollCallback uintptr + SizeCallback uintptr ) type VidMode struct { diff --git a/internal/uidriver/glfw/input.go b/internal/uidriver/glfw/input.go index dfad43822..9f262748f 100644 --- a/internal/uidriver/glfw/input.go +++ b/internal/uidriver/glfw/input.go @@ -273,7 +273,7 @@ func (i *Input) update(window *glfw.Window, context driver.UIContext) { defer i.ui.m.Unlock() i.onceCallback.Do(func() { - window.SetCharModsCallback(func(w *glfw.Window, char rune, mods glfw.ModifierKey) { + window.SetCharModsCallback(glfw.ToCharModsCallback(func(w *glfw.Window, char rune, mods glfw.ModifierKey) { // As this function is called from GLFW callbacks, the current thread is main. if !unicode.IsPrint(char) { return @@ -282,14 +282,14 @@ func (i *Input) update(window *glfw.Window, context driver.UIContext) { i.ui.m.Lock() defer i.ui.m.Unlock() i.runeBuffer = append(i.runeBuffer, char) - }) - window.SetScrollCallback(func(w *glfw.Window, xoff float64, yoff float64) { + })) + window.SetScrollCallback(glfw.ToScrollCallback(func(w *glfw.Window, xoff float64, yoff float64) { // As this function is called from GLFW callbacks, the current thread is main. i.ui.m.Lock() defer i.ui.m.Unlock() i.scrollX = xoff i.scrollY = yoff - }) + })) }) if i.keyPressed == nil { i.keyPressed = map[glfw.Key]bool{} diff --git a/internal/uidriver/glfw/ui.go b/internal/uidriver/glfw/ui.go index 0566e60bb..d374ce016 100644 --- a/internal/uidriver/glfw/ui.go +++ b/internal/uidriver/glfw/ui.go @@ -107,6 +107,10 @@ type UserInterface struct { input Input iwindow window + sizeCallback glfw.SizeCallback + framebufferSizeCallback glfw.FramebufferSizeCallback + framebufferSizeCallbackCh chan struct{} + t thread.Thread m sync.RWMutex } @@ -726,47 +730,50 @@ func (u *UserInterface) createWindow() error { // registerWindowSetSizeCallback must be called from the main thread. func (u *UserInterface) registerWindowSetSizeCallback() { - u.window.SetSizeCallback(func(_ *glfw.Window, width, height int) { - if !u.setSizeCallbackEnabled { - return - } - - if u.window.GetAttrib(glfw.Resizable) == glfw.False { - return - } - if u.isFullscreen() { - return - } - - if err := u.runOnAnotherThreadFromMainThread(func() error { - var outsideWidth, outsideHeight float64 - var outsideSizeChanged bool - - _ = u.t.Call(func() error { - if width != 0 || height != 0 { - u.setWindowSize(width, height, u.isFullscreen()) - } - - outsideWidth, outsideHeight, outsideSizeChanged = u.updateSize() - return nil - }) - if outsideSizeChanged { - u.context.Layout(outsideWidth, outsideHeight) + if u.sizeCallback == 0 { + u.sizeCallback = glfw.ToSizeCallback(func(_ *glfw.Window, width, height int) { + if !u.setSizeCallbackEnabled { + return } - if err := u.context.ForceUpdate(); err != nil { - return err + + if u.window.GetAttrib(glfw.Resizable) == glfw.False { + return } - if u.Graphics().IsGL() { + if u.isFullscreen() { + return + } + + if err := u.runOnAnotherThreadFromMainThread(func() error { + var outsideWidth, outsideHeight float64 + var outsideSizeChanged bool + _ = u.t.Call(func() error { - u.swapBuffers() + if width != 0 || height != 0 { + u.setWindowSize(width, height, u.isFullscreen()) + } + + outsideWidth, outsideHeight, outsideSizeChanged = u.updateSize() return nil }) + if outsideSizeChanged { + u.context.Layout(outsideWidth, outsideHeight) + } + if err := u.context.ForceUpdate(); err != nil { + return err + } + if u.Graphics().IsGL() { + _ = u.t.Call(func() error { + u.swapBuffers() + return nil + }) + } + return nil + }); err != nil { + u.err = err } - return nil - }); err != nil { - u.err = err - } - }) + }) + } + u.window.SetSizeCallback(u.sizeCallback) } func (u *UserInterface) init() error { @@ -1210,15 +1217,18 @@ func (u *UserInterface) setWindowSize(width, height int, fullscreen bool) { newW := width newH := height if oldW != newW || oldH != newH { - ch := make(chan struct{}, 1) - u.window.SetFramebufferSizeCallback(func(_ *glfw.Window, _, _ int) { - // This callback can be invoked multiple times by one PollEvents in theory (#1618). - // Allow the case when the channel is full. - select { - case ch <- struct{}{}: - default: - } - }) + u.framebufferSizeCallbackCh = make(chan struct{}, 1) + if u.framebufferSizeCallback == 0 { + u.framebufferSizeCallback = glfw.ToFramebufferSizeCallback(func(_ *glfw.Window, _, _ int) { + // This callback can be invoked multiple times by one PollEvents in theory (#1618). + // Allow the case when the channel is full. + select { + case u.framebufferSizeCallbackCh <- struct{}{}: + default: + } + }) + } + u.window.SetFramebufferSizeCallback(u.framebufferSizeCallback) u.window.SetSize(newW, newH) // Just after SetSize, GetSize is not reliable especially on Linux/UNIX. // Let's wait for FramebufferSize callback in any cases. @@ -1231,7 +1241,7 @@ func (u *UserInterface) setWindowSize(width, height int, fullscreen bool) { for { glfw.PollEvents() select { - case <-ch: + case <-u.framebufferSizeCallbackCh: break event case <-t.C: break event @@ -1239,8 +1249,9 @@ func (u *UserInterface) setWindowSize(width, height int, fullscreen bool) { time.Sleep(time.Millisecond) } } - u.window.SetFramebufferSizeCallback(nil) - close(ch) + u.window.SetFramebufferSizeCallback(glfw.ToFramebufferSizeCallback(nil)) + close(u.framebufferSizeCallbackCh) + u.framebufferSizeCallbackCh = nil } // Window title might be lost on macOS after coming back from fullscreen.