Memory leak - don't know why (Solved)

[linuxgurugamer]

The following code, which is called at leat 30-40 times a second, causes a memory leak of 1-2 meg a second.  Essentially, it was being called 2-4 times every time through the OnGUI function:

 

        static Color32[] pixelBlock = null;
        static RenderTexture rt, origrt;
        public  Texture2D GetButtonTexture(Texture2D img)
        {
            Texture2D img2;

            // see: https://docs.unity3d.com/ScriptReference/Texture2D.GetPixels.html
            try
            {
                pixelBlock = img.GetPixels32();
                img2 = new Texture2D(img.width, img.height, TextureFormat.ARGB32, false);
                img2.SetPixels32(pixelBlock);
                Log.Info("GetPixels32 loaded image");
            }
            catch (UnityException _e)
            {

                img.filterMode = FilterMode.Point;
                rt = RenderTexture.GetTemporary(img.width, img.height);
                rt.filterMode = FilterMode.Point;
                origrt = RenderTexture.active;
                RenderTexture.active = rt;
                Graphics.Blit(img, rt);
                img2 = new Texture2D(img.width, img.height, TextureFormat.ARGB32, false);
                img2.ReadPixels(new Rect(0, 0, img.width, img.height), 0, 0);
                Log.Info("GetPixels32 had Exception, img name: " + img.name);
                RenderTexture.ReleaseTemporary(rt);
                RenderTexture.active = origrt;
            }
            img2.Apply();
            return img2;
        }

I don't know why, can someone who knows more than I do about Unity explain it?  and maybe how to fix it?

This is from The Janitor's Closet, in the JanitorsToolbar.cs file

I can say that the no exception is thrown, it's always in the try block.

Thanks in advance.

Edited March 24, 2017 by linuxgurugamer

[xEvilReeperx]

4 hours ago, linuxgurugamer said:

img2 = new Texture2D(img.width, img.height, TextureFormat.ARGB32, false);

This is the worst one. GetPixels32() as well. This code will also potentially be very costly to run, potentially framerate-killing, because it'll stall the graphics pipeline every time you ReadPixels. I assume the error catch here is a fallback for unreadable textures

As a final punch in the pants, it'll quickly consume all available heap mem because you never destroy the Texture2D. Eventually Unity will force a cleanup and the unreferenced ones will be destroyed.

 

What is the purpose of this code? You seem to know that you can just read a texture directly from one of the components used in ApplicationLauncher buttons. If you want to copy the texture to manipulate it, instantiate a copy instead. I don't see where it's ultimately manipulated on a first glance though

Edited March 23, 2017 by xEvilReeperx

[sarbian]

And to add to what @xEvilReeperx said : Do not do non GUI code in onGUI. You do not know how many time per frame the method will be called. If you need to do something once per frame do in in an Update.

And yes, for the love of whatever you want DO NOT "new texture".

[linuxgurugamer]

7 hours ago, xEvilReeperx said:

This is the worst one. GetPixels32() as well. This code will also potentially be very costly to run, potentially framerate-killing, because it'll stall the graphics pipeline every time you ReadPixels. I assume the error catch here is a fallback for unreadable textures

As a final punch in the pants, it'll quickly consume all available heap mem because you never destroy the Texture2D. Eventually Unity will force a cleanup and the unreferenced ones will be destroyed.

 

What is the purpose of this code? You seem to know that you can just read a texture directly from one of the components used in ApplicationLauncher buttons. If you want to copy the texture to manipulate it, instantiate a copy instead. I don't see where it's ultimately manipulated on a first glance though

Janitor's Closet uses a hash of the button texture to uniquely identify a ApplicationLauncherButton.  I found that there was no other way to do it, since some mods seem to load the button texture in a way which doesn't give a name to the button.  So what happens is that I first get the texture, and then create a hash from it.  I still store the whole texture, since I use it to display the toolbar button.

I found this code online, at the referenced link.

yes, the error catch is a fallback, but ever since a minor change was made to TextureReplacer at my request, it hasn't been needed.  I leave it there in case someone, somewhere, creates an unreadable texture.

Based on @sarbian's comment, I've moved the code into a FixedUpdate function.

Re. reading the texture from the button, while I see that the sprite has a texture, it's not accessible directly, when I do, I get:

NullReferenceException: Object reference not set to an instance of an object
  at JanitorsCloset.JanitorsCloset.FixedUpdate () [0x00000] in <filename unknown>:0 

