cleanup require_context

GLMakie/src/GLAbstraction/GLAbstraction.jl

@@ -22,11 +22,15 @@ import Base: merge, resize!, similar, length, getindex, setindex!
 const GLMAKIE_DEBUG = Ref(false)
 
 require_context() = nothing # implemented in GLMakie/glwindow.jl
-function require_context_no_error(args...)
-    try
-        require_context(args...)
-    catch e
-        @error exception = (e, Base.catch_backtrace())
+function require_context_no_error(args...; async = false)
+    if async
+        require_context(args..., true)
+    else
+        try
+            require_context(args...)
+        catch e
+            @error exception = (e, Base.catch_backtrace())
+        end
     end
 end
 export require_context, check_context

GLMakie/src/GLAbstraction/GLBuffer.jl

@@ -8,6 +8,7 @@ mutable struct GLBuffer{T} <: GPUArray{T, 1}
     observers::Vector{Observables.ObserverFunction}
 
     function GLBuffer{T}(context, ptr::Ptr{T}, buff_length::Int, buffertype::GLenum, usage::GLenum) where T
+        require_context(context)
         id = glGenBuffers()
         glBindBuffer(buffertype, id)
         # size of 0 can segfault it seems
@@ -34,7 +35,6 @@ end
 bind(buffer::GLBuffer, other_target) = glBindBuffer(buffer.buffertype, other_target)
 
 function similar(x::GLBuffer{T}, buff_length::Int) where T
-    require_context(x.context)
     return GLBuffer{T}(x.context, Ptr{T}(C_NULL), buff_length, x.buffertype, x.usage)
 end
 

GLMakie/src/GLAbstraction/GLRender.jl

