Make cleanup of OpenGL objects explicit in GLMakie (#4699)

* pull tweaks from #4689

* pull context validation from #4689

* handle texture atlas explicitly + fix errors

* explicitly clean up shaders & programs

* handle GLBuffer, GLVertexArray, Postprocessors

* handle Texture explicitly

* fix SSAO error

* test cleanup

* add some sanity checks

* fix tests

* cleanup finalizers, free(), unsafe_free()

* skip GLFW initialized assert for CI

* update changelog

* fix freeing of unused uniforms & cleanup unused pattern_sections

* count and check failed OpenGL cleanup

* try to handle GLFW CI failure

* try fix CI

* require context for Texture and GLBuffer in most cases

* fix incorrect uniform cleanup

* CI please

* fix typo

* try handling GLFW init errors differently

* maybe just destroy the window if the context dies?

* fix typos

* maybe also cleanup dead screens when trying to reopen?

* tweak require_context to maybe catch error earlier

* switch before requiring

* avoid requiring context for get_texture and thus robj cleanup

* treat scene finalizer

* Treat scene finalizer in texture atlas too

* add missing passthrough

* move object deletion test to end

* warn on dead context

* avoid the last few current_context() calls

* fix scatter indices

* put finalizers behind debug constant

* don't skip observable cleanup in free

* cleanup require_context

* fix typo

* fix test errors

* clean up

* fix errors

* fix freeing of GLBuffer Observables

* cleanup unsafe_free

* cleanup called_from_finalizer & add comments to implied functions

* remove try .. catch in free

* slightly safer + better verify_free

* some small cleanups

* need to switch context

* catch GC bugs early and try destroy! outside

* switch context, since those can be called from anywhere

* switch instead of require

---------

Co-authored-by: SimonDanisch <[email protected]>

Author: ffreyer

Diff for: CHANGELOG.md

@@ -29,6 +29,7 @@
 - Fixed gaps in corners of `poly(Rect2(...))` stroke [#4664](https://github.com/MakieOrg/Makie.jl/pull/4664)
 - Fixed an issue where `reinterpret`ed arrays of line points were not handled correctly in CairoMakie [#4668](https://github.com/MakieOrg/Makie.jl/pull/4668).
 - Fixed various issues with `markerspace = :data`, `transform_marker = true` and `rotation` for scatter in CairoMakie (incorrect marker transformations, ignored transformations, Cairo state corruption) [#4663](https://github.com/MakieOrg/Makie.jl/pull/4663)
+- Refactored OpenGL cleanup to run immediately rather than on GC [#4699](https://github.com/MakieOrg/Makie.jl/pull/4699)
 
 ## [0.21.18] - 2024-12-12
 

Diff for: GLMakie/src/GLAbstraction/AbstractGPUArray.jl

@@ -190,8 +190,8 @@ gpu_setindex!(t) = error("gpu_setindex! not implemented for: $(typeof(t)). This
 max_dim(t)       = error("max_dim not implemented for: $(typeof(t)). This happens, when you call setindex! on an array, without implementing the GPUArray interface")
 
 
-function (::Type{GPUArrayType})(data::Observable; kw...) where GPUArrayType <: GPUArray
-    gpu_mem = GPUArrayType(data[]; kw...)
+function (::Type{GPUArrayType})(context, data::Observable; kw...) where GPUArrayType <: GPUArray
+    gpu_mem = GPUArrayType(context, data[]; kw...)
     # TODO merge these and handle update tracking during construction
     obs2 = on(new_data -> update!(gpu_mem, new_data), data)
     if GPUArrayType <: TextureBuffer

Diff for: GLMakie/src/GLAbstraction/GLAbstraction.jl

@@ -18,6 +18,33 @@ import FixedPointNumbers: N0f8, N0f16, N0f8, Normed
 
 import Base: merge, resize!, similar, length, getindex, setindex!
 
+# Debug tools
+const GLMAKIE_DEBUG = Ref(false)
+
+# implemented in GLMakie/glwindow.jl
+function require_context_no_error(args...) end
+
+function require_context(ctx, current = ShaderAbstractions.current_context())
+    msg = require_context_no_error(ctx, current)
+    isnothing(msg) && return nothing
+    error(msg)
+end
+function with_context(f, context)
+    CTX = ShaderAbstractions.ACTIVE_OPENGL_CONTEXT
+    old_ctx = isassigned(CTX) ? CTX[] : nothing
+    GLAbstraction.switch_context!(context)
+    try
+        f()
+    finally
+        if isnothing(old_ctx)
+            GLAbstraction.switch_context!()
+        else
+            GLAbstraction.switch_context!(old_ctx)
+        end
+    end
+end
+export require_context, with_context
+
 include("AbstractGPUArray.jl")
 
 #Methods which get overloaded by GLExtendedFunctions.jl:
@@ -41,9 +68,9 @@ import ModernGL.glGetShaderiv
 import ModernGL.glViewport
 import ModernGL.glScissor
 
+include("shaderabstraction.jl")
 include("GLUtils.jl")
 
-include("shaderabstraction.jl")
 include("GLTypes.jl")
 export GLProgram                # Shader/program object
 export Texture                  # Texture object, basically a 1/2/3D OpenGL data array

Diff for: GLMakie/src/GLAbstraction/GLBuffer.jl

@@ -7,7 +7,8 @@ mutable struct GLBuffer{T} <: GPUArray{T, 1}
     # TODO maybe also delay upload to when render happens?
     observers::Vector{Observables.ObserverFunction}
 
-    function GLBuffer{T}(ptr::Ptr{T}, buff_length::Int, buffertype::GLenum, usage::GLenum) where T
+    function GLBuffer{T}(context, ptr::Ptr{T}, buff_length::Int, buffertype::GLenum, usage::GLenum) where T
+        switch_context!(context)
         id = glGenBuffers()
         glBindBuffer(buffertype, id)
         # size of 0 can segfault it seems
@@ -16,9 +17,9 @@ mutable struct GLBuffer{T} <: GPUArray{T, 1}
         glBindBuffer(buffertype, 0)
 
         obj = new(
-            id, (buff_length,), buffertype, usage, current_context(),
+            id, (buff_length,), buffertype, usage, context,
             Observables.ObserverFunction[])
-        finalizer(free, obj)
+        GLMAKIE_DEBUG[] && finalizer(verify_free, obj)
         obj
     end
 end
@@ -34,35 +35,35 @@ end
 bind(buffer::GLBuffer, other_target) = glBindBuffer(buffer.buffertype, other_target)
 
 function similar(x::GLBuffer{T}, buff_length::Int) where T
-    GLBuffer{T}(Ptr{T}(C_NULL), buff_length, x.buffertype, x.usage)
+    return GLBuffer{T}(x.context, Ptr{T}(C_NULL), buff_length, x.buffertype, x.usage)
 end
 
 cardinality(::GLBuffer{T}) where {T} = cardinality(T)
 
 #Function to deal with any Immutable type with Real as Subtype
 function GLBuffer(
-        buffer::Union{Base.ReinterpretArray{T, 1}, DenseVector{T}};
+        context, buffer::Union{Base.ReinterpretArray{T, 1}, DenseVector{T}};
         buffertype::GLenum = GL_ARRAY_BUFFER, usage::GLenum = GL_STATIC_DRAW
     ) where T <: GLArrayEltypes
     GC.@preserve buffer begin
-        return GLBuffer{T}(pointer(buffer), length(buffer), buffertype, usage)
+        return GLBuffer{T}(context, pointer(buffer), length(buffer), buffertype, usage)
     end
 end
 
 function GLBuffer(
-        buffer::DenseVector{T};
+        context, buffer::DenseVector{T};
         buffertype::GLenum = GL_ARRAY_BUFFER, usage::GLenum = GL_STATIC_DRAW
     ) where T <: GLArrayEltypes
     GC.@preserve buffer begin
-        return GLBuffer{T}(pointer(buffer), length(buffer), buffertype, usage)
+        return GLBuffer{T}(context, pointer(buffer), length(buffer), buffertype, usage)
     end
 end
 
 function GLBuffer(
-        buffer::ShaderAbstractions.Buffer{T};
+        context, buffer::ShaderAbstractions.Buffer{T};
         buffertype::GLenum = GL_ARRAY_BUFFER, usage::GLenum = GL_STATIC_DRAW
     ) where T <: GLArrayEltypes
-    b = GLBuffer(ShaderAbstractions.data(buffer); buffertype=buffertype, usage=usage)
+    b = GLBuffer(context, ShaderAbstractions.data(buffer); buffertype=buffertype, usage=usage)
     au = ShaderAbstractions.updater(buffer)
     obsfunc = on(au.update) do (f, args)
         f(b, args...) # forward setindex! etc
@@ -76,36 +77,30 @@ end
 GLBuffer(buffer::GLBuffer) = buffer
 GLBuffer{T}(buffer::GLBuffer{T}) where {T} = buffer
 
-function GLBuffer(
-        buffer::AbstractVector{T};
-        kw_args...
-    ) where T <: GLArrayEltypes
-    GLBuffer(collect(buffer); kw_args...)
+function GLBuffer(context, buffer::AbstractVector{T}; kw_args...) where T <: GLArrayEltypes
+    return GLBuffer(context, collect(buffer); kw_args...)
 end
 
-function GLBuffer{T}(
-        buffer::AbstractVector;
-        kw_args...
-    ) where T <: GLArrayEltypes
-    GLBuffer(convert(Vector{T}, buffer); kw_args...)
+function GLBuffer{T}(context, buffer::AbstractVector; kw_args...) where T <: GLArrayEltypes
+    return GLBuffer(context, convert(Vector{T}, buffer); kw_args...)
 end
 
 function GLBuffer(
-        ::Type{T}, len::Int;
+        context, ::Type{T}, len::Int;
         buffertype::GLenum = GL_ARRAY_BUFFER, usage::GLenum = GL_STATIC_DRAW
     ) where T <: GLArrayEltypes
-    GLBuffer{T}(Ptr{T}(C_NULL), len, buffertype, usage)
+    return GLBuffer{T}(context, Ptr{T}(C_NULL), len, buffertype, usage)
 end
 
 
 function indexbuffer(
-        buffer::VectorTypes{T};
-        usage::GLenum = GL_STATIC_DRAW
+        context, buffer::VectorTypes{T}; usage::GLenum = GL_STATIC_DRAW
     ) where T <: GLArrayEltypes
-    GLBuffer(buffer, buffertype = GL_ELEMENT_ARRAY_BUFFER, usage=usage)
+    return GLBuffer(context, buffer, buffertype = GL_ELEMENT_ARRAY_BUFFER, usage=usage)
 end
 # GPUArray interface
 function gpu_data(b::GLBuffer{T}) where T
+    switch_context!(b.context)
     data = Vector{T}(undef, length(b))
     bind(b)
     glGetBufferSubData(b.buffertype, 0, sizeof(data), data)
@@ -116,6 +111,7 @@ end
 
 # Resize buffer
 function gpu_resize!(buffer::GLBuffer{T}, newdims::NTuple{1, Int}) where T
+    switch_context!(buffer.context)
     #TODO make this safe!
     newlength = newdims[1]
     oldlen    = length(buffer)
@@ -139,13 +135,15 @@ function gpu_resize!(buffer::GLBuffer{T}, newdims::NTuple{1, Int}) where T
 end
 
 function gpu_setindex!(b::GLBuffer{T}, value::Vector{T}, offset::Integer) where T
+    switch_context!(b.context)
     multiplicator = sizeof(T)
     bind(b)
     glBufferSubData(b.buffertype, multiplicator*(offset-1), sizeof(value), value)
     bind(b, 0)
 end
 
 function gpu_setindex!(b::GLBuffer{T}, value::Vector{T}, offset::UnitRange{Int}) where T
+    switch_context!(b.context)
     multiplicator = sizeof(T)
     bind(b)
     glBufferSubData(b.buffertype, multiplicator*(first(offset)-1), sizeof(value), value)
@@ -156,6 +154,8 @@ end
 # copy between two buffers
 # could be a setindex! operation, with subarrays for buffers
 function unsafe_copy!(a::GLBuffer{T}, readoffset::Int, b::GLBuffer{T}, writeoffset::Int, len::Int) where T
+    switch_context!(a.context)
+    @assert a.context == b.context
     multiplicator = sizeof(T)
     @assert a.id != 0 & b.id != 0
     glBindBuffer(GL_COPY_READ_BUFFER, a.id)
@@ -178,6 +178,7 @@ end
 
 #copy inside one buffer
 function unsafe_copy!(buffer::GLBuffer{T}, readoffset::Int, writeoffset::Int, len::Int) where T
+    switch_context!(buffer.context)
     len <= 0 && return nothing
     bind(buffer)
     ptr = Ptr{T}(glMapBuffer(buffer.buffertype, GL_READ_WRITE))
@@ -190,6 +191,7 @@ function unsafe_copy!(buffer::GLBuffer{T}, readoffset::Int, writeoffset::Int, le
 end
 
 function unsafe_copy!(a::Vector{T}, readoffset::Int, b::GLBuffer{T}, writeoffset::Int, len::Int) where T
+    switch_context!(b.context)
     bind(b)
     ptr = Ptr{T}(glMapBuffer(b.buffertype, GL_WRITE_ONLY))
     for i=1:len
@@ -200,6 +202,7 @@ function unsafe_copy!(a::Vector{T}, readoffset::Int, b::GLBuffer{T}, writeoffset
 end
 
 function unsafe_copy!(a::GLBuffer{T}, readoffset::Int, b::Vector{T}, writeoffset::Int, len::Int) where T
+    switch_context!(a.context)
     bind(a)
     ptr = Ptr{T}(glMapBuffer(a.buffertype, GL_READ_ONLY))
     for i in 1:len
@@ -210,6 +213,7 @@ function unsafe_copy!(a::GLBuffer{T}, readoffset::Int, b::Vector{T}, writeoffset
 end
 
 function gpu_getindex(b::GLBuffer{T}, range::UnitRange) where T
+    switch_context!(b.context)
     multiplicator = sizeof(T)
     offset = first(range)-1
     value = Vector{T}(undef, length(range))

Diff for: GLMakie/src/GLAbstraction/GLExtendedFunctions.jl

@@ -145,18 +145,15 @@ function glGenFramebuffers()
 end
 
 function glDeleteTextures(id::GLuint)
-    arr = [id]
-    glDeleteTextures(1, arr)
+    glDeleteTextures(1, Ref(id))
 end
 
 function glDeleteVertexArrays(id::GLuint)
-    arr = [id]
-    glDeleteVertexArrays(1, arr)
+    glDeleteVertexArrays(1, Ref(id))
 end
 
 function glDeleteBuffers(id::GLuint)
-    arr = [id]
-    glDeleteBuffers(1, arr)
+    glDeleteBuffers(1, Ref(id))
 end
 
 function glGetTexLevelParameteriv(target::GLenum, level, name::GLenum)

Diff for: GLMakie/src/GLAbstraction/GLInfo.jl

@@ -97,3 +97,11 @@ function getProgramInfo(p::GLProgram)
     @show info = glGetProgramiv(program, GL_TRANSFORM_FEEDBACK_BUFFER_MODE)
     @show info = glGetProgramiv(program, GL_TRANSFORM_FEEDBACK_VARYINGS)
 end
+
+const FAILED_FREE_COUNTER = Threads.Atomic{Int}(0)
+function verify_free(obj::T, name = T) where T
+    if obj.id != 0
+        FAILED_FREE_COUNTER[] += 1
+        Core.println(Core.stderr, "Error: ", name, " has not been freed.")
+    end
+end

Diff for: GLMakie/src/GLAbstraction/GLRender.jl

@@ -14,6 +14,7 @@ function setup_clip_planes(N::Integer)
     end
 end
 
+# Note: context required in renderloop, not per renderobject here
 
 """
 When rendering a specialised list of Renderables, we can do some optimizations

Diff for: GLMakie/src/GLAbstraction/GLShader.jl

@@ -103,6 +103,20 @@ function ShaderCache(context)
     )
 end
 
+function free(cache::ShaderCache)
+    with_context(cache.context) do
+        for (k, v) in cache.shader_cache
+            for (k2, shader) in v
+                free(shader)
+            end
+        end
+        for program in values(cache.program_cache)
+            free(program)
+        end
+    end
+    return
+end
+
 abstract type AbstractLazyShader end
 
 struct LazyShader <: AbstractLazyShader
@@ -116,9 +130,9 @@ struct LazyShader <: AbstractLazyShader
     end
 end
 
-gl_convert(shader::GLProgram, data) = shader
+gl_convert(::GLContext, shader::GLProgram, data) = shader
 
-function compile_shader(source::ShaderSource, template_src::String)
+function compile_shader(context, source::ShaderSource, template_src::String)
     name = source.name
     shaderid = createshader(source.typ)
     glShaderSource(shaderid, template_src)
@@ -127,7 +141,7 @@ function compile_shader(source::ShaderSource, template_src::String)
         GLAbstraction.print_with_lines(template_src)
         @warn("shader $(name) didn't compile. \n$(GLAbstraction.getinfolog(shaderid))")
     end
-    return Shader(name, Vector{UInt8}(template_src), source.typ, shaderid)
+    return Shader(context, name, Vector{UInt8}(template_src), source.typ, shaderid)
 end
 
 
@@ -137,14 +151,14 @@ function get_shader!(cache::ShaderCache, src::ShaderSource, template_replacement
     return get!(shader_dict, template_replacement) do
         templated_source = mustache_replace(template_replacement, src.source)
         ShaderAbstractions.switch_context!(cache.context)
-        return compile_shader(src, templated_source)
+        return compile_shader(cache.context, src, templated_source)
     end::Shader
 end
 
 function get_template!(cache::ShaderCache, src::ShaderSource, view, attributes)
     return get!(cache.template_cache, src.name) do
         templated_source, replacements = template2source(src.source, view, attributes)
-        shader = compile_shader(src, templated_source)
+        shader = compile_shader(cache.context, src, templated_source)
         template_keys = collect(keys(replacements))
         template_replacements = collect(values(replacements))
         # can't yet be in here, since we didn't even have template keys
@@ -179,7 +193,7 @@ function compile_program(shaders::Vector{Shader}, fragdatalocation)
     # generate the link locations
     nametypedict = uniform_name_type(program)
     uniformlocationdict = uniformlocations(nametypedict, program)
-    GLProgram(program, shaders, nametypedict, uniformlocationdict)
+    return GLProgram(program, shaders, nametypedict, uniformlocationdict)
 end
 
 function get_view(kw_dict)
@@ -190,12 +204,13 @@ function get_view(kw_dict)
     _view
 end
 
-gl_convert(lazyshader::AbstractLazyShader, data) = error("gl_convert shader")
-function gl_convert(lazyshader::LazyShader, data)
-    gl_convert(lazyshader.shader_cache, lazyshader, data)
+gl_convert(::GLContext, lazyshader::AbstractLazyShader, data) = error("gl_convert shader")
+function gl_convert(ctx::GLContext, lazyshader::LazyShader, data)
+    gl_convert(ctx, lazyshader.shader_cache, lazyshader, data)
 end
 
-function gl_convert(cache::ShaderCache, lazyshader::AbstractLazyShader, data)
+function gl_convert(ctx::GLContext, cache::ShaderCache, lazyshader::AbstractLazyShader, data)
+    require_context(cache.context, ctx)
     kw_dict = lazyshader.kw_args
     paths = lazyshader.paths