I've added calls to Destory() to get rid of the textures I done with.  And that seemed to fix the problem.

I've also added some code to limit the times the buttonlist is updated to once every half second, and it's all now in the FixedUpdate() function.

 

5 hours ago, sarbian said:

And to add to what @xEvilReeperx said : Do not do non GUI code in onGUI. You do not know how many time per frame the method will be called. If you need to do something once per frame do in in an Update.

And yes, for the love of whatever you want DO NOT "new texture".

First line implemented, thanks

Can you explain why the "new texture' is bad?  When I remove the "new", I get a compile error:
 

Error    CS1955    Non-invocable member 'Texture2D' cannot be used like a method.    JanitorsCloset    D:\Users\jbb\github\JanitorsCloset\JanitorsCloset\JanitorsToolbar.cs    728    Active

 

[linuxgurugamer]

35 minutes ago, linuxgurugamer said:

Re. reading the texture from the button, while I see that the sprite has a texture, it's not accessible directly, when I do, I get:

NullReferenceException: Object reference not set to an instance of an object
  at JanitorsCloset.JanitorsCloset.FixedUpdate () [0x00000] in <filename unknown>:0 

Ignore this null ref, it wasn't related.

I may now be able to access the sprite texture directly.  I don't have time to test, but a very quick test showed that it was now working.  Maybe this is all a holdover back from when the textures were being modified by Texturereplacer?

 

I'll get back to this later this evening and post my results

[xEvilReeperx]

1 hour ago, linuxgurugamer said:

 I found that there was no other way to do it, since some mods seem to load the button texture in a way which doesn't give a name to the button.

Every UnityEngine.Object has a unique identifier you can get with GetInstanceID(). One simple fix is to look at the instance IDs of the buttons themselves.