@@ -14,14 +14,14 @@ 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
 """
 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)
@@ -51,7 +51,6 @@ 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.
@@ -70,7 +69,6 @@ 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
@@ -94,7 +92,6 @@ 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,15 +103,17 @@ function ShaderCache(context)
     )
 end
 
-function free(cache::ShaderCache, called_from_finalizer = false)
+function free(cache::ShaderCache)
+    ShaderAbstractions.switch_context!(cache.context)
     for (k, v) in cache.shader_cache
         for (k2, shader) in v
-            free(shader, called_from_finalizer)
+            free(shader)
         end
     end
     for program in values(cache.program_cache)
-        free(program, called_from_finalizer)
+        free(program)
     end
+    require_context_no_error(cache.context) # just so we don't need try .. catch at call site
     return
 end
 

GLMakie/src/GLAbstraction/GLTexture.jl

@@ -92,7 +92,6 @@ Makie.@noconstprop function Texture(
         mipmap = false,
         parameters... # rest should be texture parameters
     ) where {T, NDim}
-    require_context(context)
     texparams = TextureParameters(T, NDim; parameters...)
     id = glGenTextures()
     glBindTexture(texturetype, id)
@@ -160,7 +159,6 @@ function Texture(
         format::GLenum         = default_colorformat(T),
         parameters...
     ) where T <: GLArrayEltypes
-    require_context(context)
     texparams = TextureParameters(T, 2; parameters...)
     id = glGenTextures()
 
@@ -188,13 +186,12 @@ function Texture(
         tuple(maxdims...)
     )
     set_parameters(texture)
-    texture
+    return texture
 end
 
 
 
 function TextureBuffer(buffer::GLBuffer{T}) where T <: GLArrayEltypes
-    require_context(buffer.context)
     texture_type = GL_TEXTURE_BUFFER
     id = glGenTextures()
     glBindTexture(texture_type, id)
@@ -359,13 +356,11 @@ gpu_getindex(t::TextureBuffer{T}, i::UnitRange{Int64}) where {T} = t.buffer[i]
 
 similar(t::Texture{T, NDim}, newdims::Int...) where {T, NDim} = similar(t, newdims)
 function similar(t::TextureBuffer{T}, newdims::NTuple{1, Int}) where T
-    require_context(t.texture.context)
     buff = similar(t.buffer, newdims...)
     return TextureBuffer(buff)
 end
 
 function similar(t::Texture{T, NDim}, newdims::NTuple{NDim, Int}) where {T, NDim}
-    require_context(t.context)
     id = glGenTextures()
     glBindTexture(t.texturetype, id)
     glTexImage(t.texturetype, 0, t.internalformat, newdims..., 0, t.format, t.pixeltype, C_NULL)

GLMakie/src/GLAbstraction/GLTypes.jl

@@ -404,6 +404,18 @@ function RenderObject(
     buffers = filter(((key, value),) -> isa(value, GLBuffer) || key === :indices, data)
     program = gl_convert(context, to_value(program), data) # "compile" lazyshader
     vertexarray = GLVertexArray(Dict(buffers), program)
+    require_context(context)
+
+    # Validate context of things in RenderObject
+    if GLMAKIE_DEBUG[]
+        require_context(program.context, context)
+        require_context(vertexarray.context, context)
+        for v in values(data)
+            if v isa GPUArray
+                require_context(v.context, context)
+            end
+        end
+    end
 
     # remove all uniforms not occurring in shader
     # ssao, instances transparency are special for rendering passes. TODO do this more cleanly
@@ -440,20 +452,19 @@ function free(x::T, called_from_finalizer = false) where {T}
     # don't free if already freed (this should only be set by unsafe_free)
     x.id == 0 && return
 
-    # This may be called from the scene finalizer in which case no errors, no printing allowed
+    # Note: context is checked higher up in the call stack but isn't guaranteed
+    #       to be active or alive here, because unsafe_free() may also do
+    #       cleanup that doesn't depend on context
+
+    # This may be called from the scene finalizer in which case no errors and
+    # no printing allowed from the current task
     if called_from_finalizer
-        if GLMAKIE_DEBUG[] && !context_alive(x.context)
-            Threads.@spawn println(stderr, "Warning: free(::$T) called with dead context from scene finalizer.")
-        end
         try
             unsafe_free(x)
         catch e
             Threads.@spawn Base.showerror(stderr, e)
         end
     else
-        # This should be called with a valid, active context, but we shouldn't error
-        # here because unsafe_free() also sometimes cleans up observables
-        require_context_no_error(x.context)
         unsafe_free(x)
     end
     return
@@ -498,7 +509,6 @@ function unsafe_free(x::Texture)
     clean_up_observables(x)
     x.id = ifelse(context_alive(x.context), x.id, 0)
     is_context_active(x.context) || return
-    id = Ref(x.id)
     glDeleteTextures(x.id)
     x.id = 0
     return

GLMakie/src/gl_backend.jl

@@ -16,14 +16,14 @@ using .GLAbstraction
 
 const atlas_texture_cache = Dict{Any, Tuple{Texture{Float16, 2}, Function}}()
 
-function cleanup_texture_atlas!(context, called_from_finalizer = false)
+function cleanup_texture_atlas!(context)
     to_delete = filter(atlas_ctx -> atlas_ctx[2] == context, keys(atlas_texture_cache))
-    called_from_finalizer || require_context(context)
     for (atlas, ctx) in to_delete
         tex, func = pop!(atlas_texture_cache, (atlas, ctx))
         Makie.remove_font_render_callback!(atlas, func)
-        GLAbstraction.free(tex, called_from_finalizer)
+        GLAbstraction.free(tex)
     end
+    GLAbstraction.require_context_no_error(context) # avoid try .. catch at call site
     return
 end
 
@@ -33,12 +33,17 @@ function get_texture!(context, atlas::Makie.TextureAtlas, called_from_finalizer
         if GLAbstraction.context_alive(ctx)
             return true
         else
-            if !called_from_finalizer
-                @error("Cached atlas textures should be removed explicitly! $ctx")
-                println("Reason:", GLFW.is_initialized() ? "" : " not initialized", was_destroyed(ctx) ? " destroyed" : "")
-                Base.show_backtrace(stderr, Base.catch_backtrace())
-            else
-                Threads.@spawn println(stderr, "Cached atlas textures did not get cleaned up for context ", ctx)
+            # Adding extra context, so no require_context_no_error(ctx, async = called_from_finalizer)
+            try
+                require_context(ctx)
+            catch e
+                if called_from_finalizer
+                    Threads.@spawn begin
+                        @error "Cached atlas textures should be removed explicitly!" exception = (e, catch_backtrace())
+                    end
+                else
+                    @error "Cached atlas textures should be removed explicitly!" exception = (e, catch_backtrace())
+                end
             end
             tex_func[1].id = 0 # Should get cleaned up when OpenGL context gets destroyed
             Makie.remove_font_render_callback!(atlas, tex_func[2])
@@ -51,7 +56,7 @@ function get_texture!(context, atlas::Makie.TextureAtlas, called_from_finalizer
     elseif called_from_finalizer
         return nothing
     else
-        require_context(context)
+        require_context(context, async = called_from_finalizer)
         tex = Texture(
             context, atlas.data,
             minfilter = :linear,

GLMakie/src/glwindow.jl

@@ -157,11 +157,19 @@ Makie.@noconstprop function GLFramebuffer(context, fb_size::NTuple{2, Int})
 end
 
 function destroy!(fb::GLFramebuffer)
-    # context required via free(tex)
-    @assert !isempty(fb.buffers)
+    fb.id == 0 && return
+    @assert !isempty(fb.buffers) "GLFramebuffer was cleared incorrectly (i.e. not by destroy!())"
+    ctx = first(values(fb.buffers)).context
+    ShaderAbstractions.switch_context!(ctx)
     for tex in values(fb.buffers)
         GLAbstraction.free(tex)
     end
+    # avoid try .. catch at call site
+    GLAbstraction.require_context_no_error(ctx)
+    if !ShaderAbstractions.is_context_active(ctx)
+        fb.id = 0
+        return
+    end
     id = [fb.id]
     glDeleteFramebuffers(1, id)
     fb.id = 0
@@ -219,11 +227,22 @@ function ShaderAbstractions.native_context_alive(x::GLFW.Window)
     GLFW.is_initialized() && !was_destroyed(x)
 end
 
-# require_context(ctx, current = nothing) = nothing
-function GLAbstraction.require_context(ctx, current = ShaderAbstractions.current_context())
-    @assert GLFW.is_initialized() "Context $ctx must be initialized, but is not."
-    @assert !was_destroyed(ctx) "Context $ctx must not be destroyed."
-    @assert ctx.handle == current.handle "Context $ctx must be current, but $current is."
+function GLAbstraction.require_context(ctx, current = ShaderAbstractions.current_context(); async = false)
+    if async
+        if !GLFW.is_initialized()
+            Threads.@spawn "Failed to require context:" exception = (ErrorException("Context $ctx must be initialized, but is not."), backtrace())
+        end
+        if GLAbstraction.GLMAKIE_DEBUG[] && was_destroyed(ctx)
+            Threads.@spawn "Failed to require context:" exception = (ErrorException("Context $ctx must not be destroyed."), backtrace())
+        end
+        if ctx != current
+            Threads.@spawn "Failed to require context:" exception = (ErrorException("Context $ctx must be current, but $current is."), backtrace())
+        end
+    else
+        @assert GLFW.is_initialized() "Context $ctx must be initialized, but is not."
+        @assert !GLAbstraction.GLMAKIE_DEBUG[] || !was_destroyed(ctx) "Context $ctx must not be destroyed."
+        @assert ctx == current "Context $ctx must be current, but $current is."
+    end
 end
 
 function destroy!(nw::GLFW.Window)

GLMakie/src/postprocessing.jl

@@ -80,7 +80,8 @@ end
 
 function ssao_postprocessor(framebuffer, shader_cache)
     ShaderAbstractions.switch_context!(shader_cache.context)
-    require_context(shader_cache.context)
+    require_context(shader_cache.context) # for framebuffer, uniform textures
+
     # Add missing buffers
     if !haskey(framebuffer, :position)
         glBindFramebuffer(GL_FRAMEBUFFER, framebuffer.id[1])
@@ -208,7 +209,7 @@ Returns a PostProcessor that handles fxaa.
 """
 function fxaa_postprocessor(framebuffer, shader_cache)
     ShaderAbstractions.switch_context!(shader_cache.context)
