Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 54d85c4b authored by Tyler Gunn's avatar Tyler Gunn
Browse files

InCallTonePlayer concurrency improvements. 

We've seen a number of issues lately where incall tones are not reporting
back to CallAudioManager when the tone is no longer playing.  This has
caused issues with the CallAudioModeStateMachine setting the mode back
to NORMAL.

To remedy this situation, implemented the following concurrency changes:
1) Removed the use of synchronized(this) as a means of providing a way for
a playing tone to be interrupted the InCallTonePlayer#stopTone; replaced
this in favor of using a CountDownLatch.
2) Removed synchronization primitives for mState since these are always
operations on the main thread.
3) Replaced integer sTonesPlaying with an AtomicInteger; there was no
locking on that instance and it was possible for concurrently playing
InCallTonePlayer instances to make conflicting updates.

Another optimization was moving the state change and media player cleanup
from within the media player onCompletion callback into a finally block
which takes place after we're done waiting on the playback completion.

Test: Performed manual testing on device on a carrier with ringback; verified that ringback plays and stops as expected.
Test: Performed manual testing on device by disconneting calls and verifying that the audio mode changes to MODE_NORMAL.  Also verified through log inspection that the locks and latches are performing as expected.
Test: Wrote new unit tests for InCallTonePlayer to verify that tone interruption results in signaling to CallAudioManager of the tone stop.
Fixes: 227404845
Change-Id: Ib90d1015887e0b6580950fff1c223f91e8490fb0
parent e3dfe2d9

src/com/android/server/telecom/InCallTonePlayer.java

+68 −67
Original line number Diff line number Diff line
@@ -33,6 +33,7 @@ import com.android.internal.annotations.VisibleForTesting; 


import java.util.concurrent.CountDownLatch;

import java.util.concurrent.TimeUnit;

import java.util.concurrent.atomic.AtomicInteger;



/**

 * Play a call-related tone (ringback, busy signal, etc.) either through ToneGenerator, or using a
@@ -186,7 +187,7 @@ public class InCallTonePlayer extends Thread { 
     * when we need focus and when it can be release. This should only be manipulated from the main

     * thread.

     */

    private static int sTonesPlaying = 0;

    private static AtomicInteger sTonesPlaying = new AtomicInteger(0);



    private final CallAudioManager mCallAudioManager;

    private final CallAudioRoutePeripheralAdapter mCallAudioRoutePeripheralAdapter;