If you want max efficiency, you could eliminate the constant rechecking and comparisons entirely by setting up a little tracking MonoBehaviour that sets off an event whenever a button is created or destroyed (or whatever else you'd like)

public static class ToolbarEvents
{
    public enum ChangeType { Added, Removed }

    // just like GameEvents
    public static readonly EventData<ApplicationLauncherButton, ChangeType> ButtonChange =
        new EventData<ApplicationLauncherButton, ChangeType>("AppLauncher_ButtonEvent");


    [KSPAddon(KSPAddon.Startup.MainMenu, true)]
    private class Install : MonoBehaviour
    {
        private IEnumerator Start()
        {
            while (ApplicationLauncher.Instance == null) yield return null;

            var al = ApplicationLauncher.Instance;

            ButtonEventDispatcher.AddToPrefab(al.listItemPrefab);

            foreach (var b in al.GetComponentsInChildren<ApplicationLauncherButton>(true))
                ButtonChange.Fire(b, ChangeType.Added);
                
            Destroy(gameObject);
        }
    }

    private class ButtonEventDispatcher : MonoBehaviour
    {
        [SerializeField] private ApplicationLauncherButton _ourButton; // Unity instantiation serialization will make sure this points
                                                                        // to the live button when created

        private static ApplicationLauncherButton _buttonPrefab; // this used to prevent Unity from unloading prefab from memory
                                                                // should it stop being referenced at some point

        public static void AddToPrefab(ApplicationLauncherButton prefab)
        {
            var ourPrefab = prefab.gameObject.AddComponent<ButtonEventDispatcher>();
            ourPrefab._ourButton = prefab;

            _buttonPrefab = prefab;
        }

        private void Start()
        {
            ButtonChange.Fire(_ourButton, ChangeType.Added);
        }

        private void OnDestroy()
        {
            ButtonChange.Fire(_ourButton, ChangeType.Removed);
        }

        // can also do events for hiding/showing if desired
    }
}

 

[linuxgurugamer]

The problem is that some mods update the button textures of an existing button, so the id won't change.  Could this detect that?

If there would be a way to detect this, then it would help.

The problem is that the sprite's texture is a plain texture, and I haven't yet figured out how to get a byteAR of the underlying texture;  I can do it with Texture2D, however.

 

Edited March 23, 2017 by linuxgurugamer

[linuxgurugamer]

10 hours ago, xEvilReeperx said:

This is the worst one. GetPixels32() as well

I'm assuming that this call is expensive, in terms of locking the pipe?  How can I replace it?

[DMagic]

3 hours ago, linuxgurugamer said:

Can you explain why the "new texture' is bad?  When I remove the "new", I get a compile error:

It's creating a new texture every time it's called. Even with small textures this adds up quickly over time. If the textures are always the same size then just create a texture once and reuse it. 

Is the code in the first post meant to test if the texture is readable? Because the try block is just reading the texture's pixels then setting them again. If you want to test if it's readable you can just try a single GetPixel, since Get, Set and Apply are all somewhat expensive calls to be made so often.

1 hour ago, linuxgurugamer said:

The problem is that some mods update the button textures of an existing button, so the id won't change.  Could this detect that?

Do you mean like mods that replace the stock icons, Alternate Resource Panel, or Contracts Window + (which doesn't actually change the icon)? Those are the only two that I know of. If so maybe you can just wait for a scene change to do anything with those. It's not exactly common for a user to be toggling them on and off, and if it's the only thing preventing you from using the Object listener method (which is almost certainly a far more efficient and more reliable method) I would reconsider.

[linuxgurugamer]

33 minutes ago, DMagic said:

It's creating a new texture every time it's called. Even with small textures this adds up quickly over time. If the textures are always the same size then just create a texture once and reuse it.

Ah, the light dawns.

Ok, that's easy enough to do, thanks.

37 minutes ago, DMagic said:

Is the code in the first post meant to test if the texture is readable? Because the try block is just reading the texture's pixels then setting them again. If you want to test if it's readable you can just try a single GetPixel, since Get, Set and Apply are all somewhat expensive calls to be made so often.

This code:

static Color32[] pixelBlock = null;
.
.
.

pixelBlock = img.GetPixels32();
img2 = new Texture2D(img.width, img.height, TextureFormat.ARGB32, false);
img2.SetPixels32(pixelBlock);

was copied from the link (a Unity page).   It is getting the texture into a Texture2D class (img2).  I don't thing the "catch" has been used lately, it's there for safety.

 

39 minutes ago, DMagic said:

Do you mean like mods that replace the stock icons, Alternate Resource Panel, or Contracts Window + (which doesn't actually change the icon)?

Fusebox, Ampyear, and some others also change it on the fly to show current status.  Not stock icons, their own icons on the toolbar

[linuxgurugamer]

5 hours ago, linuxgurugamer said:

6 hours ago, DMagic said:

It's creating a new texture every time it's called. Even with small textures this adds up quickly over time. If the textures are always the same size then just create a texture once and reuse it.

Ah, the light dawns.

Ok, that's easy enough to do, thanks.

Actually, I have to create a new one, because it is being stored.  But I've added code to destroy it when not being used.

[xEvilReeperx]

7 hours ago, linuxgurugamer said:

The problem is that some mods update the button textures of an existing button, so the id won't change.  Could this detect that?

If there would be a way to detect this, then it would help.

The problem is that the sprite's texture is a plain texture, and I haven't yet figured out how to get a byteAR of the underlying texture;  I can do it with Texture2D, however.

You could get the instance IDs of textures, too. This would fail if they update their image by changing UVs. But why copy the texture? You can just grab a reference to it and use it directly. I thought you were only interested in the buttons themselves, which contain everything you need to render a duplicate version (if I understand what you're attempting to do correctly) by themselves.

7 hours ago, linuxgurugamer said:

I'm assuming that this call is expensive, in terms of locking the pipe?  How can I replace it?

That's not the expensive call. If the texture is readable, a copy of it exists in system memory so Unity won't stall the GPU to read pixels back from it. It's ReadPixels that's terribly slow (if you haven't got any unreadable textures, this isn't hurting you so far).

The problem with that one is simply that every time you call it, an array allocation will be made. Why do you need a clone of the texture? Are you modifying it later?

[linuxgurugamer]

So, my final solution is this:

Turns out that I can access the sprite.texture, so I don't need to get the texture using that above code every frame. So, the only time I do that call is when setting up the button the first time, and that's needed to get the hash number.

So all I need to do now is this in the FixedUpdate():

 if (curButton.Value.origButton.sprite.texture != curButton.Value.buttonTexture2)

which is better both from a performance issue and memory usage issue.

The only issue I have now, is that the texture is being displayed a little smaller than the 38x38.

The code doing this is:

 Rect brect = new Rect(0, 41 * cnt, 41, 41);

 if (GUI.Button(brect, curButton.Value.buttonTexture2))

 

[linuxgurugamer]

I had to do a little more with Unity than I like, but I now have the buttons displaying correctly