-    require_context(shader_cache.context)
+    require_context(shader_cache.context) # for framebuffer, uniform textures
 
     # Add missing buffers
     if !haskey(framebuffer, :color_luma)

GLMakie/src/rendering.jl

@@ -118,7 +118,6 @@ end
 function GLAbstraction.render(filter_elem_func, screen::Screen)
     # Somehow errors in here get ignored silently!?
     try
-        GLAbstraction.require_context(screen.glscreen)
         for (zindex, screenid, elem) in screen.renderlist
             filter_elem_func(elem)::Bool || continue
 
@@ -130,7 +129,6 @@ function GLAbstraction.render(filter_elem_func, screen::Screen)
             glViewport(round.(Int, ppu .* minimum(a))..., round.(Int, ppu .* widths(a))...)
             render(elem)
         end
-        GLAbstraction.require_context(screen.glscreen)
     catch e
         @error "Error while rendering!" exception = e
         rethrow(e)

GLMakie/src/screen.jl

@@ -579,7 +579,7 @@ end
 function destroy!(rob::RenderObject, called_from_finalizer = false)
     # These need explicit clean up because (some of) the source observables
     # remain when the plot is deleted.
-    GLAbstraction.switch_context!(rob.context)
+    ShaderAbstractions.switch_context!(rob.context)
     tex = get_texture!(rob.context, gl_texture_atlas(), called_from_finalizer)
     for (k, v) in rob.uniforms
         if v isa Observable
