GPU_CopyImage Target Memory Leak

[DiegoAce]

After copying an image with GPU_CopyImage and then changing its target, such as with GPU_LoadTarget, there is a memory leak. gpu_copy_image_pixels_only creates a target for the newly copied image but does not free it. The user is not told that the copied image has a target that needs to be freed before creating a new one.

[grimfang4]

I'll have to look closer at this. How are you detecting the leak? Can you supply a short example program?

The intended design is that if gpu_copy_image_pixels_only() needs to create a render target to do the copy, then it won't free the target and will instead decrement the target's refcount so that it will be freed only when necessary. If you call GPU_LoadTarget() on the image, it will increment the refcount and give you the same target that has already been created.

[DiegoAce]

I'm using Instruments on my Mac. I attached a zipped folder with a small example causing the leak and the Instruments leak test of that example. It includes screenshots of the call tree and code lines of when the leaked memory was malloc'd. Let me know if you need anything else.

GPU_CopyImage Target Memory Leak.zip

[grimfang4]

Can you see if 53a665c fixes this for you?

[DiegoAce]

Yep! Instruments is no longer reporting a leak.

[grimfang4]

Cool, thanks for the extra info! That helped a lot.

[DiegoAce]

Happy to help! Thanks for making a great library! It's helped me a lot!

[DiegoAce]

There's still something unusual going on here. I attached some updated code. When adding GPU_Clear(screen), the program infinitely accumulates memory. Instruments is not reporting a leak, but Xcode's debug navigator and Activity Monitor show an increasing memory for the program. I believe this is also causing crashing on iOS. I can test this on Windows if needed. I'm testing with the newest commit.

Here are a few code arrangements that each solve the problem:
Removing GPU_Clear
Removing GPU_FreeTarget
Moving GPU_FreeTarget to after GPU_Clear

main.txt

[grimfang4]

That it shows up as not a leak, but still gains memory in some measurements says it might be leaking GPU memory. The rate at which it is leaking is big, so it's probably something texture-sized.

I looked through and yep, it's leaking the FBO on FreeTarget() because of the check against the currently-bound FBO. Check out 25f55ac?

[DiegoAce]

That seems to have fixed it! Nice job on the quick work! I found out my crashing on iOS was due to never flipping the target creating from an image.
Is this the correct procedure for drawing on images:
-LoadTarget from image
-Blit stuff onto target
-Flip target
-Then I can choose to free or not free target. If not it will later be freed when image is freed.

[grimfang4]

Thanks!

That looks right, but see below.

GPU_Flip() is good for non-context targets (i.e. those with framebuffers not backed by a window) and will do the same as a call to GPU_FlushBlitBuffer(). That is, it will make sure that the blits are applied to the target.

So, looking at this prompted me to change the semantics of freeing targets a little. Technically, having a nonzero refcount indicates that some reference expects validity, so you should explicitly free targets when you don't need them any more. To make the same workflow make sense, I added an alternative to GPU_LoadTarget(): GPU_GetTarget(). GPU_GetTarget() will not increment the refcount so you can leave the freeing up to the image. If you still want to use GPU_LoadTarget() now, you should pair it with GPU_FreeTarget(). Hopefully that works as expected and clears up the usage!

[DiegoAce]

Alright, thanks!

I didn't use FlushBlitBuffer because I'm not aware of what is the "current context target." I suppose most SDL_GPU image and target operation functions sets that as current.

Sounds good! Loading and Freeing the target seems most appropriate to me. I was a little unsure because it seemed like SDL_GPU was cleaning up the target regardless.

[grimfang4]

Yeah, all operations that affect the underlying texture (clear, blit, flush, flip) or need a particular context (free image or target) will set the current context target.

It was cleaning up the target as long as the target was only loaded once, but I think this will be much clearer and safer.