feat: GLES Linux AdapterContext — thread-safe EGL switching (#332) - #351
feat: GLES Linux AdapterContext — thread-safe EGL switching (#332)#351lkmavi wants to merge 3 commits into
Conversation
Add mutex + LockOSThread AdapterContext on Linux matching Windows WGL parity (FEAT-GLES-003). X11/headless share Instance context; Wayland keeps Surface-owned context. Cover with unit and integration tests.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Allocate swapchain FBO via Lock(pbuffer) like Windows hidden DC, lock Destroy* before glDelete*, and serialize AdapterContext.Destroy on the mutex. Avoids Mesa pbuffer↔window invalidation and deletes without a current context after Unlock unmakes.
|
Follow-up fix pushed for review findings:
|
kolkov
left a comment
There was a problem hiding this comment.
Solid work — this is the correct architecture. Windows WGL parity achieved: mutex + LockOSThread + MakeCurrent, Instance-shared on X11/headless, Surface-owned on Wayland. The surface_linux_compat.go Phase-1 wrappers are properly removed. Tests cover ownership, nil-safety, mutex serialization, EGL Lock round-trips, and integration with real Mesa.
One blocking issue.
Blocking
Windows AdapterContext.Destroy() doesn't take the mutex
Linux AdapterContext.Destroy() (this PR) correctly acquires c.mu.Lock():
// Linux (this PR) — correct:
func (c *AdapterContext) Destroy() {
c.mu.Lock()
defer c.mu.Unlock()
if c.owns && c.eglCtx != nil {
c.eglCtx.Destroy()
...
}
}Windows adapter_context.go:161-167 does NOT:
// Windows — missing mutex:
func (c *AdapterContext) Destroy() {
if c.hglrc != 0 {
_ = wgl.MakeCurrent(0, 0)
_ = wgl.DeleteContext(c.hglrc)
c.hglrc = 0
}
}This PR adds Lock to all Device.Destroy* methods in device.go (Windows) — the same lock discipline should apply to AdapterContext.Destroy() itself. A goroutine calling Lock() while another calls Destroy() can race: wglDeleteContext destroys the context while wglMakeCurrent is about to use it.
Fix: add c.mu.Lock()/defer c.mu.Unlock() to Windows AdapterContext.Destroy() in adapter_context.go.
Non-blocking
1. Linux DestroyQuerySet stub asymmetry
PR commit 75b9b51 adds AdapterContext lock to Windows DestroyQuerySet (device.go). Linux DestroyQuerySet (device_linux.go:666) is a no-op stub. Not a bug (nothing to destroy on Linux yet), but consider adding a comment noting "stub — add Lock when GL query objects are implemented".
2. GL() contract: "Must be called while locked" vs actual usage
Windows AdapterContext.GL() docs say "Must be called while locked." But CreateShaderModule and CreateFence call d.ctx.GL() without holding the lock on both platforms. This is safe — GL() returns a pointer set at init time, and these methods don't issue GL calls. But the docstring is misleading. Consider relaxing to "Safe to read without Lock; GL calls on the returned context require Lock."
3. Codecov: 100% patch coverage
All modified and coverable lines covered. 283 LOC unit tests + 153 LOC integration tests. Well done.
Fix Windows Destroy() mutex and this is merge-ready. The Linux AdapterContext architecture is correct.
Summary
AdapterContext(Lock/LockForSurface/Unlock) withsync.Mutex+runtime.LockOSThread+eglMakeCurrent, matching Windows WGL parity (FEAT-GLES-003 / feat: GLES Linux AdapterContext — thread-safe context switching #332).surface_linux_compat.go; add unit + integration tests for ownership, nil-safe lock, mutex serialization, and EGL Lock round-trips.Test plan
GOOS=linux go test ./hal/gles/ -count=1GOOS=linux go test -tags=integration ./hal/gles/ -count=1(Mesa/EGL available)golangci-lint run --timeout=5m ./hal/gles/...Closes #332