Mincus's picture

OpenTK and IDisposable

I have been wondering about whether OpenTK expects unmanaged resources to use IDisposable.
Largely up until now I've just been ensuring to release all resources in the Unload() function for Gamewindow. This works well enough when you keep track of all the resources, but no-one's perfect so I was looking at IDisposable.
On the surface it looks like a good plan, but having read this thread (which doesn't appear to have a solution), it brings up the question "what if the OpenGL context has been destroyed?"

My question is basically this: Should OpenTK library users be implementing the IDisposable pattern or does it really not matter?
The reason for this is I'm intending on writing a short tutorial series to get new users familiar with the Gamewindow class, using VBOs and eventually extending it to a small game (or two) building up a simple toolkit (I had been intending this before Fiddler made the post and have the first tutorial largely ready, I'll post the first two shortly in a blog post for comments). Including the IDisposable pattern in such a tutorial would make sense if OpenTK expects it, but I also don't want to make the tutorials too complicated, and IDisposable is a fairly complicated pattern to understand some of the reasoning behind (I refer to this codeproject article as a good reference for understanding it, with this MSDN article explaining how it should be implemented).

So I knocked up a fairly simple example of how I suspect IDisposable would fit into OpenTK. It doesn't cause any problems, but I also know that doing things this way without IDisposable also won't cause any problems (whether you forget to call Unload on the texture or not).

	public class TextureClass : IDisposable
	{
		private sealed class TextureWrapper : IDisposable
		{
			public TextureWrapper(int textureID)
			{
				TextureID = textureID;
				Disposed = false;
			}
 
			public int TextureID;
 
			public bool Disposed;
 
			public void Dispose ()
			{
				if(!Disposed)
				{
					GC.SuppressFinalize(this);
					GL.DeleteTexture(TextureID);
					Disposed = true;
				}
			}
		}
 
		public TextureClass(string filename)
		{
			Filename = filename;
 
			Disposed = false;
		}
 
		private string Filename;
 
		private TextureWrapper Texture = null;
 
		public bool Disposed;
 
		public int ID { get { return Texture.TextureID; } }
 
		public void Load()
		{
			Bitmap TextureBMP = new Bitmap(Filename);
			int handle = 0;
 
            BitmapData TextureData = TextureBMP.LockBits(new Rectangle(0, 0, TextureBMP.Width, TextureBMP.Height), ImageLockMode.ReadOnly, System.Drawing.Imaging.PixelFormat.Format32bppArgb);
 
            GL.GenTextures(1, out handle);
            GL.BindTexture(TextureTarget.Texture2D, handle);
            GL.TexEnv(TextureEnvTarget.TextureEnv, TextureEnvParameter.TextureEnvMode, (float)TextureEnvMode.Modulate);
            GL.TexParameter(TextureTarget.Texture2D, TextureParameterName.TextureMinFilter, (float)TextureMinFilter.LinearMipmapLinear);
            GL.TexParameter(TextureTarget.Texture2D, TextureParameterName.TextureMagFilter, (float)TextureMagFilter.Linear);
 
            Glu.Build2DMipmap(TextureTarget.Texture2D, (int)PixelInternalFormat.Srgb8Alpha8, TextureBMP.Width, TextureBMP.Height, OpenTK.Graphics.PixelFormat.Bgra, PixelType.UnsignedByte, TextureData.Scan0);
 
			Texture = new TextureWrapper(handle);
 
			Disposed = false;
		}
 
		public void Unload()
		{
			Dispose();
		}
 
		public void Dispose()
		{
			if(!Disposed)
			{
				GC.SuppressFinalize(this);
				if(Texture != null)
				{
					Texture.Dispose();
				}
				Disposed = true;
			}
		}
	}

Full solution with simple use case is attached (expects logo.jpg from the OpenTK examples and works with OpenTK 0.9.8).

AttachmentSize
disposable example.zip3 KB

Comments

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
the Fiddler's picture

Edit: Updated code with martinsm's observations.

While resource leaks during application shutdown tend to be harmless (driver bugs notwithstanding), leaks during runtime are nasty. My personal preference is to use a pattern like this:

public sealed class Texture : IDisposable
{
    readonly int Id;
    readonly GraphicsContext Context;
    bool disposed;
 
    public Texture(GraphicsContext context)
    {
        if (context == null)
            throw new ArgumentNullException("context");
        if (GraphicsContext.CurrentContext != context)
            throw new InvalidOperationException("Add suitable error message here.");
 
        Context = context;
        GL.GenTextures(1, out Id);
    }
 
    public void Dispose()
    {
         Dispose(true);
         GC.SuppressFinalize(this);
    }
 
