Search Unity

Is there a memory leak?

Discussion in 'Scripting' started by MarkusGod, Mar 27, 2018.

  1. MarkusGod

    MarkusGod

    Joined:
    Jan 10, 2017
    Posts:
    168
    Hello guys, I'm inspecting my code for a memory leak. I'm not sure is it really there but what I catch during testing is that used memory increasing every time new music track is loaded. It would be really nice if somebody with a sharp eye can inspect this piece of code and say is there a memory leak.
    (Unity 2017.4.0f1)
    Code (CSharp):
    1. using System.Collections.Generic;
    2. using System.IO;
    3. using System.Linq;
    4. using UnityEngine;
    5. using UnityEngine.UI;
    6.  
    7. public class MusicPlayer : MonoBehaviour
    8. {
    9.     public enum SeekDirection
    10.     {
    11.         Forward,
    12.         Backward
    13.     }
    14.  
    15.     private string _absolutePath = "./music";
    16.  
    17.     [SerializeField] [HideInInspector] private int _currentIndex;
    18.  
    19.     private FileInfo[] _soundFiles;
    20.     private readonly List<string> _validExtensions = new List<string> {".ogg"};
    21.  
    22.     public AudioClip ClipToPlay;
    23.  
    24.     public Text MusicName;
    25.  
    26.     public ParticleSystem prt;
    27.     public AudioSource source;
    28.  
    29.     private void Start()
    30.     {
    31.         if (Application.isEditor) _absolutePath = "Assets/";
    32.  
    33.         if (source == null) source = gameObject.AddComponent<AudioSource>();
    34.  
    35.         ReloadSounds();
    36.         PlayCurrent();
    37.     }
    38.  
    39.     private void FixedUpdate()
    40.     {
    41.         if (!source.isPlaying)
    42.         {
    43.             Seek(SeekDirection.Forward);
    44.             PlayCurrent();
    45.         }
    46.     }
    47.  
    48.     private void Seek(SeekDirection d)
    49.     {
    50.         if (d == SeekDirection.Forward)
    51.         {
    52.             _currentIndex = (_currentIndex + 1) % _soundFiles.Length;
    53.         }
    54.         else
    55.         {
    56.             _currentIndex--;
    57.             if (_currentIndex < 0) _currentIndex = _soundFiles.Length - 1;
    58.         }
    59.     }
    60.  
    61.     private void PlayCurrent()
    62.     {
    63.         ClipToPlay = LoadClip(_soundFiles[_currentIndex].FullName);
    64.         source.clip = ClipToPlay;
    65.         source.Play();
    66.         prt.Simulate(0, true, true);
    67.         prt.Play(true);
    68.         MusicName.text = ClipToPlay.name.Replace(".ogg", "");
    69.         ReloadSounds();
    70.     }
    71.  
    72.     private void ReloadSounds()
    73.     {
    74.         var info = new DirectoryInfo(_absolutePath);
    75.         _soundFiles = info.GetFiles()
    76.             .Where(f => IsValidFileType(f.Name))
    77.             .ToArray();
    78.     }
    79.  
    80.     private bool IsValidFileType(string fileName)
    81.     {
    82.         return _validExtensions.Contains(Path.GetExtension(fileName));
    83.     }
    84.  
    85.     private AudioClip LoadClip(string path)
    86.     {
    87.         var www = new WWW("file://" + path);
    88.         Debug.Log("loading " + path);
    89.  
    90.         var clip = www.GetAudioClip(false);
    91.         while (clip.loadState != AudioDataLoadState.Loaded)
    92.         {
    93.         }
    94.  
    95.         clip.name = Path.GetFileName(path);
    96.         return clip;
    97.     }
    98. }
     
  2. Vryken

    Vryken

    Joined:
    Jan 23, 2018
    Posts:
    2,106
    I haven't fiddled with Unity's file management system yet, but if I had to guess, I noticed that AudioClips have a method called UnloadAudioData(). Perhaps you could try calling this at the start of your LoadClip method and see if it makes a difference:
    Code (CSharp):
    1.  
    2. private AudioClip LoadClip(string path) {
    3.    ClipToPlay.UnloadAudioData();
    4.  
    5.    //The rest of your code
    6. }
    7.  
    Or alternatively, I wouldn't use a global AudioClip variable in the first place here, since you are returning a local AudioClip in your LoadClip method.
    In your PlayCurrent method, instead of this:
    Code (CSharp):
    1.  
    2. ClipToPlay = LoadClip(_soundFiles[_currentIndex].FullName);
    3. source.clip = ClipToPlay;
    4.  
    I would do this:
    Code (CSharp):
    1. source.clip = LoadClip(_soundFiles[_currentIndex].FullName);
    That way your AudioSource is using a completely new AudioClip every time you load one.
     
    MarkusGod likes this.
  3. MarkusGod

    MarkusGod

    Joined:
    Jan 10, 2017
    Posts:
    168
    Hi, thanks for the suggestion, it actually appears that we need unload and destroy audioclip so the solution would be to add
    Code (CSharp):
    1. if (ClipToPlay != null && ClipToPlay.loadState == AudioDataLoadState.Loaded)
    2.         {
    3.             ClipToPlay.UnloadAudioData();
    4.             Destroy(ClipToPlay);
    5.         }
    to the start of PlayCurrent(), but your suggestion have a way more logic than mine.:)