@@ -212,6 +213,12 @@ public class InCallTonePlayer extends Thread { 
    private final MediaPlayerFactory mMediaPlayerFactory;

    private final AudioManagerAdapter mAudioManagerAdapter;



    /**

     * Latch used for awaiting on playback, which may be interrupted if the tone is stopped from

     * outside the playback.

     */

    private final CountDownLatch mPlaybackLatch = new CountDownLatch(1);



    /**

     * Initializes the tone player. Private; use the {@link Factory} to create tone players.

     *
@@ -388,26 +395,21 @@ public class InCallTonePlayer extends Thread { 
            }



            Log.i(this, "playToneGeneratorTone: toneType=%d", toneType);

            // TODO: Certain CDMA tones need to check the ringer-volume state before

            // playing. See CallNotifier.InCallTonePlayer.



            // TODO: Some tones play through the end of a call so we need to inform

            // CallAudioManager that we want focus the same way that Ringer does.



            synchronized (this) {

                if (mState != STATE_STOPPED) {

            mState = STATE_ON;

            toneGenerator.startTone(toneType);

            try {

                Log.v(this, "Starting tone %d...waiting for %d ms.", mToneId,

                        toneLengthMillis + TIMEOUT_BUFFER_MILLIS);

                        wait(toneLengthMillis + TIMEOUT_BUFFER_MILLIS);

                    } catch (InterruptedException e) {

                        Log.w(this, "wait interrupted", e);

                    }

                if (mPlaybackLatch.await(toneLengthMillis + TIMEOUT_BUFFER_MILLIS,

                        TimeUnit.MILLISECONDS)) {

                    Log.i(this, "playToneGeneratorTone: tone playback stopped.");

                }

            } catch (InterruptedException e) {

                Log.w(this, "playToneGeneratorTone: wait interrupted", e);

            }

            mState = STATE_OFF;

            // Redundant; don't want anyone re-using at this point.

            mState = STATE_STOPPED;

        } finally {

            if (toneGenerator != null) {

                toneGenerator.release();
@@ -421,10 +423,7 @@ public class InCallTonePlayer extends Thread { 
     * @param toneResourceId The resource ID of the tone to play.

     */

    private void playMediaTone(int stream, int toneResourceId) {

        synchronized (this) {

            if (mState != STATE_STOPPED) {

        mState = STATE_ON;

            }

        Log.i(this, "playMediaTone: toneResourceId=%d", toneResourceId);

        AudioAttributes attributes = new AudioAttributes.Builder()

                .setUsage(AudioAttributes.USAGE_VOICE_COMMUNICATION)
@@ -434,32 +433,32 @@ public class InCallTonePlayer extends Thread { 
        mToneMediaPlayer = mMediaPlayerFactory.get(toneResourceId, attributes);

        mToneMediaPlayer.setLooping(false);

        int durationMillis = mToneMediaPlayer.getDuration();

            final CountDownLatch toneLatch = new CountDownLatch(1);

        mToneMediaPlayer.setOnCompletionListener(new MediaPlayer.OnCompletionListener() {

            @Override

            public void onCompletion(MediaPlayer mp) {

                Log.i(InCallTonePlayer.this, "playMediaTone: toneResourceId=%d completed.",

                        toneResourceId);

                    synchronized (InCallTonePlayer.this) {

                        mState = STATE_OFF;

                    }

                    mToneMediaPlayer.release();

                    mToneMediaPlayer = null;

                    toneLatch.countDown();

                mPlaybackLatch.countDown();

            }

        });

            mToneMediaPlayer.start();



        try {

            mToneMediaPlayer.start();

            // Wait for the tone to stop playing; timeout at 2x the length of the file just to

                // be on the safe side.

                toneLatch.await(durationMillis * 2, TimeUnit.MILLISECONDS);

            // be on the safe side.  Playback can also be stopped via stopTone().

            if (mPlaybackLatch.await(durationMillis * 2, TimeUnit.MILLISECONDS)) {

                Log.i(this, "playMediaTone: tone playback stopped.");

            }

        } catch (InterruptedException ie) {

            Log.e(this, ie, "playMediaTone: tone playback interrupted.");

        } finally {

            // Redundant; don't want anyone re-using at this point.

            mState = STATE_STOPPED;

            mToneMediaPlayer.release();

            mToneMediaPlayer = null;

        }

    }



    }



    @VisibleForTesting

    public boolean startTone() {

        // Skip playing the end call tone if the volume is silenced.
@@ -468,8 +467,12 @@ public class InCallTonePlayer extends Thread { 
            return false;

        }



        sTonesPlaying++;

        if (sTonesPlaying == 1) {

        // Tone already done; don't allow re-used

        if (mState == STATE_STOPPED) {

            return false;

        }



        if (sTonesPlaying.incrementAndGet() == 1) {

            mCallAudioManager.setIsTonePlaying(true);

        }
@@ -494,18 +497,17 @@ public class InCallTonePlayer extends Thread { 
     */

    @VisibleForTesting

    public void stopTone() {

        synchronized (this) {

        if (mState == STATE_ON) {

                Log.d(this, "Stopping the tone %d.", mToneId);

                notify();

            Log.i(this, "stopTone: Stopping the tone %d.", mToneId);

            // Notify the playback to end early.

            mPlaybackLatch.countDown();

        }

        mState = STATE_STOPPED;

    }

    }



    @VisibleForTesting

    public void cleanup() {

        sTonesPlaying = 0;

        sTonesPlaying.set(0);

    }



    private void cleanUpTonePlayer() {
@@ -514,12 +516,11 @@ public class InCallTonePlayer extends Thread { 
        mMainThreadHandler.post(new Runnable("ICTP.cUTP", mLock) {

            @Override

            public void loggedRun() {

                if (sTonesPlaying == 0) {

                    Log.wtf(InCallTonePlayer.this,

                            "cleanUpTonePlayer(): Over-releasing focus for tone player.");

                } else if (--sTonesPlaying == 0) {

                int newToneCount = sTonesPlaying.updateAndGet( t -> Math.min(0, t--));



                if (newToneCount == 0) {

                    Log.i(InCallTonePlayer.this,

                            "cleanUpTonePlayer(): tonesPlaying=%d, tone completed", sTonesPlaying);

                            "cleanUpTonePlayer(): tonesPlaying=%d, tone completed", newToneCount);

                    if (mCallAudioManager != null) {

                        mCallAudioManager.setIsTonePlaying(false);

                    } else {
@@ -528,7 +529,7 @@ public class InCallTonePlayer extends Thread { 
                    }

                } else {

                    Log.i(InCallTonePlayer.this,

                            "cleanUpTonePlayer(): tonesPlaying=%d; still playing", sTonesPlaying);

                            "cleanUpTonePlayer(): tonesPlaying=%d; still playing", newToneCount);

                }

            }

        }.prepare());

tests/src/com/android/server/telecom/tests/InCallTonePlayerTest.java

+37 −3
Original line number Diff line number Diff line
@@ -87,7 +87,7 @@ public class InCallTonePlayerTest extends TelecomTestCase { 


        @Override

        public int getDuration() {

            return 0;

            return 1000;

        }

    };
@@ -134,7 +134,41 @@ public class InCallTonePlayerTest extends TelecomTestCase { 
        verify(mMediaPlayerFactory, never()).get(anyInt(), any());

    }



    @FlakyTest

    @SmallTest

    @Test

    public void testInterruptMediaTone() {

        when(mAudioManagerAdapter.isVolumeOverZero()).thenReturn(true);

        assertTrue(mInCallTonePlayer.startTone());

        // Verify we did play a tone.

        verify(mMediaPlayerFactory, timeout(5000)).get(anyInt(), any());

        verify(mCallAudioManager).setIsTonePlaying(eq(true));



        mInCallTonePlayer.stopTone();

        // Timeouts due to threads!

        verify(mCallAudioManager, timeout(5000)).setIsTonePlaying(eq(false));



        // Correctness check: ensure we can't start the tone again.

        assertFalse(mInCallTonePlayer.startTone());

    }



    @SmallTest

    @Test

    public void testInterruptToneGenerator() {

        mInCallTonePlayer = mFactory.createPlayer(InCallTonePlayer.TONE_RING_BACK);

        when(mAudioManagerAdapter.isVolumeOverZero()).thenReturn(true);

        assertTrue(mInCallTonePlayer.startTone());

        verify(mToneGenerator, timeout(5000)).startTone(anyInt());

        verify(mCallAudioManager).setIsTonePlaying(eq(true));



        mInCallTonePlayer.stopTone();

        // Timeouts due to threads!

        verify(mCallAudioManager, timeout(5000)).setIsTonePlaying(eq(false));

        verify(mToneGenerator, timeout(5000)).release();



        // Correctness check: ensure we can't start the tone again.

        assertFalse(mInCallTonePlayer.startTone());

    }



    @SmallTest

    @Test

    public void testEndCallToneWhenNotSilenced() {
@@ -143,6 +177,6 @@ public class InCallTonePlayerTest extends TelecomTestCase { 


        // Verify we did play a tone.

        verify(mMediaPlayerFactory, timeout(5000)).get(anyInt(), any());

        verify(mCallAudioManager).setIsTonePlaying(eq(true));

        verify(mCallAudioManager, timeout(5000)).setIsTonePlaying(eq(true));

    }

}