Make cleanup of OpenGL objects explicit in GLMakie

CHANGELOG.md

@@ -28,6 +28,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
 

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

GLMakie/src/GLAbstraction/GLAbstraction.jl

@@ -18,6 +18,10 @@ import FixedPointNumbers: N0f8, N0f16, N0f8, Normed
 
 import Base: merge, resize!, similar, length, getindex, setindex!
 
+# Debug tools
+require_context() = nothing # implemented in GLMakie/glwindow
+export require_context
+
 include("AbstractGPUArray.jl")
 
 #Methods which get overloaded by GLExtendedFunctions.jl:

GLMakie/src/GLAbstraction/GLBuffer.jl

@@ -7,7 +7,7 @@ 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
         id = glGenBuffers()
         glBindBuffer(buffertype, id)
         # size of 0 can segfault it seems
@@ -16,9 +16,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)
+        finalizer(verify_free, obj)
         obj
     end
 end
@@ -34,35 +34,36 @@ 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)
+    require_context(x.context)
+    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
+        buffer::VectorTypes{T}; usage::GLenum = GL_STATIC_DRAW
     ) where T <: GLArrayEltypes
-    GLBuffer(buffer, buffertype = GL_ELEMENT_ARRAY_BUFFER, usage=usage)
+    return GLBuffer(current_context(), buffer, buffertype = GL_ELEMENT_ARRAY_BUFFER, usage=usage)
 end
 # GPUArray interface
 function gpu_data(b::GLBuffer{T}) where T
+    require_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
+    require_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
+    require_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
+    require_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
+    require_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
+    require_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
+    require_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
+    require_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
+    require_context(b.context)
     multiplicator = sizeof(T)
     offset = first(range)-1
     value = Vector{T}(undef, length(range))

GLMakie/src/GLAbstraction/GLInfo.jl

@@ -97,3 +97,12 @@ 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 = Ref(0)
+
+function verify_free(obj::T, name = string(T)) where T
+    if obj.id != 0
+        FAILED_FREE_COUNTER[] = FAILED_FREE_COUNTER[] + 1
+        Threads.@spawn println(stderr, "Error: $name has not been freed.")
+    end
+end

GLMakie/src/GLAbstraction/GLRender.jl

@@ -21,6 +21,7 @@ When rendering a specialised list of Renderables, we can do some optimizations
 function render(list::Vector{RenderObject{Pre}}) where Pre
     isempty(list) && return nothing
     first(list).prerenderfunction()
+    require_context(first(list).context)
     vertexarray = first(list).vertexarray
     program = vertexarray.program
     glUseProgram(program.id)
@@ -50,6 +51,7 @@ function render(list::Vector{RenderObject{Pre}}) where Pre
             end
         end
         renderobject.postrenderfunction()
+        require_context(renderobject.context)
     end
     # we need to assume, that we're done here, which is why
     # we need to bind VertexArray to 0.
@@ -68,6 +70,7 @@ a lot of objects.
 """
 function render(renderobject::RenderObject, vertexarray=renderobject.vertexarray)
     if renderobject.visible
+        require_context(renderobject.context)
         renderobject.prerenderfunction()
         setup_clip_planes(to_value(get(renderobject.uniforms, :num_clip_planes, 0)))
         program = vertexarray.program
@@ -91,6 +94,7 @@ function render(renderobject::RenderObject, vertexarray=renderobject.vertexarray
         bind(vertexarray)
         renderobject.postrenderfunction()
         glBindVertexArray(0)
+        require_context(renderobject.context)
     end
     return
 end

GLMakie/src/GLAbstraction/GLShader.jl

@@ -103,6 +103,18 @@ function ShaderCache(context)
     )
 end
 
+function free(cache::ShaderCache)
+    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
+    return
+end
+
 abstract type AbstractLazyShader end
 
 struct LazyShader <: AbstractLazyShader
@@ -118,7 +130,7 @@ end
 
 gl_convert(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 +139,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(name, Vector{UInt8}(template_src), source.typ, shaderid, context)
 end
 
 
@@ -137,14 +149,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