@@ -598,6 +598,8 @@ function destroy!(rob::RenderObject, called_from_finalizer = false)
         Observables.clear(obs)
     end
     GLAbstraction.free(rob.vertexarray, called_from_finalizer)
+    # avoid try .. catch at call site (call site usually destroys multipel renderobjects)
+    GLAbstraction.require_context_no_error(rob.context; async = called_from_finalizer)
 end
 
 # Note: called from scene finalizer, must not error
@@ -663,16 +665,20 @@ function destroy!(screen::Screen)
         empty!(screen)
     end
     @assert screen.rendertask === nothing
-    foreach(destroy!, screen.postprocessors) # before texture atlas, otherwise it regenerates
-    destroy!(screen.framebuffer)
-    cleanup_texture_atlas!(window)
-    GLAbstraction.free(screen.shader_cache)
-    # Since those are sets, we can just delete them from there, even if they weren't in there (e.g. reuse=false)
+
+    # Since those are sets, we can just delete them from there, even if they
+    # weren't in there (e.g. reuse=false)
     delete!(SCREEN_REUSE_POOL, screen)
     delete!(ALL_SCREENS, screen)
     if screen in SINGLETON_SCREEN
         empty!(SINGLETON_SCREEN)
     end
+
+    foreach(destroy!, screen.postprocessors) # before texture atlas, otherwise it regenerates
+    destroy!(screen.framebuffer)
+    cleanup_texture_atlas!(window)
+    GLAbstraction.free(screen.shader_cache)
+
     destroy!(window)
     return
 end
@@ -685,12 +691,13 @@ Doesn't destroy the screen and instead frees it to be re-used again, if `reuse=t
 """
 function Base.close(screen::Screen; reuse=true)
     @debug("Close screen!")
+
+    # If the context is dead we should completely destroy the screen
     if !GLAbstraction.context_alive(screen.glscreen)
         destroy!(screen)
+        return
     end
 
-    # TODO: CI sometimes fails to adjust visibility with a GLFW init error
-    # This should not stop us from cleaning up OpenGL objects!
     set_screen_visibility!(screen, false)
     if screen.window_open[] # otherwise we trigger an infinite loop of closing
         screen.window_open[] = false