7

I have a SoundManager class for easy sound management. Essentially:

public class SoundManager {
    public static class Sound {
        private Clip clip; // for internal use

        public void stop() {...}
        public void start() {...}
        public void volume(float) {...}
        // etc.
    }

    public Sound get(String filename) {
        // Gets a Sound for the given clip
    }

    // moar stuff
}

Most of the uses of this are as follows:

sounds.get("zap.wav").start();

As I understand it, this should not keep a reference to the newly created sound in memory, and it should be garbage collected fairly rapidly. However, with a short sound file (108 KB, clocking in at a whopping 00:00:00 seconds, and about 0.8s in reality), I can only get to about 2100 invocations before I get an OutOfMemoryError:

# There is insufficient memory for the Java Runtime Environment to continue.
# Native memory allocation (malloc) failed to allocate 3874172 bytes for jbyte in C:\BUILD_AREA\jdk6_34\hotspot\src\share\vm\prims\jni.cpp
# An error report file with more information is saved as:
# [path]

I tried implementing a private static final Vector<WeakReference<Sound>> in the SoundManager.Sound class, adding the following to the constructor:

// Add to the sound list.
allSounds.add(new WeakReference<SoundManager.Sound>(this));
System.out.println(allSounds.size());

This also allows me to iterate through at the end of the program and stop all sounds (in an applet, this is not always done automatically).

However, I still only get about 10 more invocations before the same OutOfMemoryError occurs.

If it matters, for each file name, I'm caching the file contents as a byte[], but this is only done once per file so it shouldn't accumulate.

So why are these references being retained, and how can I stop it without just increasing the heap size?


EDIT: The "error report with more information" contains, at line 32:

