Search Unity

  1. Welcome to the Unity Forums! Please take the time to read our Code of Conduct to familiarize yourself with the forum rules and how to post constructively.
  2. Dismiss Notice

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.:)