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

Commit eda85aaf authored by Santiago Seifert's avatar Santiago Seifert
Browse files

Prevent deadlock when calling BT or Audio 

Both Bluetooth and Audio APIs may require binder threads
to complete. As a result we must not block any binder threads
on calls to those APIs.

This method also adds TODOs for the changes that require changes
to the thread on which certain operations run.

Bug: b/329929065
Test: atest CtsMediaHostTestCases CtsMediaBetterTogetherTestCases AudioManagerRouteControllerTest
Test: Also manual using wired and bluetooth outputs.
Change-Id: Ided514035d657214233fa9a427949977a9ea35d5
parent 05ea0a25

services/core/java/com/android/server/media/AudioManagerRouteController.java

+80 −22
Original line number Diff line number Diff line
@@ -42,6 +42,7 @@ import android.util.Slog; 
import android.util.SparseArray;



import com.android.internal.R;

import com.android.internal.annotations.GuardedBy;

import com.android.server.media.BluetoothRouteController.NoOpBluetoothRouteController;



import java.util.HashMap;
@@ -57,8 +58,21 @@ import java.util.Objects; 
 *

 * <p>This implementation obtains and manages all routes via {@link AudioManager}, with the

 * exception of {@link AudioManager#handleBluetoothActiveDeviceChanged inactive bluetooth} routes

 * which are managed by {@link BluetoothDeviceRoutesManager}, which depends on the

 * bluetooth stack ({@link BluetoothAdapter} and related classes).

 * which are managed by {@link BluetoothDeviceRoutesManager}, which depends on the bluetooth stack

 * ({@link BluetoothAdapter} and related classes).

 *

 * <p>This class runs as part of the system_server process, but depends on classes that may

 * communicate with other processes, like bluetooth or audio server. And these other processes may

 * require binder threads from system server. As a result, there are a few threading considerations

 * to keep in mind:

 *

 * <ul>

 *   <li>Some of this class' internal state is synchronized using {@code this} as lock.

 *   <li>Binder threads may call into this class and run synchronized code.

 *   <li>As a result the above, in order to avoid deadlocks, calls to components that may call into

 *       other processes (like {@link AudioManager} or {@link BluetoothDeviceRoutesManager}) must

 *       not be synchronized nor occur on a binder thread.

 * </ul>

 */

/* package */ final class AudioManagerRouteController implements DeviceRouteController {

    private static final String TAG = SystemMediaRoute2Provider.TAG;
@@ -77,10 +91,6 @@ import java.util.Objects; 
    @NonNull private final OnDeviceRouteChangedListener mOnDeviceRouteChangedListener;

    @NonNull private final BluetoothDeviceRoutesManager mBluetoothRouteController;



    @NonNull

    private final Map<String, MediaRoute2InfoHolder> mRouteIdToAvailableDeviceRoutes =

            new HashMap<>();



    @NonNull private final AudioProductStrategy mStrategyForMedia;



    @NonNull private final AudioDeviceCallback mAudioDeviceCallback = new AudioDeviceCallbackImpl();
@@ -91,7 +101,14 @@ import java.util.Objects; 
    private final AudioManager.OnDevicesForAttributesChangedListener

            mOnDevicesForAttributesChangedListener = this::onDevicesForAttributesChangedListener;



    @NonNull private MediaRoute2Info mSelectedRoute;

    @GuardedBy("this")

    @NonNull

    private final Map<String, MediaRoute2InfoHolder> mRouteIdToAvailableDeviceRoutes =

            new HashMap<>();



    @GuardedBy("this")

    @NonNull

    private MediaRoute2Info mSelectedRoute;



    // TODO: b/305199571 - Support nullable btAdapter and strategyForMedia which, when null, means

    // no support for transferring to inactive bluetooth routes and transferring to any routes
@@ -170,7 +187,7 @@ import java.util.Objects; 


    @RequiresPermission(Manifest.permission.MODIFY_AUDIO_ROUTING)

    @Override

    public synchronized void transferTo(@Nullable String routeId) {

    public void transferTo(@Nullable String routeId) {

        if (routeId == null) {

            // This should never happen: This branch should only execute when the matching bluetooth

            // route controller is not the no-op one.
@@ -179,11 +196,17 @@ import java.util.Objects; 
            Slog.e(TAG, "Unexpected call to AudioPoliciesDeviceRouteController#transferTo(null)");

            return;

        }

        MediaRoute2InfoHolder mediaRoute2InfoHolder = mRouteIdToAvailableDeviceRoutes.get(routeId);

        MediaRoute2InfoHolder mediaRoute2InfoHolder;

        synchronized (this) {

            mediaRoute2InfoHolder = mRouteIdToAvailableDeviceRoutes.get(routeId);

        }

        if (mediaRoute2InfoHolder == null) {

            Slog.w(TAG, "transferTo: Ignoring transfer request to unknown route id : " + routeId);

            return;

        }

        // TODO: b/329929065 - Push audio manager and bluetooth operations to the handler, so that

        // they don't run on a binder thread, so as to prevent possible deadlocks (these operations

        // may need system_server binder threads to complete).

        if (mediaRoute2InfoHolder.mCorrespondsToInactiveBluetoothRoute) {

            // By default, the last connected device is the active route so we don't need to apply a

            // routing audio policy.
@@ -206,7 +229,7 @@ import java.util.Objects; 
                Manifest.permission.QUERY_AUDIO_STATE

            })

    @Override

    public synchronized boolean updateVolume(int volume) {

    public boolean updateVolume(int volume) {

        // TODO: b/305199571 - Optimize so that we only update the volume of the selected route. We

        // don't need to rebuild all available routes.

        rebuildAvailableRoutesAndNotify();
@@ -231,7 +254,7 @@ import java.util.Objects; 
                Manifest.permission.MODIFY_AUDIO_ROUTING,

                Manifest.permission.QUERY_AUDIO_STATE

            })

    private synchronized void rebuildAvailableRoutesAndNotify() {

    private void rebuildAvailableRoutesAndNotify() {

        rebuildAvailableRoutes();

        mOnDeviceRouteChangedListener.onDeviceRouteChanged();

    }
@@ -241,7 +264,7 @@ import java.util.Objects; 
                Manifest.permission.MODIFY_AUDIO_ROUTING,

                Manifest.permission.QUERY_AUDIO_STATE

            })

    private synchronized void rebuildAvailableRoutes() {

    private void rebuildAvailableRoutes() {

        List<AudioDeviceAttributes> attributesOfSelectedOutputDevices =

                mAudioManager.getDevicesForAttributes(MEDIA_USAGE_AUDIO_ATTRIBUTES);

        int selectedDeviceAttributesType;
@@ -260,8 +283,43 @@ import java.util.Objects; 
            selectedDeviceAttributesType = attributesOfSelectedOutputDevices.get(0).getType();

        }



        AudioDeviceInfo[] audioDeviceInfos =

                mAudioManager.getDevices(AudioManager.GET_DEVICES_OUTPUTS);

        updateAvailableRoutes(

                selectedDeviceAttributesType,

                /* audioDeviceInfos= */ mAudioManager.getDevices(AudioManager.GET_DEVICES_OUTPUTS),

                /* availableBluetoothRoutes= */ mBluetoothRouteController

                        .getAvailableBluetoothRoutes(),

                /* musicVolume= */ mAudioManager.getStreamVolume(AudioManager.STREAM_MUSIC),

                /* musicMaxVolume= */ mAudioManager.getStreamMaxVolume(AudioManager.STREAM_MUSIC),

                /* isVolumeFixed= */ mAudioManager.isVolumeFixed());

    }



    /**

     * Updates route and session info using the given information from {@link AudioManager}.

     *

     * <p>Synchronization is limited to this method in order to avoid calling into {@link

     * AudioManager} or {@link BluetoothDeviceRoutesManager} while holding a lock that may also be

     * acquired by binder threads. See class javadoc for more details.

     *

     * @param selectedDeviceAttributesType The {@link AudioDeviceInfo#getType() type} that

     *     corresponds to the currently selected route.

     * @param audioDeviceInfos The available audio outputs as obtained from {@link

     *     AudioManager#getDevices}.

     * @param availableBluetoothRoutes The available bluetooth routes as obtained from {@link

     *     BluetoothDeviceRoutesManager#getAvailableBluetoothRoutes()}.

     * @param musicVolume The volume of the music stream as obtained from {@link

     *     AudioManager#getStreamVolume}.

     * @param musicMaxVolume The max volume of the music stream as obtained from {@link

     *     AudioManager#getStreamMaxVolume}.

     * @param isVolumeFixed Whether the volume is fixed as obtained from {@link

     *     AudioManager#isVolumeFixed()}.

     */

    private synchronized void updateAvailableRoutes(

            int selectedDeviceAttributesType,

            AudioDeviceInfo[] audioDeviceInfos,

            List<MediaRoute2Info> availableBluetoothRoutes,

            int musicVolume,

            int musicMaxVolume,

            boolean isVolumeFixed) {

        mRouteIdToAvailableDeviceRoutes.clear();

        MediaRoute2InfoHolder newSelectedRouteHolder = null;

        for (AudioDeviceInfo audioDeviceInfo : audioDeviceInfos) {
@@ -301,7 +359,8 @@ import java.util.Objects; 
            newSelectedRouteHolder = mRouteIdToAvailableDeviceRoutes.values().iterator().next();

        }

        MediaRoute2InfoHolder selectedRouteHolderWithUpdatedVolumeInfo =

                newSelectedRouteHolder.copyWithVolumeInfoFromAudioManager(mAudioManager);

                newSelectedRouteHolder.copyWithVolumeInfo(

                        musicVolume, musicMaxVolume, isVolumeFixed);

        mRouteIdToAvailableDeviceRoutes.put(

                newSelectedRouteHolder.mMediaRoute2Info.getId(),

                selectedRouteHolderWithUpdatedVolumeInfo);
@@ -309,7 +368,7 @@ import java.util.Objects; 


        // We only add those BT routes that we have not already obtained from audio manager (which

        // are active).

        mBluetoothRouteController.getAvailableBluetoothRoutes().stream()

        availableBluetoothRoutes.stream()

                .filter(it -> !mRouteIdToAvailableDeviceRoutes.containsKey(it.getId()))

                .map(MediaRoute2InfoHolder::createForInactiveBluetoothRoute)

                .forEach(
@@ -430,17 +489,16 @@ import java.util.Objects; 
            mCorrespondsToInactiveBluetoothRoute = correspondsToInactiveBluetoothRoute;

        }



        public MediaRoute2InfoHolder copyWithVolumeInfoFromAudioManager(

                AudioManager mAudioManager) {

        public MediaRoute2InfoHolder copyWithVolumeInfo(

                int musicVolume, int musicMaxVolume, boolean isVolumeFixed) {

            MediaRoute2Info routeInfoWithVolumeInfo =

                    new MediaRoute2Info.Builder(mMediaRoute2Info)

                            .setVolumeHandling(

                                    mAudioManager.isVolumeFixed()

                                    isVolumeFixed

                                            ? MediaRoute2Info.PLAYBACK_VOLUME_FIXED

                                            : MediaRoute2Info.PLAYBACK_VOLUME_VARIABLE)

                            .setVolume(mAudioManager.getStreamVolume(AudioManager.STREAM_MUSIC))

                            .setVolumeMax(

                                    mAudioManager.getStreamMaxVolume(AudioManager.STREAM_MUSIC))

                            .setVolume(musicVolume)

                            .setVolumeMax(musicMaxVolume)

                            .build();

            return new MediaRoute2InfoHolder(

                    routeInfoWithVolumeInfo,

services/core/java/com/android/server/media/SystemMediaRoute2Provider.java

+2 −0
Original line number Diff line number Diff line
@@ -646,6 +646,8 @@ class SystemMediaRoute2Provider extends MediaRoute2Provider { 
                return;

            }



            // TODO: b/310145678 - Post this to mHandler once mHandler does not run on the main

            // thread.

            updateVolume();

        }

    }