Java frames: (J=compiled Java code, j=interpreted, Vv=VM code)
J  com.sun.media.sound.DirectAudioDevice.nWrite(J[BIIIFF)I
J  com.sun.media.sound.DirectAudioDevice$DirectDL.write([BII)I
j  com.sun.media.sound.DirectAudioDevice$DirectClip.run()V+163
j  java.lang.Thread.run()V+11
v  ~StubRoutines::call_stub

Does this mean that this issue is completely out of my control? Does javasound need time to "cool down"? I'm spewing these sounds out at 300/second, for debugging purposes.


EDIT with more info on my use of JavaSound.

The first time I call sounds.get("zap.wav"), it sees that "zap.wav" has not been loaded before. It writes the file to a byte[] and stores it. It then proceeds as if it had been cached before.

The first and all subsequent times (after caching), the method takes the byte[] stored in memory, makes a new ByteArrayInputStream, and uses AudioSystem.getAudioInputStream(bais) on said stream. Could it be these streams holding memory? I would think that when the Sound (and thus the Clip) is collected, the stream would be closed also.


EDIT with the get method per request. This is public Sound get(String name).

  • byteCache is a HashMap<String, byte[]>
  • clazz is a Class<?>

byteCache is a HashMap<String, byte[]> and clazz is a Class<?>

try {
    // Create a clip.
    Clip clip = AudioSystem.getClip();

    // Find the full name.
    final String fullPath = prefix + name;

    // See what we have already.
    byte[] theseBytes = byteCache.get(fullPath);

    // Have we found the bytes yet?
    if (theseBytes == null) {
        // Nope. Read it in.
        InputStream is = clazz.getResourceAsStream(fullPath);

        // Credit for this goes to Evgeniy Dorofeev:
        // http://stackoverflow.com/a/15725969/732016

        // Output to a temporary stream.
        ByteArrayOutputStream baos = new ByteArrayOutputStream();

        // Loop.
        for (int b; (b = is.read()) != -1;) {
            // Write it.
            baos.write(b);
        }

        // Close the input stream now.
        is.close();

        // Create a byte array.
        theseBytes = baos.toByteArray();

        // Put in map for later reference.
        byteCache.put(fullPath, theseBytes);
    }

    // Get a BAIS.
    ByteArrayInputStream bais = new ByteArrayInputStream(theseBytes);

    // Convert to an audio stream.
    AudioInputStream ais = AudioSystem.getAudioInputStream(bais);

    // Open the clip.
    clip.open(ais);

    // Create a new Sound and return it.
    return new Sound(clip);
} catch (Exception e) {
    // If they're watching, let them know.
    e.printStackTrace();

    // Nothing to do here.
    return null;
}

EDIT after heap profiling.

Heapdumped about 5 seconds before crash. Well this is telling:

heap dump chart

Problem Suspect #1:

2,062 instances of "com.sun.media.sound.DirectAudioDevice$DirectClip", loaded by "" occupy 230,207,264 (93.19%) bytes.

Keywords com.sun.media.sound.DirectAudioDevice$DirectClip

These Clip objects are strongly referenced by Sound objects but the Sound objects are only weakly referenced in a Vector<WeakReference<Sound>>.

I can also see that each Clip object contains a copy of the byte[].


EDIT per Phil's comments:

I've changed this:

// Convert to an audio stream.
AudioInputStream ais = AudioSystem.getAudioInputStream(bais);

// Open the clip.
clip.open(ais);

to this:

// Convert to an audio stream.
AudioInputStream ais = AudioSystem.getAudioInputStream(bais);

// Close the stream to prevent a memory leak.
ais.close();

// Open the clip.
clip.open(ais);
clip.close();

This fixes the error but never plays any sound.

If I omit clip.close() the error still occurs. If I move ais.close() to after clip.open the error still occurs.

I've also tried adding a LineListener to the clip as it's created:

@Override
public void update(LineEvent le) {
    if (le.getType() == LineEvent.Type.STOP) {
        if (le.getLine() instanceof Clip) {
            System.out.println("draining");
            ((Clip)le.getLine()).drain();
        }
    }
}

I get a "draining" message each time the clip finishes or is stopped (i.e., 30+ times/second after it starts to happen), but still get the same error. Replacing drain with flush has no effect either. Using close makes the line unopenable later on (even when listening for START and calling open and start).

wchargin
  • 15,589
  • 12
  • 71
  • 110
  • 1
    It looks to me like you're using some JNI library that's chewing up heap. – Hot Licks Apr 01 '13 at 03:38
  • Would that be a javasound issue then? Everything I've done is vanilla. – wchargin Apr 01 '13 at 03:41
  • I know nothing about JavaSound. But you may be "abusing" it somehow, by not resetting something, etc. – Hot Licks Apr 01 '13 at 03:44
  • OutOfMemoryError in this case would means something related to the stack. Aren't you having some kind of circular calls hidden somewhere? – MeTitus Apr 01 '13 at 03:49
  • @Marco wouldn't that be a `StackOverflowError`? I don't think I have any circular calls - the frame rate is fine and it doesn't seem to be deadlocking. – wchargin Apr 01 '13 at 03:52
  • I did not get this: "I can only get to about 2100 invocations..." also if you were catching just once the exceptions would not happening. The byte is saved onto the stack so maybe you're missing something. – MeTitus Apr 01 '13 at 03:53
  • By "I can only get to about 2100 invocations..." I mean I can only run `sounds.get("zap.wav").start()` about 2100 times before the OOM error. The `byte[]` should be stored on the heap - it's a value in a `HashMap` - but only once. – wchargin Apr 01 '13 at 03:57
  • @WChargin you're right this is a heap memory issue. Have you tried using a stream reader rather than caching the sounds? – MeTitus Apr 01 '13 at 03:59
  • Using a stream reader for what? I'm not caching any sounds, only the `byte[]`, which is only a `byte[110624]` - doesn't seem that bad to me. By comparison, just one of my image files is 365451 bytes. – wchargin Apr 01 '13 at 04:03
  • @WChargin like this one for example: AudioInputStream – MeTitus Apr 01 '13 at 04:04
  • That is not what the jvm is saying: "Native memory allocation (malloc) failed to allocate 3874172 bytes for jbyte in". You can check the max heap size with this: jinfo -flag MaxHeapSize. As I understood it, you have a list in which you store a list of bytes, as the contents of the musics you want to play, to me this is a no brainer, that list gets bigger in heap memory usage that the jvm can deal with. – MeTitus Apr 01 '13 at 04:10
  • 1
    And as for the stream, you should never store that kind of thing in memory, you input streams to read the content as it is needed. Can you show the content of the get method? – MeTitus Apr 01 '13 at 04:12
  • Profile your heap use and see where it's all going. – Hot Licks Apr 01 '13 at 15:33
  • Just out of curiosity, why are you using weak references vs just ditching the files immediately? (I suspect there is some other reference to the objects you're not aware of.) – Hot Licks Apr 01 '13 at 16:35
  • @HotLicks I'm using weak references so that I can stop them all at the end of the program. Chrome, at least, doesn't shut down the sounds when the tab is closed if it's an applet. – wchargin Apr 01 '13 at 16:36
  • I'm going to guess that `byteCache` is a static member variable, in which you are storing copies of all the sounds you have loaded. – jtahlborn Apr 01 '13 at 16:38
  • `byteCache` is a *non*-static member variable and `byteCache.size()` is never more than `3`. – wchargin Apr 01 '13 at 16:40
  • @WChargin - but it's in SoundManager, right, so it's sticking around regardless. are you loading the same sounds over and over? – jtahlborn Apr 01 '13 at 18:22
  • Yes, I am loading the same sound over and over. – wchargin Apr 02 '13 at 01:10
  • There is something really fundamentally wrong with this code: sounds.get("zap.wav").start(); You are using Clips, but Clips are designed to be preloaded. Combining the load and start acts with a Clip is asking for trouble. The start should be an entirely separate occurrence. If not, your concept is seriously off kilter and needs to be rethought. – Phil Freihofner Apr 03 '13 at 08:15

3 Answers3

2

I suspect that the problem is that you are not explicitly closing the audio streams. You should not rely on the garbage collector to close them.

The allocations seem to be failing in native allocations not in normal Java allocations, and I suspect that the normal behaviour of "the GC runs before throwing an OOME" applies in this case.

Either way, it is best practice to close your streams explicitly (using finally or the Java 7 try with resources). This applies to any kind of stream that involves external resources or off-heap memory buffers.

Stephen C
  • 698,415
  • 94
  • 811
  • 1,216
  • (assuming "audio streams" == `AudioInputStream ais`) How would I close those while still reading the bytes into the clip? The [tag:javasound] tag wiki doesn't close any streams. – wchargin Apr 01 '13 at 15:35
  • You don't. You wait until after you have finished reading. I'm afraid I can't figure out what your code is actually doing ... – Stephen C Apr 01 '13 at 15:43
1

Please excuse me if this is totally off, but I want to check on two fundamentals that I can't discern from a casual reading of your code.

1) How long are your sound files? Given a specific number of files, lengths in milliseconds, sample rate and encoding (e.g., 16-bit, stereo) you should be able to calculate the expected amount of memory to be consumed. What is that amount?

2) A really common error with Clips is to recreate them every time you play them instead of reusing existing Clips. I see in the comments the line: "sounds.get("zap.wav").start()" and that makes me wonder if you are making this fundamental error. You should only make a Clip once, and then reset the frame position to 0 when you wish to play it again. If you are recreating Clips at a furious rate, you WILL fill up memory very quickly, as each playback will create an additional object with its own copy of the PCM data.

Also, as one commenter states, it IS important to close the various streams. Not doing so will cause memory leaks.

Phil Freihofner
  • 7,645
  • 1
  • 20
  • 41
  • Re: (2) I am indeed creating a `Clip` each time. If the sound file is about 30 frames long, and two turrets fire 5 frames apart, you should be able to hear two distinct but overlapping firing sounds. Is there a way to *link* the PCM data of two or more clips? That's my ultimate goal. Also, could you provide an example of how/when to close the streams correctly? – wchargin Apr 02 '13 at 01:10
  • A "frame" in the context of sound is another word for a sample, and there are probably 44100 samples per second. Maybe you are using the word "frame" above as a term for the number of visual frames, and are displaying something like 30 or 60 fps? Different term! If your sound is 1 second long, stereo, 16-bit encoding, that is 4 bytes per sample * 44100 = 176400 bytes consumed each time you play that sound cue! So you really should reuse the Clips as much as possible. Do you know how to do that? You can make two copies and use them if you need to overlap. Some folks make their own mixer. – Phil Freihofner Apr 02 '13 at 04:14
  • To close an AudioInputStream named 'ais': ais.close(); ais = null; Do this once you are done reading from the stream. You *might* be able to save some space by nulling your clips, but I suspect this won't work. You might try using SourceDataLine instead of Clip. They consume MUCH less memory, and a new SDL starts quicker than a new Clip. Restarting an existing Clip is quickest, though. – Phil Freihofner Apr 02 '13 at 04:21
  • What do you mean by making two copies of the clip? That is my goal; how do I do so? – wchargin Apr 02 '13 at 23:01
  • "when you are done reading from the stream" == "when the clip has finished playing"? – wchargin Apr 02 '13 at 23:02
  • Have you read the spec on AudioInputStream? When you are finished instantiating a Clip, you are done with the AIS. It's not part of the playback. Do you know the difference between the heap and reference variables? To make two Clips from the same source file, you need to instantiate two separate reference variables in two separate acts so they won't refer to the same object on the heap. Don't try to assign clipGunshot1stcopy to clipGunShot2ndcopy. Have you read the java tutorials on sound? They are a tough read, but worth the struggle: http://docs.oracle.com/javase/tutorial/sound/playing.html – Phil Freihofner Apr 03 '13 at 02:23
  • I'm so sorry! I seem to have led you seriously astray. Never mind for the moment about closing the AudioInputSteam. The edit you made "on my advice" is quite wrong. In part, I was getting mixed up with SourceDataLine playback. I think the change you made should be undone. Instead, as a first step, perhaps see if you can just make a single Clip and play it back multiple times (never mind the sound overlapping situation--deal with that later) by resetting the frame position and and then restarting it. If you get that right, it will be easier to deal with all the rest. – Phil Freihofner Apr 03 '13 at 07:57
  • Does your 'reset' use either setFramePosition(0) or setMicrosecondPosition(0) on the clip instance? ...Maybe you should check out a simple but capable sound management library like TinySound. – Phil Freihofner Apr 03 '13 at 19:09
  • My reset uses `clip.setFramePosition(0)`; is that a problem? – wchargin Apr 03 '13 at 21:27
  • That is correct. I was just trying to see if you are correctly reusing Clips rather than making hundreds of them (which would probably eat up all your memory as you describe). – Phil Freihofner Apr 04 '13 at 04:29
  • I *am* making hundreds of clips, though, for reasons described [above](http://stackoverflow.com/questions/15737244/weak-references-and-outofmemoryerrors/15796828?noredirect=1#comment22389777_15749156). Is there no way to just link the data of `byte[]` between clips? – wchargin Apr 04 '13 at 04:36
  • Not that I am aware of. Each time you instantiate a Clip, a separate copy of the ENTIRE sound file is created. That is why I created my own Clip with the capability of multiple playbacks. See "Another approach:" below. But in your case, how many different sounds are there and how many times, at most, will the same one overlap with itself? Then, make one Clip for each instance, and reuse the Clips as needed by never closing them, but rather setFramePosition(0) and play the same instance again, as needed. You will still use a lot of memory to set it all up initially, but it won't keep growing. – Phil Freihofner Apr 04 '13 at 15:47
0

Another approach: abandon the use of Java's Clip and write your own. I did this. I made an object that stores the data in memory and two other objects that provide "cursors" into the storage for two different type of playback. One is for looping and the other is for overlapping playback. Both can be set to run at different playback speeds, so you can get different effects by speeding up or slowing down the playback. Both direct their output to a mixer I wrote, where the data is consolidated into a single SourceDataLine.

The code is presented here, included in the jar linked in the first post: http://www.java-gaming.org/topics/simple-audio-mixer-2nd-pass/27943/view.html

I'm looking forward to getting back to working on it some more, possibly putting it on GitHub.

Also, TinySound is a very capable sound manager. You can learn about it here. The approach is very similar to what I do, mixing down to a single output. TinySound provides support for Ogg and such. I don't think it provides vari-speed playback.

http://www.java-gaming.org/topics/need-a-really-simple-library-for-playing-sounds-and-music-try-tinysound/25974/view.html

Phil Freihofner
  • 7,645
  • 1
  • 20
  • 41