Dispose VBOs created during mipmap generation

[smoogipoo]

I don't think this would've caused any problems because these are periodically free'd via

osu-framework/osu.Framework/Graphics/Rendering/Renderer.cs

Lines 811 to 823 in 7c5b248

	private void freeUnusedVertexBuffers()
	{
	if (ResetId % vbo_free_check_interval != 0)
	return;
	foreach (var buf in vertexBuffersInUse)
	{
	if (buf.InUse && ResetId - buf.LastUseResetId > vbo_free_check_interval)
	buf.Free();
	}
	vertexBuffersInUse.RemoveAll(b => !b.InUse);
	}

[bdach]

This branch somehow is causing segfaults on my linux setup (ubuntu 22.04, NVIDIA GeForce RTX 3060 laptop version, nvidia proprietary drivers, legacy gl) that are not there on master:

[11570.418684] Draw (GameThrea[102898]: segfault at 0 ip 00007fa7fa9ea600 sp 00007fa800df8ae0 error 4 in libnvidia-glcore.so.525.105.17[7fa7f96fc000+2505000]
[11570.418695] Code: f8 41 8d 57 ff 44 29 fb c1 e0 10 25 00 00 ff 1f 0d 7c 05 00 60 89 01 48 8d 41 04 48 8d 4c 91 08 4c 89 e2 0f 1f 80 00 00 00 00 <0f> b7 32 48 83 c0 04 48 83 c2 04 44 01 f6 66 89 70 fc 0f b7 72 fe

Not sure where to even begin with this, but I'll have a poke...

[bdach]

Following patch appears to fix the segfault:

diff --git a/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs b/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
index 524fa8d57..4d66540b4 100644
--- a/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
+++ b/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
@@ -81,7 +81,7 @@ protected virtual void Initialise()
 
         public void Dispose()
         {
-            Dispose(true);
+            Renderer.ScheduleDisposal(v => v.Dispose(true), this);
             GC.SuppressFinalize(this);
         }
 

I would be lying if I said I'm entirely following why, but that was the primary difference between GLVertexBuffer's and VeldridVertexBuffer's disposal. And the GLVertexBuffer destructor does that too, so it seems correct?

[smoogipoo]

Hmm, that's pretty weird. It should definitely be a valid operation to delete a VBO after a glDrawX... Does it also fail if VeldridVertexBuffer is changed to dispose immediately?

Or… are you able to repro in debug? Can you check if it fails on the glDelete calls or potentially a call later on?

[bdach]

Does it also fail if VeldridVertexBuffer is changed to dispose immediately?

Removing the schedule for veldrid vertex buffer does not seem to reproduce the same segfault, no. I have no clue why this is the case, but that's what I'm seeing.

Or… are you able to repro in debug? Can you check if it fails on the glDelete calls or potentially a call later on?

Debug doesn't do much here since it's a segfault, and so the kernel just kills the game process. The disposal completes just fine (glDeleteX is called both on the VBO and VAO without issue), and the game gets killed at an indeterminate time after that. What I can say, though, is that if I do something along the lines of

diff --git a/osu.Framework/Graphics/OpenGL/Textures/GLTexture.cs b/osu.Framework/Graphics/OpenGL/Textures/GLTexture.cs
index 904f20812..175fee0b8 100644
--- a/osu.Framework/Graphics/OpenGL/Textures/GLTexture.cs
+++ b/osu.Framework/Graphics/OpenGL/Textures/GLTexture.cs
@@ -314,8 +314,8 @@ public unsafe bool Upload()
                         // Perform the actual mip level draw
                         Renderer.PushViewport(new RectangleI(0, 0, width, height));
 
-                        quadBuffer.Update();
-                        quadBuffer.Draw();
+                        //quadBuffer.Update();
+                        //quadBuffer.Draw();
 
                         Renderer.PopViewport();
                     }

then the game looks broken (because mipmaps are now empty), but at least it stops segfaulting. So to my uneducated eye this appears to be an indication that the nvidia driver might be reordering gl calls such that both the update and draw calls end up being called after the VBO is disposed causing it to panic. It is also both calls that need to be commented out to observe this, commenting just any one of those two without commenting the other still causes a crash.

[frenzibyte]

You could try inserting a glFinish after both calls to see if that's the case.

Another thing comes to mind is if VAOs should be unbound before deleting it. Veldrid for example generates one VAO and always keeps it bound for the lifetime of the graphics device. It would make sense why it doesn't fail when scheduling the disposal, because another VAO would've already been bound before deleting the current one.

Try this and see if it helps:

diff --git a/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs b/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
index 524fa8d57..f94d2b0f3 100644
--- a/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
+++ b/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
@@ -177,6 +177,8 @@ void IVertexBuffer.Free()
         {
             if (isInitialised)
             {
+                GL.BindVertexArray(0);
+
                 GL.DeleteBuffer(vboId);
                 GL.DeleteVertexArray(vaoId);
             }

Or:

diff --git a/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs b/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
index 524fa8d57..0fa00d9b4 100644
--- a/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
+++ b/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
@@ -114,6 +114,7 @@ public void Bind(bool forRendering)
 
         public virtual void Unbind()
         {
+            Renderer.BindVertexArray(0);
         }
 
         protected virtual int ToElements(int vertices) => vertices;
@@ -133,6 +134,8 @@ public void DrawRange(int startIndex, int endIndex)
 
             int countVertices = endIndex - startIndex;
             GL.DrawElements(Type, ToElements(countVertices), DrawElementsType.UnsignedShort, (IntPtr)(ToElementIndex(startIndex) * sizeof(ushort)));
+
+            Unbind();
         }
 
         public void Update()
@@ -150,6 +153,8 @@ public void UpdateRange(int startIndex, int endIndex)
             GL.BufferSubData(BufferTarget.ArrayBuffer, (IntPtr)(startIndex * STRIDE), (IntPtr)(countVertices * STRIDE), ref getMemory().Span[startIndex]);
 
             FrameStatistics.Add(StatisticsCounterType.VerticesUpl, countVertices);
+
+            Unbind();
         }
 
         private ref Memory<DepthWrappingVertex<T>> getMemory()

or maybe just generate one VAO and bind it for the lifetime of the renderer.

[bdach]

You could try inserting a glFinish after both calls to see if that's the case.

I tried applying this:

diff --git a/osu.Framework/Graphics/OpenGL/Textures/GLTexture.cs b/osu.Framework/Graphics/OpenGL/Textures/GLTexture.cs
index 904f20812..3b081c2aa 100644
--- a/osu.Framework/Graphics/OpenGL/Textures/GLTexture.cs
+++ b/osu.Framework/Graphics/OpenGL/Textures/GLTexture.cs
@@ -316,6 +316,7 @@ public unsafe bool Upload()
 
                         quadBuffer.Update();
                         quadBuffer.Draw();
+                        GL.Finish();
 
                         Renderer.PopViewport();
                     }

and it still segfaults.

Try this and see if it helps:

diff --git a/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs b/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
index 524fa8d57..f94d2b0f3 100644
--- a/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
+++ b/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
@@ -177,6 +177,8 @@ void IVertexBuffer.Free()
         {
             if (isInitialised)
             {
+                GL.BindVertexArray(0);
+
                 GL.DeleteBuffer(vboId);
                 GL.DeleteVertexArray(vaoId);
             }

This one doesn't work either, but:

diff --git a/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs b/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
index 524fa8d57..0fa00d9b4 100644
--- a/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
+++ b/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
@@ -114,6 +114,7 @@ public void Bind(bool forRendering)
 
         public virtual void Unbind()
         {
+            Renderer.BindVertexArray(0);
         }
 
         protected virtual int ToElements(int vertices) => vertices;
@@ -133,6 +134,8 @@ public void DrawRange(int startIndex, int endIndex)
 
             int countVertices = endIndex - startIndex;
             GL.DrawElements(Type, ToElements(countVertices), DrawElementsType.UnsignedShort, (IntPtr)(ToElementIndex(startIndex) * sizeof(ushort)));
+
+            Unbind();
         }
 
         public void Update()
@@ -150,6 +153,8 @@ public void UpdateRange(int startIndex, int endIndex)
             GL.BufferSubData(BufferTarget.ArrayBuffer, (IntPtr)(startIndex * STRIDE), (IntPtr)(countVertices * STRIDE), ref getMemory().Span[startIndex]);
 
             FrameStatistics.Add(StatisticsCounterType.VerticesUpl, countVertices);
+
+            Unbind();
         }
 
         private ref Memory<DepthWrappingVertex<T>> getMemory()

this one appears to stop the crashes.

[frenzibyte]

That's interesting, I'm also wondering if commenting the glDeleteVertexArray operation stops the crash as well. Normally we wouldn't want to delete VAOs at all, but cache them for activating a specific set of vertex attributes.

[bdach]

If you mean something like this:

diff --git a/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs b/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
index 524fa8d57..285f0eb7c 100644
--- a/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
+++ b/osu.Framework/Graphics/OpenGL/Buffers/GLVertexBuffer.cs
@@ -178,7 +178,7 @@ void IVertexBuffer.Free()
             if (isInitialised)
             {
                 GL.DeleteBuffer(vboId);
-                GL.DeleteVertexArray(vaoId);
+                //GL.DeleteVertexArray(vaoId);
             }
 
             memoryOwner?.Dispose();

then yes, not deleting the VAO also fixes.