    // If the class is not sealed, make this protected virtual.
    void Dispose(bool manual)
    {
        if (!disposed)
       {
           if (manual)
           {
                GraphicsContext current_context = GraphicsContext.CurrentContext;
                if (current_context == Context)
                {
                     int id = Id;
                     GL.DeleteTextures(1, ref id);
                     disposed = true;
               }
               else
               {
                   Debug.Print("[Warning] Attempted to free {0} through GraphicsContext: {1}.", this, current_context);
                   // Don't mark the resource as disposed to give the user a (misguided?) chance to dispose it using the correct context.
               }
           }
           else
           {
               Debug.WriteLine(String.Format("[Warning] Resource {0} leaked.", this));
               disposed = true;
           }
     }
 
     ~Texture()
     {
         Dispose(false);
     }
 
     public override string ToString()
     {
          return String.Format("{0} (Id: {1}, Context: {2}",GetType(), Id, Context);
     }
}

This may look like overkill, but the warning message comes in handy. For example, my last application used to get gradually slower for no visible reason (falling from 120 -> 30 fps or worse). I eventually traced the issue to a texture leak inside the Resize event: the application created a texture with the exact window Width / Height, but forgot to Dispose() the previous texture.

Adding a warning message makes issues like this impossible to miss.

Edit: note that the constructor requires a GraphicsContext instance. This pattern eliminates a whole class of bugs related to calling OpenGL functions without or with the wrong context. Overkill? Maybe, but defensive programming pays off in larger projects.

Edit 2: also note that we store a reference to the GraphicsContext. This ensures that the context won't be garbage collected while we are using it.

Unfortunately, this pattern still suffers from two potential issues:

  • The user can call Dispose() on the context before disposing its resources.
  • The user may make the context non-current before disposing a resource belonging to it.

I haven't been able to find a good solution to these issues yet. Any ideas?

Edit 3: The code now attempts to counter the above issues by checking GraphicsContext.CurrentContext before attempting to dispose the resource.

martinsm's picture

You are not assigning context argument in constructor to member variable context.
And storing GraphicsContext as member in Texture class will not guarantee that Texture will be disposed after GraphicsContext.
Afaik if Texture and its GraphicsContext is on list marked ready for finalization then there is no order how they will be disposed by GC. It can be Texture first, or it can be GraphicsContext first.

objarni's picture

Just an application programmers random thoughts:

OpenGL-logic: textures must be disposed before context, right?

Real-life: GL-context are not disposed until application ends its lifetime. Correct?

In that case, disposing gl-contexts seems over-complicated and unnecessary. This is not an operating system..

Conclusion: Making sure textures are deallocated at level-change and such things is good enough, and the routines for doing so may assume the gl-context is fine.

A bit naive?

martinsm's picture

Real-life: GL-context are not disposed until application ends its lifetime. Correct?
I think in OpenTK GL-context is only disposed automatically when you close window, not at end of application lifetime.

objarni's picture

How many of the applications you've seen use OpenTK open-close windows multiple times?

In my experience the count is close to zero.

I'm not saying this issue is unimportant, just that it does not have high priority. It is a "nice to have" feature rather than a "must have" feature, speaking in "business terms" :)

the Fiddler's picture
martinsm wrote:

You are not assigning context argument in constructor to member variable context.

You are right, fixed now.

martinsm wrote:

And storing GraphicsContext as member in Texture class will not guarantee that Texture will be disposed after GraphicsContext.
Afaik if Texture and its GraphicsContext is on list marked ready for finalization then there is no order how they will be disposed by GC. It can be Texture first, or it can be GraphicsContext first.

Indeed, but this is not the point. The point is that the GC will not collect the GraphicsContext as long as there are live references to it. Once all references are dead the GraphicsContext becomes eligible for finalization. Finalization order doesn't actually matter, as taking down the context will (should) free all dependent OpenGL resources.

Which brings us to the issue I mentioned: we must check whether the context is still alive and current when disposing any resource. I have updated the code with an attempt to do this (any outstanding issues you can see)?

Edit:

objarni wrote:

How many of the applications you've seen use OpenTK open-close windows multiple times?

Indeed, but you can't be sure noone will ever attempt that. It's one use-case that's been bothering me since starting the gw-next2 branch: should we be able to re-open a closed window (recreating its handle, context, etc) or should we treat it as a disposed resource?

Mincus's picture

Ok, thanks for the replies.
Seems the answer to my original question is a yes: we should be using IDisposable.
So, will drop that into my tutorials, with Fiddler's suggested usage.

objarni's picture

Indeed, but you can't be sure noone will ever attempt that.

Since you cannot be sure that no one will ever attempt to do that, you are prioritizing this esoteric issue, instead of fixing things that we need today, like better platform indepence, gl3 testing, removing the "author" text on doc pages (to improve documentation flow) etc.?

I'm trying to be pragmatic here. Priority to things we need today, not things we maybe need tomorrow.

Mincus's picture

With regard to re-opening Gamewindows:
I think it should just be clear it's not supported. It's not a common enough use case to worry about and if someone needs that sort of use from OpenTK they need to be working with a GLControl really, IMO.
As I understand it, Gamewindow has clear use cases, and re-opening the same window doesn't fall in those.

objarni's picture

An idea:

Is it possible to "reuse" OpenGL contexts?

When a window closes, any gl context it hosts is "collected" (not disposed), and when some new window (or the same window Shows() again), a collected context is used (or a new one is created if the collection is empty)?

This would:

1) Make re-opening windows a lot faster (contexts are "cached")
2) Solve the disposion-problem (just dispose all collected contexts on application exit)
3) Take some memory overhead in applications with a lot of open-close-window logic, since the collected contexts are not in use when windows are closed (and this should not be that much of the time in the application anyway)

This is sort of like the fast-reallocation-algorithm-trick in C++, where you override new/delete for small/frequently used structures (say link list nodes) and instead of malloc:ing new nodes at "new", you primarily reuse "collected" nodes. Just on a totally different scale :)