problems with onSensorChanged method

Question

my issue is this:

i need the phone to create a MediaPlayer object called smp when:

it sees that it's been shaken, it confirms that smp isn't already created, it sees that a button called hilt isChecked.

when it satisfies these conditions, it should create the player, play a sound, and destroy the player. the problem i have is that it will play the sound at most twice and then force close. i'm not sure why my logic doesn't work when i've used it for buttons in the app and it causes no problems.

my thinking is that i've got too many unused MediaPlayer objects but i can't see how since the OnCompletionListener should be calling release() every time the player is finished.

anyway, i'm sure it won't be that hard to better minds than mine. here's the code. thanks for any help.

UPDATE: this is the code i'm running now:

public void onSensorChanged(int sensor, float[] values) {

    synchronized (lock){
        if (!jobRunning){
            jobRunning = true;
        }else{ 
            return;
        }
    }

    if (sensor == SensorManager.SENSOR_ACCELEROMETER) {
        long curTime = System.currentTimeMillis();

        if ((curTime - lastUpdate) > 100) {
            long diffTime = (curTime - lastUpdate);
            lastUpdate = curTime;

            x = values[SensorManager.DATA_X];
            y = values[SensorManager.DATA_Y];
            z = values[SensorManager.DATA_Z];

            float speed = Math.abs(x+y+z - last_x - last_y - last_z)
                              / diffTime * 10000;


                if (speed > SHAKE_THRESHOLD && hilt.isChecked() == true) {

                    smp = MediaPlayer.create(getBaseContext(), swingSoundFx[rnd.nextInt(SWING_NUM)]); 
                    smp.setOnCompletionListener( new OnCompletionListener(){

                        @Override
                        public void onCompletion(MediaPlayer mp) {
                            if (smp != null){
                                smp.stop();
                                smp.release();
                            }

                        }   
                    }

                    );
                    smp.start();    
        }
        last_x = x;
        last_y = y;
        last_z = z;


        }
    }
    synchronized (lock){
        jobRunning = false;
    }
}

Answer 1

Original anwser

You have smp in an instance variable. Whenever you start a sound you are overwriting that variable, but your listener tries to close the currently stored player, so if for some reason you replaced the smp variable inbetween, you'll have a lingering player that you never closed (and caching effects will make that even uglier if smp is not volatile, but it doesn't work either way). The fix to that is simple: instead of closing the last player stored and let the previous one drop without ever getting closed, just use the MediaPlayer passed to you in the argument of onCompletion.

public void onCompletion(MediaPlayer mp) {
  mp.stop();
  mp.release();

This is the most obvious problem here.

Now that won't be enough to solve your problem because though that will help cleanup your players correctly, you will still start several of them at the same time. I don't think that calling start() on the player does result in isPlaying() being true immediately - it probably waits for the media to be open first and all, which gives a time window for your code to run another one (even more so since playing will likely happen in another thread). As it happens, chances are you will right away : on any strong acceleration you will receive high values several times in a row, so you will probably enter your test several times in a row, as often as 20 times a second or more if you asked for fast updates. In that short interval your phone won't have had time to open the media file and start playing it, so isPlaying() will still return false and you'll spawn a new one.

So actually what I would suggest is, instead of relying on isPlaying() to know whether something is in the works, having a synchronized boolean separate from the media player which tells you if you are indeed waiting for playing to start or playing. Turn it to true when start, to false after you stopped and destroyed your player, in the callback, without forgetting synchronization. That should work.
Edit: Sorry if that wasn't clear, you should synchronize the accesses to your boolean but not on the boolean itself or any transient object for that matter. For example, when the sound begins playing:

synchronized(lock) {
  if (soundRunning) return;
  soundRunning = true;
}

And when it stops, in the handler:

synchronized(lock) {
  soundRunning = false;
}

Going further, if you expect to play sounds several times, I would suggest having a longer-lived media player and reset it instead of creating and destroying one every time. This should conserve some resources. Of course if you do this don't forget to destroy it when you know you won't play sounds any more.

Answer for new code edit

Okay, there are two reasons for which this does not work.
The first one is, you did not change the onCompletion handler to stop the right object. You are still trying to stop a Player that does not point any more on the one that is playing, because you overwrote the variable. You can re-read the first paragraph of the initial anwser, as it still holds.

The other reason is, your boolean is not preventing two sounds to play at the same time, but preventing the method to run twice at the same time. It's not doing the right thing. Besides not only does it do the wrong thing, it's not even doing it in the right way. In the detail:

It's not the right way to prevent a method from running twice at the same time. That's the job of synchronize. You can just make your method synchronized, and any subsequent invocation will wait for any running instance to finish. You would not need a boolean variable to do that. You need a variable because:
You want to avoid starting playing a sound when one is running already, not avoid running concurrently the method that starts the sound. And the sound playing far exceeds the running life of your method, which is why you need to store it in a variable. As such, you should turn the boolean to true when you start playing and back to false when you stop playing, like this:

if (speed > SHAKE_THRESHOLD && hilt.isChecked() == true) {
  synchronized(lock) {
    if (isPlaying) return;
    isPlaying = true;
  }
  smp = MediaPlayer.create(getBaseContext(), swingSoundFx[rnd.nextInt(SWING_NUM)]); 
  smp.setOnCompletionListener( new OnCompletionListener(){
    @Override
    public void onCompletion(MediaPlayer mp) {
      mp.stop();
      mp.release();
      synchronized(lock) { isPlaying = false; }
    }
  });   
  smp.start();    
}

I didn't bother to test this so it might have bugs, but it's the general direction. You don't care whether the method gets called several times concurrently (or if you do, you can just synchronize it separately), but you want to know when the player is playing to just skip running another one at the same time, and that's what the boolean is supposed to mean.

Hope that helps, and that it was clearer that my initial answer.